diff options
Diffstat (limited to 'spec/services')
97 files changed, 2592 insertions, 1659 deletions
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 2f920de7fc7..8f81c1967d5 100644 --- a/spec/services/alert_management/process_prometheus_alert_service_spec.rb +++ b/spec/services/alert_management/process_prometheus_alert_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do end describe '#execute' do - let(:service) { described_class.new(project, nil, payload) } + let(:service) { described_class.new(project, payload) } let(:auto_close_incident) { true } let(:create_issue) { true } let(:send_email) { true } diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb index 03e55b3ec46..46483fede97 100644 --- a/spec/services/application_settings/update_service_spec.rb +++ b/spec/services/application_settings/update_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe ApplicationSettings::UpdateService do expect(application_settings.terms).to eq('Be nice!') end - it 'Only queries once when the terms are changed' do + it 'only queries once when the terms are changed' do create(:term, terms: 'Other terms') expect(application_settings.terms).to eq('Other terms') @@ -257,7 +257,7 @@ RSpec.describe ApplicationSettings::UpdateService do described_class.new(application_settings, admin, { external_authorization_service_enabled: false }).execute end - it 'does validate labels if external authorization gets enabled ' do + it 'does validate labels if external authorization gets enabled' do expect_any_instance_of(described_class).to receive(:validate_classification_label) described_class.new(application_settings, admin, { external_authorization_service_enabled: true }).execute @@ -350,4 +350,28 @@ RSpec.describe ApplicationSettings::UpdateService do expect(application_settings.issues_create_limit).to eq(600) end end + + context 'when require_admin_approval_after_user_signup changes' do + context 'when it goes from enabled to disabled' do + let(:params) { { require_admin_approval_after_user_signup: false } } + + it 'calls ApproveBlockedPendingApprovalUsersWorker' do + expect(ApproveBlockedPendingApprovalUsersWorker).to receive(:perform_async) + + subject.execute + end + end + + context 'when it goes from disabled to enabled' do + let(:params) { { require_admin_approval_after_user_signup: true } } + + it 'does not call ApproveBlockedPendingApprovalUsersWorker' do + application_settings.update!(require_admin_approval_after_user_signup: false) + + expect(ApproveBlockedPendingApprovalUsersWorker).not_to receive(:perform_async) + + subject.execute + 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 90ef32f1c5c..ba7acd3d3df 100644 --- a/spec/services/auth/container_registry_authentication_service_spec.rb +++ b/spec/services/auth/container_registry_authentication_service_spec.rb @@ -5,993 +5,5 @@ require 'spec_helper' RSpec.describe Auth::ContainerRegistryAuthenticationService do include AdminModeHelper - let(:current_project) { nil } - let(:current_user) { nil } - let(:current_params) { {} } - let(:rsa_key) { OpenSSL::PKey::RSA.generate(512) } - let(:payload) { JWT.decode(subject[:token], rsa_key, true, { algorithm: 'RS256' }).first } - - let(:authentication_abilities) do - [:read_container_image, :create_container_image, :admin_container_image] - end - - subject do - described_class.new(current_project, current_user, current_params) - .execute(authentication_abilities: authentication_abilities) - end - - before do - allow(Gitlab.config.registry).to receive_messages(enabled: true, issuer: 'rspec', key: nil) - allow_next_instance_of(JSONWebToken::RSAToken) do |instance| - allow(instance).to receive(:key).and_return(rsa_key) - end - end - - shared_examples 'an authenticated' do - it { is_expected.to include(:token) } - it { expect(payload).to include('access') } - end - - shared_examples 'a valid token' do - it { is_expected.to include(:token) } - it { expect(payload).to include('access') } - - context 'a expirable' do - let(:expires_at) { Time.zone.at(payload['exp']) } - let(:expire_delay) { 10 } - - context 'for default configuration' do - it { expect(expires_at).not_to be_within(2.seconds).of(Time.current + expire_delay.minutes) } - end - - context 'for changed configuration' do - before do - stub_application_setting(container_registry_token_expire_delay: expire_delay) - end - - it { expect(expires_at).to be_within(2.seconds).of(Time.current + expire_delay.minutes) } - end - end - end - - shared_examples 'a browsable' do - let(:access) do - [{ 'type' => 'registry', - 'name' => 'catalog', - 'actions' => ['*'] }] - end - - it_behaves_like 'a valid token' - it_behaves_like 'not a container repository factory' - - it 'has the correct scope' do - expect(payload).to include('access' => access) - end - end - - shared_examples 'an accessible' do - let(:access) do - [{ 'type' => 'repository', - 'name' => project.full_path, - 'actions' => actions }] - end - - it_behaves_like 'a valid token' - - it 'has the correct scope' do - expect(payload).to include('access' => access) - end - end - - shared_examples 'an inaccessible' do - it_behaves_like 'a valid token' - it { expect(payload).to include('access' => []) } - end - - shared_examples 'a deletable' do - it_behaves_like 'an accessible' do - let(:actions) { ['*'] } - end - end - - shared_examples 'a deletable since registry 2.7' do - it_behaves_like 'an accessible' do - let(:actions) { ['delete'] } - end - end - - shared_examples 'a pullable' do - it_behaves_like 'an accessible' do - let(:actions) { ['pull'] } - end - end - - shared_examples 'a pushable' do - it_behaves_like 'an accessible' do - let(:actions) { ['push'] } - end - end - - shared_examples 'a pullable and pushable' do - it_behaves_like 'an accessible' do - let(:actions) { %w(pull push) } - end - end - - shared_examples 'a forbidden' do - it { is_expected.to include(http_status: 403) } - it { is_expected.not_to include(:token) } - end - - shared_examples 'container repository factory' do - it 'creates a new container repository resource' do - expect { subject } - .to change { project.container_repositories.count }.by(1) - end - end - - shared_examples 'not a container repository factory' do - it 'does not create a new container repository resource' do - expect { subject }.not_to change { ContainerRepository.count } - end - end - - describe '#full_access_token' do - let_it_be(:project) { create(:project) } - let(:token) { described_class.full_access_token(project.full_path) } - - subject { { token: token } } - - it_behaves_like 'an accessible' do - let(:actions) { ['*'] } - end - - it_behaves_like 'not a container repository factory' - end - - describe '#pull_access_token' do - let_it_be(:project) { create(:project) } - let(:token) { described_class.pull_access_token(project.full_path) } - - subject { { token: token } } - - it_behaves_like 'an accessible' do - let(:actions) { ['pull'] } - end - - it_behaves_like 'not a container repository factory' - end - - context 'user authorization' do - let_it_be(:current_user) { create(:user) } - - context 'for registry catalog' do - let(:current_params) do - { scopes: ["registry:catalog:*"] } - end - - context 'disallow browsing for users without GitLab admin rights' do - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - - context 'for private project' do - 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_all do - project.add_developer(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a pushable' - it_behaves_like 'container repository factory' - end - - context 'disallow developer to delete images' do - before_all do - project.add_developer(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - - it 'logs an auth warning' do - expect(Gitlab::AuthLogger).to receive(:warn).with( - message: 'Denied container registry permissions', - scope_type: 'repository', - requested_project_path: project.full_path, - requested_actions: ['*'], - authorized_actions: [], - user_id: current_user.id, - username: current_user.username - ) - - subject - end - end - - context 'disallow developer to delete images since registry 2.7' do - before_all do - project.add_developer(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'allow reporter to pull images' do - before_all do - project.add_reporter(current_user) - end - - context 'when pulling from root level repository' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - end - - context 'disallow reporter to delete images' do - before_all do - project.add_reporter(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow reporter to delete images since registry 2.7' do - before_all do - project.add_reporter(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'return a least of privileges' do - before_all do - project.add_reporter(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push,pull"] } - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'disallow guest to pull or push images' do - before_all do - project.add_guest(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,push"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow guest to delete images' do - before_all do - project.add_guest(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow guest to delete images since registry 2.7' do - before_all do - project.add_guest(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - context 'allow anyone to pull images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to push images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images since registry 2.7' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'when repository name is invalid' do - let(:current_params) do - { scopes: ['repository:invalid:push'] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - - context 'for internal project' do - let_it_be(:project) { create(:project, :internal) } - - context 'for internal user' do - context 'allow anyone to pull images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to push images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images since registry 2.7' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - - context 'for external user' do - context 'disallow anyone to pull or push images' do - let_it_be(:current_user) { create(:user, external: true) } - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,push"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images' do - let_it_be(:current_user) { create(:user, external: true) } - let(:current_params) do - { scopes: ["repository:#{project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'disallow anyone to delete images since registry 2.7' do - let_it_be(:current_user) { create(:user, external: true) } - let(:current_params) do - { scopes: ["repository:#{project.full_path}:delete"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - end - end - - context 'delete authorized as maintainer' do - let_it_be(:current_project) { create(:project) } - let_it_be(:current_user) { create(:user) } - - let(:authentication_abilities) do - [:admin_container_image] - end - - before_all do - current_project.add_maintainer(current_user) - end - - it_behaves_like 'a valid token' - - context 'allow to delete images' do - let(:current_params) do - { scopes: ["repository:#{current_project.full_path}:*"] } - end - - it_behaves_like 'a deletable' do - let(:project) { current_project } - end - end - - context 'allow to delete images since registry 2.7' do - let(:current_params) do - { scopes: ["repository:#{current_project.full_path}:delete"] } - end - - it_behaves_like 'a deletable since registry 2.7' do - let(:project) { current_project } - end - end - end - - context 'build authorized as user' do - 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_all do - current_project.add_developer(current_user) - end - - context 'allow to use offline_token' do - let(:current_params) do - { offline_token: true } - end - - it_behaves_like 'an authenticated' - end - - it_behaves_like 'a valid token' - - context 'allow to pull and push images' do - let(:current_params) do - { scopes: ["repository:#{current_project.full_path}:pull,push"] } - end - - it_behaves_like 'a pullable and pushable' do - let(:project) { current_project } - end - - it_behaves_like 'container repository factory' do - let(:project) { current_project } - end - end - - context 'allow to delete images since registry 2.7' do - let(:current_params) do - { scopes: ["repository:#{current_project.full_path}:delete"] } - end - - it_behaves_like 'a deletable since registry 2.7' do - let(:project) { current_project } - end - end - - context 'disallow to delete images' do - let(:current_params) do - { scopes: ["repository:#{current_project.full_path}:*"] } - end - - it_behaves_like 'an inaccessible' do - let(:project) { current_project } - end - end - - context 'for other projects' do - context 'when pulling' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - context 'allow for public' do - let_it_be(:project) { create(:project, :public) } - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - shared_examples 'pullable for being team member' do - context 'when you are not member' do - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'when you are member' do - before_all do - project.add_developer(current_user) - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'when you are owner' do - let_it_be(:project) { create(:project, namespace: current_user.namespace) } - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - end - - context 'for private' do - let_it_be(:project) { create(:project, :private) } - - it_behaves_like 'pullable for being team member' - - context 'when you are admin' do - let_it_be(:current_user) { create(:admin) } - - context 'when you are not member' do - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'when you are member' do - before_all do - project.add_developer(current_user) - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'when you are owner' do - let_it_be(:project) { create(:project, namespace: current_user.namespace) } - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - end - end - end - - context 'when pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - context 'disallow for all' do - context 'when you are member' do - let_it_be(:project) { create(:project, :public) } - - before_all do - project.add_developer(current_user) - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - - context 'when you are owner' do - let_it_be(:project) { create(:project, :public, namespace: current_user.namespace) } - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - end - end - - context 'for project without container registry' do - let_it_be(:project) { create(:project, :public, container_registry_enabled: false) } - - before do - project.update!(container_registry_enabled: false) - end - - context 'disallow when pulling' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - - context 'for project that disables repository' do - let_it_be(:project) { create(:project, :public, :repository_disabled) } - - context 'disallow when pulling' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'an inaccessible' - it_behaves_like 'not a container repository factory' - end - end - end - - context 'registry catalog browsing authorized as admin' do - let_it_be(:current_user) { create(:user, :admin) } - let_it_be(:project) { create(:project, :public) } - - let(:current_params) do - { scopes: ["registry:catalog:*"] } - end - - it_behaves_like 'a browsable' - end - - context 'support for multiple scopes' do - let_it_be(:internal_project) { create(:project, :internal) } - let_it_be(:private_project) { create(:project, :private) } - - let(:current_params) do - { - scopes: [ - "repository:#{internal_project.full_path}:pull", - "repository:#{private_project.full_path}:pull" - ] - } - end - - context 'user has access to all projects' do - 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 - [ - { 'type' => 'repository', - 'name' => internal_project.full_path, - 'actions' => ['pull'] }, - { 'type' => 'repository', - 'name' => private_project.full_path, - 'actions' => ['pull'] } - ] - end - end - end - - context 'user only has access to internal project' do - let_it_be(:current_user) { create(:user) } - - it_behaves_like 'a browsable' do - let(:access) do - [ - { 'type' => 'repository', - 'name' => internal_project.full_path, - 'actions' => ['pull'] } - ] - end - end - end - - context 'anonymous access is rejected' do - let(:current_user) { nil } - - it_behaves_like 'a forbidden' - end - end - - context 'unauthorized' do - context 'disallow to use scope-less authentication' do - it_behaves_like 'a forbidden' - it_behaves_like 'not a container repository factory' - end - - context 'for invalid scope' do - let(:current_params) do - { scopes: ['invalid:aa:bb'] } - end - - it_behaves_like 'a forbidden' - it_behaves_like 'not a container repository factory' - end - - context 'for private project' do - let_it_be(:project) { create(:project, :private) } - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - it_behaves_like 'a forbidden' - end - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - context 'when pulling and pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull,push"] } - end - - it_behaves_like 'a pullable' - it_behaves_like 'not a container repository factory' - end - - context 'when pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a forbidden' - it_behaves_like 'not a container repository factory' - end - end - - context 'for registry catalog' do - let(:current_params) do - { scopes: ["registry:catalog:*"] } - end - - it_behaves_like 'a forbidden' - it_behaves_like 'not a container repository factory' - end - end - - context 'for deploy tokens' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:pull"] } - end - - context 'when deploy token has read and write registry as scopes' do - let(:current_user) { create(:deploy_token, write_registry: true, projects: [project]) } - - shared_examples 'able to login' do - context 'registry provides read_container_image authentication_abilities' do - let(:current_params) { {} } - let(:authentication_abilities) { [:read_container_image] } - - it_behaves_like 'an authenticated' - end - end - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - context 'when pulling' do - it_behaves_like 'a pullable' - end - - context 'when pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a pushable' - end - - it_behaves_like 'able to login' - end - - context 'for internal project' do - let_it_be(:project) { create(:project, :internal) } - - context 'when pulling' do - it_behaves_like 'a pullable' - end - - context 'when pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a pushable' - end - - it_behaves_like 'able to login' - end - - context 'for private project' do - let_it_be(:project) { create(:project, :private) } - - context 'when pulling' do - it_behaves_like 'a pullable' - end - - context 'when pushing' do - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a pushable' - end - - it_behaves_like 'able to login' - end - end - - context 'when deploy token does not have read_registry scope' do - let(:current_user) { create(:deploy_token, projects: [project], read_registry: false) } - - shared_examples 'unable to login' do - context 'registry provides no container authentication_abilities' do - let(:current_params) { {} } - let(:authentication_abilities) { [] } - - it_behaves_like 'a forbidden' - end - - context 'registry provides inapplicable container authentication_abilities' do - let(:current_params) { {} } - let(:authentication_abilities) { [:download_code] } - - it_behaves_like 'a forbidden' - end - end - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - context 'when pulling' do - it_behaves_like 'a pullable' - end - - it_behaves_like 'unable to login' - end - - context 'for internal project' do - let_it_be(:project) { create(:project, :internal) } - - context 'when pulling' do - it_behaves_like 'an inaccessible' - end - - it_behaves_like 'unable to login' - end - - context 'for private project' do - let_it_be(:project) { create(:project, :internal) } - - context 'when pulling' do - it_behaves_like 'an inaccessible' - end - - context 'when logging in' do - let(:current_params) { {} } - let(:authentication_abilities) { [] } - - it_behaves_like 'a forbidden' - end - - it_behaves_like 'unable to login' - end - end - - context 'when deploy token is not related to the project' do - let_it_be(:current_user) { create(:deploy_token, read_registry: false) } - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - context 'when pulling' do - it_behaves_like 'a pullable' - end - end - - context 'for internal project' do - let_it_be(:project) { create(:project, :internal) } - - context 'when pulling' do - it_behaves_like 'an inaccessible' - end - end - - context 'for private project' do - let_it_be(:project) { create(:project, :internal) } - - context 'when pulling' do - it_behaves_like 'an inaccessible' - end - end - end - - context 'when deploy token has been revoked' do - let(:current_user) { create(:deploy_token, :revoked, projects: [project]) } - - context 'for public project' do - let_it_be(:project) { create(:project, :public) } - - it_behaves_like 'a pullable' - end - - context 'for internal project' do - let_it_be(:project) { create(:project, :internal) } - - it_behaves_like 'an inaccessible' - end - - context 'for private project' do - let_it_be(:project) { create(:project, :internal) } - - it_behaves_like 'an inaccessible' - end - end - end - - context 'user authorization' do - let_it_be(:current_user) { create(:user) } - - context 'with multiple scopes' do - let_it_be(:project) { create(:project) } - - context 'allow developer to push images' do - before_all do - project.add_developer(current_user) - end - - let(:current_params) do - { scopes: ["repository:#{project.full_path}:push"] } - end - - it_behaves_like 'a pushable' - it_behaves_like 'container repository factory' - end - end - end + it_behaves_like 'a container registry auth service' end diff --git a/spec/services/auth/dependency_proxy_authentication_service_spec.rb b/spec/services/auth/dependency_proxy_authentication_service_spec.rb new file mode 100644 index 00000000000..ba50149f53a --- /dev/null +++ b/spec/services/auth/dependency_proxy_authentication_service_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Auth::DependencyProxyAuthenticationService do + let_it_be(:user) { create(:user) } + let(:service) { Auth::DependencyProxyAuthenticationService.new(nil, user) } + + before do + stub_config(dependency_proxy: { enabled: true }) + end + + describe '#execute' do + subject { service.execute(authentication_abilities: nil) } + + context 'dependency proxy is not enabled' do + before do + stub_config(dependency_proxy: { enabled: false }) + end + + it 'returns not found' do + result = subject + + expect(result[:http_status]).to eq(404) + expect(result[:message]).to eq('dependency proxy not enabled') + end + end + + context 'without a user' do + let(:user) { nil } + + it 'returns forbidden' do + result = subject + + expect(result[:http_status]).to eq(403) + expect(result[:message]).to eq('access forbidden') + end + end + + context 'with a user' do + it 'returns a token' do + expect(subject[:token]).not_to be_nil + end + end + end +end diff --git a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb index 7b428550768..eaa5f723bec 100644 --- a/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/auto_merge/merge_when_pipeline_succeeds_service_spec.rb @@ -191,7 +191,7 @@ RSpec.describe AutoMerge::MergeWhenPipelineSucceedsService do expect(mr_merge_if_green_enabled.merge_user).to be nil end - it 'Posts a system note' do + it 'posts a system note' do note = mr_merge_if_green_enabled.notes.last expect(note.note).to include 'canceled the automatic merge' end diff --git a/spec/services/boards/lists/create_service_spec.rb b/spec/services/boards/lists/create_service_spec.rb index 88b6c3098d1..d639fdbb46a 100644 --- a/spec/services/boards/lists/create_service_spec.rb +++ b/spec/services/boards/lists/create_service_spec.rb @@ -5,27 +5,29 @@ require 'spec_helper' RSpec.describe Boards::Lists::CreateService do describe '#execute' do shared_examples 'creating board lists' do - let(:user) { create(:user) } + let_it_be(:user) { create(:user) } - subject(:service) { described_class.new(parent, user, label_id: label.id) } - - before do + before_all do parent.add_developer(user) end + subject(:service) { described_class.new(parent, user, label_id: label.id) } + context 'when board lists is empty' do it 'creates a new list at beginning of the list' do - list = service.execute(board) + response = service.execute(board) - expect(list.position).to eq 0 + expect(response.success?).to eq(true) + expect(response.payload[:list].position).to eq 0 end end context 'when board lists has the done list' do it 'creates a new list at beginning of the list' do - list = service.execute(board) + response = service.execute(board) - expect(list.position).to eq 0 + expect(response.success?).to eq(true) + expect(response.payload[:list].position).to eq 0 end end @@ -34,9 +36,10 @@ RSpec.describe Boards::Lists::CreateService do create(:list, board: board, position: 0) create(:list, board: board, position: 1) - list = service.execute(board) + response = service.execute(board) - expect(list.position).to eq 2 + expect(response.success?).to eq(true) + expect(response.payload[:list].position).to eq 2 end end @@ -44,32 +47,35 @@ RSpec.describe Boards::Lists::CreateService do it 'creates a new list at end of the label lists' do list1 = create(:list, board: board, position: 0) - list2 = service.execute(board) + list2 = service.execute(board).payload[:list] expect(list1.reload.position).to eq 0 expect(list2.reload.position).to eq 1 end end - context 'when provided label does not belongs to the parent' do - it 'raises an error' do + context 'when provided label does not belong to the parent' do + it 'returns an error' do label = create(:label, name: 'in-development') service = described_class.new(parent, user, label_id: label.id) - expect { service.execute(board) }.to raise_error(ActiveRecord::RecordNotFound) + response = service.execute(board) + + expect(response.success?).to eq(false) + expect(response.errors).to include('Label not found') end end context 'when backlog param is sent' do it 'creates one and only one backlog list' do service = described_class.new(parent, user, 'backlog' => true) - list = service.execute(board) + list = service.execute(board).payload[:list] expect(list.list_type).to eq('backlog') expect(list.position).to be_nil expect(list).to be_valid - another_backlog = service.execute(board) + another_backlog = service.execute(board).payload[:list] expect(another_backlog).to eq list end @@ -77,17 +83,17 @@ RSpec.describe Boards::Lists::CreateService do end context 'when board parent is a project' do - let(:parent) { create(:project) } - let(:board) { create(:board, project: parent) } - let(:label) { create(:label, project: parent, name: 'in-progress') } + let_it_be(:parent) { create(:project) } + let_it_be(:board) { create(:board, project: parent) } + let_it_be(:label) { create(:label, project: parent, name: 'in-progress') } it_behaves_like 'creating board lists' end context 'when board parent is a group' do - let(:parent) { create(:group) } - let(:board) { create(:board, group: parent) } - let(:label) { create(:group_label, group: parent, name: 'in-progress') } + let_it_be(:parent) { create(:group) } + let_it_be(:board) { create(:board, group: parent) } + let_it_be(:label) { create(:group_label, group: parent, name: 'in-progress') } it_behaves_like 'creating board lists' end diff --git a/spec/services/ci/compare_accessibility_reports_service_spec.rb b/spec/services/ci/compare_accessibility_reports_service_spec.rb index 6903a633eeb..e0b84219834 100644 --- a/spec/services/ci/compare_accessibility_reports_service_spec.rb +++ b/spec/services/ci/compare_accessibility_reports_service_spec.rb @@ -29,34 +29,4 @@ RSpec.describe Ci::CompareAccessibilityReportsService do end end end - - describe '#latest?' do - subject { service.latest?(base_pipeline, head_pipeline, data) } - - let!(:base_pipeline) { nil } - let!(:head_pipeline) { create(:ci_pipeline, :with_accessibility_reports, project: project) } - let!(:key) { service.send(:key, base_pipeline, head_pipeline) } - - context 'when cache key is latest' do - let(:data) { { key: key } } - - it { is_expected.to be_truthy } - end - - context 'when cache key is outdated' do - before do - head_pipeline.update_column(:updated_at, 10.minutes.ago) - end - - let(:data) { { key: key } } - - it { is_expected.to be_falsy } - end - - context 'when cache key is empty' do - let(:data) { { key: nil } } - - it { is_expected.to be_falsy } - end - end end diff --git a/spec/services/ci/compare_codequality_reports_service_spec.rb b/spec/services/ci/compare_codequality_reports_service_spec.rb new file mode 100644 index 00000000000..ef762a2e9ad --- /dev/null +++ b/spec/services/ci/compare_codequality_reports_service_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CompareCodequalityReportsService do + let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + + describe '#execute' do + subject { service.execute(base_pipeline, head_pipeline) } + + context 'when head pipeline has a codequality report' do + let(:base_pipeline) { nil } + let(:head_pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) } + + it 'returns status and data' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]).to match_schema('entities/codequality_reports_comparer') + end + end + + context 'when base and head pipelines have codequality reports' do + let(:base_pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) } + let(:head_pipeline) { create(:ci_pipeline, :with_codequality_reports, project: project) } + + it 'returns status and data' do + expect(subject[:status]).to eq(:parsed) + expect(subject[:data]).to match_schema('entities/codequality_reports_comparer') + end + end + end +end diff --git a/spec/services/ci/compare_reports_base_service_spec.rb b/spec/services/ci/compare_reports_base_service_spec.rb new file mode 100644 index 00000000000..9ce58c4972d --- /dev/null +++ b/spec/services/ci/compare_reports_base_service_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CompareReportsBaseService do + let(:service) { described_class.new(project) } + let(:project) { create(:project, :repository) } + + describe '#latest?' do + subject { service.latest?(base_pipeline, head_pipeline, data) } + + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } + let!(:key) { service.send(:key, base_pipeline, head_pipeline) } + + context 'when cache key is latest' do + let(:data) { { key: key } } + + it { is_expected.to be_truthy } + end + + context 'when cache key is outdated' do + before do + head_pipeline.update_column(:updated_at, 10.minutes.ago) + end + + let(:data) { { key: key } } + + it { is_expected.to be_falsy } + end + + context 'when cache key is empty' do + let(:data) { { key: nil } } + + it { is_expected.to be_falsy } + end + end +end diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb index 377c801b008..01d58b2095f 100644 --- a/spec/services/ci/compare_test_reports_service_spec.rb +++ b/spec/services/ci/compare_test_reports_service_spec.rb @@ -67,7 +67,7 @@ RSpec.describe Ci::CompareTestReportsService do # 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) + Ci::TestFailureHistoryService.new(head_pipeline).execute end it 'loads recent failures on limited test cases to avoid building up a huge DB query', :aggregate_failures do @@ -80,34 +80,4 @@ RSpec.describe Ci::CompareTestReportsService do end end end - - describe '#latest?' do - subject { service.latest?(base_pipeline, head_pipeline, data) } - - let!(:base_pipeline) { nil } - let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } - let!(:key) { service.send(:key, base_pipeline, head_pipeline) } - - context 'when cache key is latest' do - let(:data) { { key: key } } - - it { is_expected.to be_truthy } - end - - context 'when cache key is outdated' do - before do - head_pipeline.update_column(:updated_at, 10.minutes.ago) - end - - let(:data) { { key: key } } - - it { is_expected.to be_falsy } - end - - context 'when cache key is empty' do - let(:data) { { key: nil } } - - it { is_expected.to be_falsy } - end - end end diff --git a/spec/services/ci/create_job_artifacts_service_spec.rb b/spec/services/ci/create_job_artifacts_service_spec.rb index 72b0d220b11..29e51a23dea 100644 --- a/spec/services/ci/create_job_artifacts_service_spec.rb +++ b/spec/services/ci/create_job_artifacts_service_spec.rb @@ -24,7 +24,7 @@ RSpec.describe Ci::CreateJobArtifactsService do upload = Tempfile.new('upload') FileUtils.copy(path, upload.path) - UploadedFile.new(upload.path, params) + UploadedFile.new(upload.path, **params) end describe '#execute' do diff --git a/spec/services/ci/create_pipeline_service/merge_requests_spec.rb b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb new file mode 100644 index 00000000000..e5347faed6a --- /dev/null +++ b/spec/services/ci/create_pipeline_service/merge_requests_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::CreatePipelineService do + context 'merge requests handling' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:user) { project.owner } + + let(:ref) { 'refs/heads/feature' } + let(:source) { :push } + let(:service) { described_class.new(project, user, { ref: ref }) } + let(:pipeline) { service.execute(source) } + + before do + stub_ci_pipeline_yaml_file <<-EOS + workflow: + rules: + # do not create pipelines if merge requests are opened + - if: $CI_OPEN_MERGE_REQUESTS + when: never + + - if: $CI_COMMIT_BRANCH + + rspec: + script: echo Hello World + EOS + end + + context 'when pushing a change' do + context 'when a merge request already exists' do + let!(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'feature', + target_project: project, + target_branch: 'master') + end + + it 'does not create a pipeline' do + expect(pipeline).not_to be_persisted + end + end + + context 'when no merge request exists' do + it 'does create a pipeline' do + expect(pipeline.errors).to be_empty + expect(pipeline).to be_persisted + end + end + end + end +end diff --git a/spec/services/ci/create_pipeline_service/rules_spec.rb b/spec/services/ci/create_pipeline_service/rules_spec.rb index a0ff2fff0ef..ac6c4c188e4 100644 --- a/spec/services/ci/create_pipeline_service/rules_spec.rb +++ b/spec/services/ci/create_pipeline_service/rules_spec.rb @@ -93,6 +93,148 @@ RSpec.describe Ci::CreatePipelineService do end end end + + context 'with allow_failure and exit_codes', :aggregate_failures do + def find_job(name) + pipeline.builds.find_by(name: name) + end + + let(:config) do + <<-EOY + job-1: + script: exit 42 + allow_failure: + exit_codes: 42 + rules: + - if: $CI_COMMIT_REF_NAME == "master" + allow_failure: false + + job-2: + script: exit 42 + allow_failure: + exit_codes: 42 + rules: + - if: $CI_COMMIT_REF_NAME == "master" + allow_failure: true + + job-3: + script: exit 42 + allow_failure: + exit_codes: 42 + rules: + - if: $CI_COMMIT_REF_NAME == "master" + when: manual + EOY + end + + it 'creates a pipeline' do + expect(pipeline).to be_persisted + expect(build_names).to contain_exactly( + 'job-1', 'job-2', 'job-3' + ) + end + + it 'assigns job:allow_failure values to the builds' do + expect(find_job('job-1').allow_failure).to eq(false) + expect(find_job('job-2').allow_failure).to eq(true) + expect(find_job('job-3').allow_failure).to eq(false) + end + + it 'removes exit_codes if allow_failure is specified' do + expect(find_job('job-1').options.dig(:allow_failure_criteria)).to be_nil + expect(find_job('job-2').options.dig(:allow_failure_criteria)).to be_nil + expect(find_job('job-3').options.dig(:allow_failure_criteria, :exit_codes)).to eq([42]) + end + + context 'with ci_allow_failure_with_exit_codes disabled' do + before do + stub_feature_flags(ci_allow_failure_with_exit_codes: false) + end + + it 'does not persist allow_failure_criteria' do + expect(pipeline).to be_persisted + + expect(find_job('job-1').options.key?(:allow_failure_criteria)).to be_falsey + expect(find_job('job-2').options.key?(:allow_failure_criteria)).to be_falsey + expect(find_job('job-3').options.key?(:allow_failure_criteria)).to be_falsey + end + end + end + + context 'if:' do + context 'variables:' do + let(:config) do + <<-EOY + job: + script: "echo job1" + variables: + VAR1: my var 1 + VAR2: my var 2 + rules: + - if: $CI_COMMIT_REF_NAME =~ /master/ + variables: + VAR1: overridden var 1 + - if: $CI_COMMIT_REF_NAME =~ /feature/ + variables: + VAR2: overridden var 2 + VAR3: new var 3 + - when: on_success + EOY + end + + let(:job) { pipeline.builds.find_by(name: 'job') } + + context 'when matching to the first rule' do + let(:ref) { 'refs/heads/master' } + + it 'overrides VAR1' do + variables = job.scoped_variables_hash + + expect(variables['VAR1']).to eq('overridden var 1') + expect(variables['VAR2']).to eq('my var 2') + expect(variables['VAR3']).to be_nil + end + + context 'when FF ci_rules_variables is disabled' do + before do + stub_feature_flags(ci_rules_variables: false) + end + + it 'does not affect variables' do + variables = job.scoped_variables_hash + + expect(variables['VAR1']).to eq('my var 1') + expect(variables['VAR2']).to eq('my var 2') + expect(variables['VAR3']).to be_nil + end + end + end + + context 'when matching to the second rule' do + let(:ref) { 'refs/heads/feature' } + + it 'overrides VAR2 and adds VAR3' do + variables = job.scoped_variables_hash + + expect(variables['VAR1']).to eq('my var 1') + expect(variables['VAR2']).to eq('overridden var 2') + expect(variables['VAR3']).to eq('new var 3') + end + end + + context 'when no match' do + let(:ref) { 'refs/heads/wip' } + + it 'does not affect vars' do + variables = job.scoped_variables_hash + + expect(variables['VAR1']).to eq('my var 1') + expect(variables['VAR2']).to eq('my var 2') + expect(variables['VAR3']).to be_nil + end + end + end + end end context 'when workflow:rules are used' do diff --git a/spec/services/ci/generate_coverage_reports_service_spec.rb b/spec/services/ci/generate_coverage_reports_service_spec.rb index d39053adebc..d12a9268e7e 100644 --- a/spec/services/ci/generate_coverage_reports_service_spec.rb +++ b/spec/services/ci/generate_coverage_reports_service_spec.rb @@ -52,34 +52,4 @@ RSpec.describe Ci::GenerateCoverageReportsService do end end end - - describe '#latest?' do - subject { service.latest?(base_pipeline, head_pipeline, data) } - - let!(:base_pipeline) { nil } - let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } - let!(:key) { service.send(:key, base_pipeline, head_pipeline) } - - context 'when cache key is latest' do - let(:data) { { key: key } } - - it { is_expected.to be_truthy } - end - - context 'when cache key is outdated' do - before do - head_pipeline.update_column(:updated_at, 10.minutes.ago) - end - - let(:data) { { key: key } } - - it { is_expected.to be_falsy } - end - - context 'when cache key is empty' do - let(:data) { { key: nil } } - - it { is_expected.to be_falsy } - end - end end diff --git a/spec/services/ci/generate_terraform_reports_service_spec.rb b/spec/services/ci/generate_terraform_reports_service_spec.rb index 25bf96035b2..c9ac74e050c 100644 --- a/spec/services/ci/generate_terraform_reports_service_spec.rb +++ b/spec/services/ci/generate_terraform_reports_service_spec.rb @@ -64,33 +64,4 @@ RSpec.describe Ci::GenerateTerraformReportsService do end end end - - describe '#latest?' do - let_it_be(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } - - subject { described_class.new(project) } - - it 'returns true when cache key is latest' do - cache_key = subject.send(:key, nil, head_pipeline) - - result = subject.latest?(nil, head_pipeline, key: cache_key) - - expect(result).to eq(true) - end - - it 'returns false when cache key is outdated' do - cache_key = subject.send(:key, nil, head_pipeline) - head_pipeline.update_column(:updated_at, 10.minutes.ago) - - result = subject.latest?(nil, head_pipeline, key: cache_key) - - expect(result).to eq(false) - end - - it 'returns false when cache key is nil' do - result = subject.latest?(nil, head_pipeline, key: nil) - - expect(result).to eq(false) - 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 95c98c2b5ef..1735f4cfc97 100644 --- a/spec/services/ci/list_config_variables_service_spec.rb +++ b/spec/services/ci/list_config_variables_service_spec.rb @@ -2,7 +2,9 @@ require 'spec_helper' -RSpec.describe Ci::ListConfigVariablesService do +RSpec.describe Ci::ListConfigVariablesService, :use_clean_rails_memory_store_caching do + include ReactiveCachingHelpers + let(:project) { create(:project, :repository) } let(:user) { project.creator } let(:service) { described_class.new(project, user) } @@ -31,6 +33,10 @@ RSpec.describe Ci::ListConfigVariablesService do } end + before do + synchronous_reactive_cache(service) + 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: '' }) @@ -65,6 +71,8 @@ RSpec.describe Ci::ListConfigVariablesService do HEREDOC end end + + synchronous_reactive_cache(service) end it 'returns variable list' do @@ -77,6 +85,10 @@ RSpec.describe Ci::ListConfigVariablesService do let(:sha) { 'invalid-sha' } let(:ci_config) { nil } + before do + synchronous_reactive_cache(service) + end + it 'returns empty json' do expect(subject).to eq({}) end @@ -96,11 +108,44 @@ RSpec.describe Ci::ListConfigVariablesService do } end + before do + synchronous_reactive_cache(service) + end + it 'returns empty result' do expect(subject).to eq({}) end end + context 'when reading from cache' do + let(:sha) { 'master' } + let(:ci_config) { {} } + let(:reactive_cache_params) { [sha] } + let(:return_value) { { 'KEY1' => { value: 'val 1', description: 'description 1' } } } + + before do + stub_reactive_cache(service, return_value, reactive_cache_params) + end + + it 'returns variable list' do + expect(subject).to eq(return_value) + end + end + + context 'when the cache is empty' do + let(:sha) { 'master' } + let(:ci_config) { {} } + let(:reactive_cache_params) { [sha] } + + it 'returns nil and enquques the worker to fill cache' do + expect(ExternalServiceReactiveCachingWorker) + .to receive(:perform_async) + .with(service.class, service.id, *reactive_cache_params) + + expect(subject).to be_nil + end + end + private def stub_gitlab_ci_yml_for_sha(sha, result) diff --git a/spec/services/ci/pipelines/create_artifact_service_spec.rb b/spec/services/ci/pipelines/create_artifact_service_spec.rb index 6f177889ed3..4e9248d9d1a 100644 --- a/spec/services/ci/pipelines/create_artifact_service_spec.rb +++ b/spec/services/ci/pipelines/create_artifact_service_spec.rb @@ -7,7 +7,8 @@ RSpec.describe ::Ci::Pipelines::CreateArtifactService do subject { described_class.new.execute(pipeline) } context 'when pipeline has coverage reports' do - let(:pipeline) { create(:ci_pipeline, :with_coverage_reports) } + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project) } context 'when pipeline is finished' do it 'creates a pipeline artifact' do diff --git a/spec/services/ci/test_cases_service_spec.rb b/spec/services/ci/test_cases_service_spec.rb deleted file mode 100644 index b61d308640f..00000000000 --- a/spec/services/ci/test_cases_service_spec.rb +++ /dev/null @@ -1,94 +0,0 @@ -# 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/ci/test_failure_history_service_spec.rb b/spec/services/ci/test_failure_history_service_spec.rb new file mode 100644 index 00000000000..e858c85490d --- /dev/null +++ b/spec/services/ci/test_failure_history_service_spec.rb @@ -0,0 +1,192 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::TestFailureHistoryService, :aggregate_failures do + describe '#execute' do + let(:project) { create(:project) } + let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } + + subject(:execute_service) { described_class.new(pipeline).execute } + + context 'when pipeline has failed builds with test reports' do + before do + # The test report has 2 test case failures + create(:ci_build, :failed, :test_reports, pipeline: pipeline, project: project) + end + + 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 pipeline is not for the default branch' do + before do + pipeline.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(pipeline).execute }.not_to raise_error + + expect(Ci::TestCase.count).to eq(2) + expect(Ci::TestCaseFailure.count).to eq(2) + 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 + + context 'when number of failed test cases across multiple builds exceed the limit' do + before do + stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 2) + + # This other test report has 1 unique test case failure which brings us to 3 total failures across all builds + # thus exceeding the limit of 2 for MAX_TRACKABLE_FAILURES + create(:ci_build, :failed, :test_reports_with_duplicate_failed_test_names, pipeline: pipeline, project: project) + 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 test failure data have duplicates within the same payload (happens when the JUnit report has duplicate test case names but have different failures)' do + before do + # The test report has 2 test case failures but with the same test case keys + create(:ci_build, :failed, :test_reports_with_duplicate_failed_test_names, pipeline: pipeline, project: project) + end + + it 'does not fail but does not persist duplicate data' do + expect { execute_service }.not_to raise_error + + expect(Ci::TestCase.count).to eq(1) + expect(Ci::TestCaseFailure.count).to eq(1) + end + end + + context 'when pipeline has no failed builds with test reports' do + before do + create(:ci_build, :test_reports, pipeline: pipeline, project: project) + create(:ci_build, :failed, pipeline: pipeline, project: project) + 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 + + describe '#should_track_failures?' do + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project, ref: project.default_branch) } + + subject { described_class.new(pipeline).should_track_failures? } + + before do + create(:ci_build, :test_reports, :failed, pipeline: pipeline, project: project) + create(:ci_build, :test_reports, :failed, pipeline: pipeline, project: project) + end + + context 'when feature flag is enabled and pipeline ref is the default branch' do + it { is_expected.to eq(true) } + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(test_failure_history: false) + end + + it { is_expected.to eq(false) } + end + + context 'when pipeline is not equal to the project default branch' do + before do + pipeline.update_column(:ref, 'some-other-branch') + end + + it { is_expected.to eq(false) } + end + + context 'when total number of builds with failed tests exceeds the max number of trackable failures' do + before do + stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 1) + end + + it { is_expected.to eq(false) } + end + end + + describe '#async' do + let(:pipeline) { double(id: 1) } + let(:service) { described_class.new(pipeline) } + + context 'when service should track failures' do + before do + allow(service).to receive(:should_track_failures?).and_return(true) + end + + it 'enqueues the worker when #perform_if_needed is called' do + expect(Ci::TestFailureHistoryWorker).to receive(:perform_async).with(pipeline.id) + + service.async.perform_if_needed + end + end + + context 'when service should not track failures' do + before do + allow(service).to receive(:should_track_failures?).and_return(false) + end + + it 'does not enqueue the worker when #perform_if_needed is called' do + expect(Ci::TestFailureHistoryWorker).not_to receive(:perform_async) + + service.async.perform_if_needed + end + end + end +end diff --git a/spec/services/ci/update_build_state_service_spec.rb b/spec/services/ci/update_build_state_service_spec.rb index 2545909bf56..3112e5dda1b 100644 --- a/spec/services/ci/update_build_state_service_spec.rb +++ b/spec/services/ci/update_build_state_service_spec.rb @@ -80,7 +80,11 @@ RSpec.describe Ci::UpdateBuildStateService do context 'when build has a checksum' do let(:params) do - { checksum: 'crc32:12345678', state: 'failed', failure_reason: 'script_failure' } + { + output: { checksum: 'crc32:12345678', bytesize: 123 }, + failure_reason: 'script_failure', + state: 'failed' + } end context 'when build does not have associated trace chunks' do @@ -154,14 +158,74 @@ RSpec.describe Ci::UpdateBuildStateService do end context 'when trace checksum is valid' do - let(:params) { { checksum: 'crc32:ed82cd11', state: 'success' } } + let(:params) do + { output: { checksum: 'crc32:ed82cd11', bytesize: 4 }, state: 'success' } + end - it 'does not increment invalid trace metric' do + it 'does not increment invalid or corrupted trace metric' do execute_with_stubbed_metrics! expect(metrics) .not_to have_received(:increment_trace_operation) .with(operation: :invalid) + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :corrupted) + end + + context 'when using deprecated parameters' do + let(:params) do + { checksum: 'crc32:ed82cd11', state: 'success' } + end + + it 'does not increment invalid or corrupted trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :invalid) + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :corrupted) + end + end + end + + context 'when trace checksum is invalid and the log is corrupted' do + let(:params) do + { output: { checksum: 'crc32:12345678', bytesize: 1 }, state: 'success' } + end + + it 'increments invalid and corrupted trace metrics' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :invalid) + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :corrupted) + end + end + + context 'when trace checksum is invalid but the log seems fine' do + let(:params) do + { output: { checksum: 'crc32:12345678', bytesize: 4 }, state: 'success' } + end + + it 'does not increment corrupted trace metric' do + execute_with_stubbed_metrics! + + expect(metrics) + .to have_received(:increment_trace_operation) + .with(operation: :invalid) + + expect(metrics) + .not_to have_received(:increment_trace_operation) + .with(operation: :corrupted) end end diff --git a/spec/services/clusters/applications/create_service_spec.rb b/spec/services/clusters/applications/create_service_spec.rb index f93ae2c62f3..f3b420510a6 100644 --- a/spec/services/clusters/applications/create_service_spec.rb +++ b/spec/services/clusters/applications/create_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Clusters::Applications::CreateService do let(:cluster) { create(:cluster, :project, :provided_by_gcp) } let(:user) { create(:user) } - let(:params) { { application: 'helm' } } + let(:params) { { application: 'ingress' } } let(:service) { described_class.new(cluster, user, params) } describe '#execute' do @@ -23,16 +23,16 @@ RSpec.describe Clusters::Applications::CreateService do subject cluster.reload - end.to change(cluster, :application_helm) + end.to change(cluster, :application_ingress) end context 'application already installed' do - let!(:application) { create(:clusters_applications_helm, :installed, cluster: cluster) } + let!(:application) { create(:clusters_applications_ingress, :installed, cluster: cluster) } it 'does not create a new application' do expect do subject - end.not_to change(Clusters::Applications::Helm, :count) + end.not_to change(Clusters::Applications::Ingress, :count) end it 'schedules an upgrade for the application' do @@ -43,10 +43,6 @@ RSpec.describe Clusters::Applications::CreateService do end context 'known applications' do - before do - create(:clusters_applications_helm, :installed, cluster: cluster) - end - context 'ingress application' do let(:params) do { @@ -215,19 +211,17 @@ RSpec.describe Clusters::Applications::CreateService do using RSpec::Parameterized::TableSyntax - where(:application, :association, :allowed, :pre_create_helm, :pre_create_ingress) do - 'helm' | :application_helm | true | false | false - 'ingress' | :application_ingress | true | true | false - 'runner' | :application_runner | true | true | false - 'prometheus' | :application_prometheus | true | true | false - 'jupyter' | :application_jupyter | true | true | true + where(:application, :association, :allowed, :pre_create_ingress) do + 'ingress' | :application_ingress | true | false + 'runner' | :application_runner | true | false + 'prometheus' | :application_prometheus | true | false + 'jupyter' | :application_jupyter | true | true end with_them do before do klass = "Clusters::Applications::#{application.titleize}" allow_any_instance_of(klass.constantize).to receive(:make_scheduled!).and_call_original - create(:clusters_applications_helm, :installed, cluster: cluster) if pre_create_helm create(:clusters_applications_ingress, :installed, cluster: cluster, external_hostname: 'example.com') if pre_create_ingress end @@ -252,7 +246,7 @@ RSpec.describe Clusters::Applications::CreateService do it 'makes the application scheduled' do expect do subject - end.to change { Clusters::Applications::Helm.with_status(:scheduled).count }.by(1) + end.to change { Clusters::Applications::Ingress.with_status(:scheduled).count }.by(1) end it 'schedules an install via worker' do @@ -266,7 +260,7 @@ RSpec.describe Clusters::Applications::CreateService do end context 'when application is associated with a cluster' do - let(:application) { create(:clusters_applications_helm, :installable, cluster: cluster) } + let(:application) { create(:clusters_applications_ingress, :installable, cluster: cluster) } let(:worker_arguments) { [application.name, application.id] } it_behaves_like 'installable applications' @@ -280,7 +274,7 @@ RSpec.describe Clusters::Applications::CreateService do end context 'when installation is already in progress' do - let!(:application) { create(:clusters_applications_helm, :installing, cluster: cluster) } + let!(:application) { create(:clusters_applications_ingress, :installing, cluster: cluster) } it 'raises an exception' do expect { subject } @@ -295,7 +289,7 @@ RSpec.describe Clusters::Applications::CreateService do context 'when application is installed' do %i(installed updated).each do |status| - let(:application) { create(:clusters_applications_helm, status, cluster: cluster) } + let(:application) { create(:clusters_applications_ingress, status, cluster: cluster) } it 'schedules an upgrade via worker' do expect(ClusterUpgradeAppWorker) diff --git a/spec/services/clusters/applications/prometheus_health_check_service_spec.rb b/spec/services/clusters/applications/prometheus_health_check_service_spec.rb index fc5a80688e6..ee47d00f700 100644 --- a/spec/services/clusters/applications/prometheus_health_check_service_spec.rb +++ b/spec/services/clusters/applications/prometheus_health_check_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Clusters::Applications::PrometheusHealthCheckService, '#execute' RSpec.shared_examples 'sends alert' do it 'sends an alert' do expect_next_instance_of(Projects::Alerting::NotifyService) do |notify_service| - expect(notify_service).to receive(:execute).with(alerts_service.token) + expect(notify_service).to receive(:execute).with(integration.token, integration) end subject @@ -40,8 +40,8 @@ RSpec.describe Clusters::Applications::PrometheusHealthCheckService, '#execute' end context 'when cluster is project_type' do - let_it_be(:alerts_service) { create(:alerts_service) } - let_it_be(:project) { create(:project, alerts_service: alerts_service) } + let_it_be(:project) { create(:project) } + let_it_be(:integration) { create(:alert_management_http_integration, project: project) } let(:applications_prometheus_healthy) { true } let(:prometheus) { create(:clusters_applications_prometheus, status: prometheus_status_value, healthy: applications_prometheus_healthy) } let(:cluster) { create(:cluster, :project, application_prometheus: prometheus, projects: [project]) } diff --git a/spec/services/clusters/aws/authorize_role_service_spec.rb b/spec/services/clusters/aws/authorize_role_service_spec.rb index 302bae6e3ff..17bbc372675 100644 --- a/spec/services/clusters/aws/authorize_role_service_spec.rb +++ b/spec/services/clusters/aws/authorize_role_service_spec.rb @@ -40,7 +40,7 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do shared_examples 'bad request' do it 'returns an empty hash' do expect(subject.status).to eq(:unprocessable_entity) - expect(subject.body).to eq({}) + expect(subject.body).to eq({ message: message }) end it 'logs the error' do @@ -52,12 +52,14 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do context 'role does not exist' do let(:user) { create(:user) } + let(:message) { 'Error: Unable to find AWS role for current user' } include_examples 'bad request' end context 'supplied ARN is invalid' do let(:role_arn) { 'invalid' } + let(:message) { 'Validation failed: Role arn must be a valid Amazon Resource Name' } include_examples 'bad request' end @@ -69,18 +71,29 @@ RSpec.describe Clusters::Aws::AuthorizeRoleService do context 'error fetching credentials' do let(:error) { Aws::STS::Errors::ServiceError.new(nil, 'error message') } + let(:message) { 'AWS service error: error message' } + + include_examples 'bad request' + end + + context 'error in assuming role' do + let(:raw_message) { "User foo is not authorized to perform: sts:AssumeRole on resource bar" } + let(:error) { Aws::STS::Errors::AccessDenied.new(nil, raw_message) } + let(:message) { "Access denied: #{raw_message}" } include_examples 'bad request' end context 'credentials not configured' do let(:error) { Aws::Errors::MissingCredentialsError.new('error message') } + let(:message) { "Error: No AWS credentials were supplied" } include_examples 'bad request' end context 'role not configured' do let(:error) { Clusters::Aws::FetchCredentialsService::MissingRoleError.new('error message') } + let(:message) { "Error: No AWS provision role found for user" } include_examples 'bad request' end diff --git a/spec/services/clusters/aws/fetch_credentials_service_spec.rb b/spec/services/clusters/aws/fetch_credentials_service_spec.rb index 361a947f634..0358ca1f535 100644 --- a/spec/services/clusters/aws/fetch_credentials_service_spec.rb +++ b/spec/services/clusters/aws/fetch_credentials_service_spec.rb @@ -60,9 +60,7 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do 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')) - .and_return(session_policy) + stub_file_read(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json'), content: session_policy) end it { is_expected.to eq assumed_role_credentials } @@ -83,5 +81,59 @@ RSpec.describe Clusters::Aws::FetchCredentialsService do expect { subject }.to raise_error(described_class::MissingRoleError, 'AWS provisioning role not configured') end end + + context 'with an instance profile attached to an IAM role' do + let(:sts_client) { Aws::STS::Client.new(region: region, stub_responses: true) } + let(:provision_role) { create(:aws_role, user: user, region: 'custom-region') } + + before do + stub_application_setting(eks_access_key_id: nil) + stub_application_setting(eks_secret_access_key: nil) + + expect(Aws::STS::Client).to receive(:new) + .with(region: region) + .and_return(sts_client) + + expect(Aws::AssumeRoleCredentials).to receive(:new) + .with( + client: sts_client, + role_arn: provision_role.role_arn, + role_session_name: session_name, + external_id: provision_role.role_external_id, + policy: session_policy + ).and_call_original + end + + context 'provider is specified' do + let(:region) { provider.region } + let(:session_name) { "gitlab-eks-cluster-#{provider.cluster_id}-user-#{user.id}" } + let(:session_policy) { nil } + + it 'returns credentials', :aggregate_failures do + expect(subject.access_key_id).to be_present + expect(subject.secret_access_key).to be_present + expect(subject.session_token).to be_present + end + end + + context 'provider is not specifed' do + let(:provider) { nil } + let(:region) { provision_role.region } + let(:session_name) { "gitlab-eks-autofill-user-#{user.id}" } + let(:session_policy) { 'policy-document' } + + before do + stub_file_read(Rails.root.join('vendor', 'aws', 'iam', 'eks_cluster_read_only_policy.json'), content: session_policy) + end + + subject { described_class.new(provision_role, provider: provider).execute } + + it 'returns credentials', :aggregate_failures do + expect(subject.access_key_id).to be_present + expect(subject.secret_access_key).to be_present + expect(subject.session_token).to be_present + end + end + end end end diff --git a/spec/services/clusters/aws/provision_service_spec.rb b/spec/services/clusters/aws/provision_service_spec.rb index 52612e5ac40..5efac29ec1e 100644 --- a/spec/services/clusters/aws/provision_service_spec.rb +++ b/spec/services/clusters/aws/provision_service_spec.rb @@ -42,9 +42,7 @@ RSpec.describe Clusters::Aws::ProvisionService do allow(provider).to receive(:api_client) .and_return(client) - allow(File).to receive(:read) - .with(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml')) - .and_return(cloudformation_template) + stub_file_read(Rails.root.join('vendor', 'aws', 'cloudformation', 'eks_cluster.yaml'), content: cloudformation_template) end it 'updates the provider status to :creating and configures the provider with credentials' do diff --git a/spec/services/clusters/cleanup/app_service_spec.rb b/spec/services/clusters/cleanup/app_service_spec.rb index ba1be7448a4..ea1194d2100 100644 --- a/spec/services/clusters/cleanup/app_service_spec.rb +++ b/spec/services/clusters/cleanup/app_service_spec.rb @@ -67,7 +67,8 @@ RSpec.describe Clusters::Cleanup::AppService do it 'only uninstalls apps that are not dependencies for other installed apps' do expect(Clusters::Applications::UninstallWorker) - .not_to receive(:perform_async).with(helm.name, helm.id) + .to receive(:perform_async).with(helm.name, helm.id) + .and_call_original expect(Clusters::Applications::UninstallWorker) .not_to receive(:perform_async).with(ingress.name, ingress.id) @@ -85,7 +86,7 @@ RSpec.describe Clusters::Cleanup::AppService do it 'logs application uninstalls and next execution' do expect(logger).to receive(:info) - .with(log_meta.merge(event: :uninstalling_app, application: kind_of(String))).twice + .with(log_meta.merge(event: :uninstalling_app, application: kind_of(String))).exactly(3).times expect(logger).to receive(:info) .with(log_meta.merge(event: :scheduling_execution, next_execution: 1)) diff --git a/spec/services/concerns/exclusive_lease_guard_spec.rb b/spec/services/concerns/exclusive_lease_guard_spec.rb index d54ba6abadd..6a2aa0a377b 100644 --- a/spec/services/concerns/exclusive_lease_guard_spec.rb +++ b/spec/services/concerns/exclusive_lease_guard_spec.rb @@ -51,7 +51,7 @@ RSpec.describe ExclusiveLeaseGuard, :clean_gitlab_redis_shared_state do it 'does not call internal_method but logs error', :aggregate_failures do expect(subject).not_to receive(:internal_method) - expect(Gitlab::AppLogger).to receive(:error).with("Cannot obtain an exclusive lease for #{subject.class.name}. There must be another instance already in execution.") + expect(Gitlab::AppLogger).to receive(:error).with("Cannot obtain an exclusive lease for #{subject.lease_key}. There must be another instance already in execution.") subject.call end diff --git a/spec/services/container_expiration_policies/cleanup_service_spec.rb b/spec/services/container_expiration_policies/cleanup_service_spec.rb index 2da35cfc3fb..8438073ceb0 100644 --- a/spec/services/container_expiration_policies/cleanup_service_spec.rb +++ b/spec/services/container_expiration_policies/cleanup_service_spec.rb @@ -28,6 +28,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do 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) + expect(repository.expiration_policy_completed_at).not_to eq(nil) end end end @@ -45,6 +46,7 @@ RSpec.describe ContainerExpirationPolicies::CleanupService do 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) + expect(repository.expiration_policy_completed_at).to eq(nil) end end end diff --git a/spec/services/dependency_proxy/auth_token_service_spec.rb b/spec/services/dependency_proxy/auth_token_service_spec.rb new file mode 100644 index 00000000000..4b96f9d75a9 --- /dev/null +++ b/spec/services/dependency_proxy/auth_token_service_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::AuthTokenService do + include DependencyProxyHelpers + + describe '.decoded_token_payload' do + let_it_be(:user) { create(:user) } + let_it_be(:token) { build_jwt(user) } + + subject { described_class.decoded_token_payload(token.encoded) } + + it 'returns the user' do + result = subject + + expect(result['user_id']).to eq(user.id) + end + + it 'raises an error if the token is expired' do + travel_to(Time.zone.now + Auth::DependencyProxyAuthenticationService.token_expire_at + 1.minute) do + expect { subject }.to raise_error(JWT::ExpiredSignature) + end + end + + it 'raises an error if decoding fails' do + allow(JWT).to receive(:decode).and_raise(JWT::DecodeError) + + expect { subject }.to raise_error(JWT::DecodeError) + end + + it 'raises an error if signature is immature' do + allow(JWT).to receive(:decode).and_raise(JWT::ImmatureSignature) + + expect { subject }.to raise_error(JWT::ImmatureSignature) + end + end +end diff --git a/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb new file mode 100644 index 00000000000..c375e5a2fa3 --- /dev/null +++ b/spec/services/dependency_proxy/find_or_create_manifest_service_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::FindOrCreateManifestService do + include DependencyProxyHelpers + + let_it_be(:image) { 'alpine' } + let_it_be(:tag) { 'latest' } + let_it_be(:dependency_proxy_manifest) { create(:dependency_proxy_manifest, file_name: "#{image}:#{tag}.json") } + let(:manifest) { dependency_proxy_manifest.file.read } + let(:group) { dependency_proxy_manifest.group } + let(:token) { Digest::SHA256.hexdigest('123') } + let(:headers) { { 'docker-content-digest' => dependency_proxy_manifest.digest } } + + describe '#execute' do + subject { described_class.new(group, image, tag, token).execute } + + context 'when no manifest exists' do + let_it_be(:image) { 'new-image' } + + before do + stub_manifest_head(image, tag, digest: dependency_proxy_manifest.digest) + stub_manifest_download(image, tag, headers: headers) + end + + it 'downloads manifest from remote registry if there is no cached one', :aggregate_failures do + expect { subject }.to change { group.dependency_proxy_manifests.count }.by(1) + expect(subject[:status]).to eq(:success) + expect(subject[:manifest]).to be_a(DependencyProxy::Manifest) + expect(subject[:manifest]).to be_persisted + end + end + + context 'when manifest exists' do + before do + stub_manifest_head(image, tag, digest: dependency_proxy_manifest.digest) + end + + shared_examples 'using the cached manifest' do + it 'uses cached manifest instead of downloading one', :aggregate_failures do + expect(subject[:status]).to eq(:success) + expect(subject[:manifest]).to be_a(DependencyProxy::Manifest) + expect(subject[:manifest]).to eq(dependency_proxy_manifest) + end + end + + it_behaves_like 'using the cached manifest' + + context 'when digest is stale' do + let(:digest) { 'new-digest' } + + before do + stub_manifest_head(image, tag, digest: digest) + stub_manifest_download(image, tag, headers: { 'docker-content-digest' => digest }) + end + + it 'downloads the new manifest and updates the existing record', :aggregate_failures do + expect(subject[:status]).to eq(:success) + expect(subject[:manifest]).to eq(dependency_proxy_manifest) + expect(subject[:manifest].digest).to eq(digest) + end + end + + context 'failed connection' do + before do + expect(DependencyProxy::HeadManifestService).to receive(:new).and_raise(Net::OpenTimeout) + end + + it_behaves_like 'using the cached manifest' + + context 'and no manifest is cached' do + let_it_be(:image) { 'new-image' } + + it 'returns an error', :aggregate_failures do + expect(subject[:status]).to eq(:error) + expect(subject[:http_status]).to eq(503) + expect(subject[:message]).to eq('Failed to download the manifest from the external registry') + end + end + end + end + end +end diff --git a/spec/services/dependency_proxy/head_manifest_service_spec.rb b/spec/services/dependency_proxy/head_manifest_service_spec.rb new file mode 100644 index 00000000000..7c7ebe4d181 --- /dev/null +++ b/spec/services/dependency_proxy/head_manifest_service_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe DependencyProxy::HeadManifestService do + include DependencyProxyHelpers + + let(:image) { 'alpine' } + let(:tag) { 'latest' } + let(:token) { Digest::SHA256.hexdigest('123') } + let(:digest) { '12345' } + + subject { described_class.new(image, tag, token).execute } + + context 'remote request is successful' do + before do + stub_manifest_head(image, tag, digest: digest) + end + + it { expect(subject[:status]).to eq(:success) } + it { expect(subject[:digest]).to eq(digest) } + end + + context 'remote request is not found' do + before do + stub_manifest_head(image, tag, status: 404, body: '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, method: :head).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/pull_manifest_service_spec.rb b/spec/services/dependency_proxy/pull_manifest_service_spec.rb index 030ed9c001b..b760839d1fb 100644 --- a/spec/services/dependency_proxy/pull_manifest_service_spec.rb +++ b/spec/services/dependency_proxy/pull_manifest_service_spec.rb @@ -8,26 +8,43 @@ RSpec.describe DependencyProxy::PullManifestService do let(:tag) { '3.9' } let(:token) { Digest::SHA256.hexdigest('123') } let(:manifest) { { foo: 'bar' }.to_json } + let(:digest) { '12345' } + let(:headers) { { 'docker-content-digest' => digest } } - subject { described_class.new(image, tag, token).execute } + subject { described_class.new(image, tag, token).execute_with_manifest(&method(:check_response)) } context 'remote request is successful' do before do - stub_manifest_download(image, tag) + stub_manifest_download(image, tag, headers: headers) end - it { expect(subject[:status]).to eq(:success) } - it { expect(subject[:manifest]).to eq(manifest) } + it 'successfully returns the manifest' do + def check_response(response) + response[:file].rewind + + expect(response[:status]).to eq(:success) + expect(response[:file].read).to eq(manifest) + expect(response[:digest]).to eq(digest) + end + + subject + end end context 'remote request is not found' do before do - stub_manifest_download(image, tag, 404, 'Not found') + stub_manifest_download(image, tag, status: 404, body: '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') } + it 'returns a 404 not found error' do + def check_response(response) + expect(response[:status]).to eq(:error) + expect(response[:http_status]).to eq(404) + expect(response[:message]).to eq('Not found') + end + + subject + end end context 'net timeout exception' do @@ -37,8 +54,20 @@ RSpec.describe DependencyProxy::PullManifestService do 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') } + it 'returns a 599 error' do + def check_response(response) + expect(response[:status]).to eq(:error) + expect(response[:http_status]).to eq(599) + expect(response[:message]).to eq('execution expired') + end + + subject + end + end + + context 'no block is given' do + subject { described_class.new(image, tag, token).execute_with_manifest } + + it { expect { subject }.to raise_error(ArgumentError, 'Block must be provided') } end end diff --git a/spec/services/environments/canary_ingress/update_service_spec.rb b/spec/services/environments/canary_ingress/update_service_spec.rb new file mode 100644 index 00000000000..31d6f543817 --- /dev/null +++ b/spec/services/environments/canary_ingress/update_service_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Environments::CanaryIngress::UpdateService, :clean_gitlab_redis_cache do + include KubernetesHelpers + + let_it_be(:project, refind: true) { create(:project) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:reporter) { create(:user) } + let(:user) { maintainer } + let(:params) { {} } + let(:service) { described_class.new(project, user, params) } + + before_all do + project.add_maintainer(maintainer) + project.add_reporter(reporter) + end + + shared_examples_for 'failed request' do + it 'returns an error' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq(message) + end + end + + describe '#execute_async' do + subject { service.execute_async(environment) } + + let(:environment) { create(:environment, project: project) } + let(:params) { { weight: 50 } } + let(:canary_ingress) { ::Gitlab::Kubernetes::Ingress.new(kube_ingress(track: :canary)) } + + context 'when canary_ingress_weight_control feature flag is disabled' do + before do + stub_feature_flags(canary_ingress_weight_control: false) + end + + it_behaves_like 'failed request' do + let(:message) { "Feature flag is not enabled on the environment's project." } + end + end + + context 'when the actor does not have permission to update environment' do + let(:user) { reporter } + + it_behaves_like 'failed request' do + let(:message) { "You do not have permission to update the environment." } + end + end + + context 'when weight parameter is invalid' do + let(:params) { { weight: 'unknown' } } + + it_behaves_like 'failed request' do + let(:message) { 'Canary weight must be specified and valid range (0..100).' } + end + end + + context 'when no parameters exist' do + let(:params) { {} } + + it_behaves_like 'failed request' do + let(:message) { 'Canary weight must be specified and valid range (0..100).' } + end + end + + context 'when environment has a running deployment' do + before do + allow(environment).to receive(:has_running_deployments?) { true } + end + + it_behaves_like 'failed request' do + let(:message) { 'There are running deployments on the environment. Please retry later.' } + end + end + + context 'when canary ingress was updated recently' do + before do + allow(::Gitlab::ApplicationRateLimiter).to receive(:throttled?) { true } + end + + it_behaves_like 'failed request' do + let(:message) { "This environment's canary ingress has been updated recently. Please retry later." } + end + end + end + + describe '#execute' do + subject { service.execute(environment) } + + let(:environment) { create(:environment, project: project) } + let(:params) { { weight: 50 } } + let(:canary_ingress) { ::Gitlab::Kubernetes::Ingress.new(kube_ingress(track: :canary)) } + + context 'when canary ingress is present in the environment' do + before do + allow(environment).to receive(:ingresses) { [canary_ingress] } + end + + context 'when patch request succeeds' do + let(:patch_data) do + { + metadata: { + annotations: { + Gitlab::Kubernetes::Ingress::ANNOTATION_KEY_CANARY_WEIGHT => params[:weight].to_s + } + } + } + end + + before do + allow(environment).to receive(:patch_ingress).with(canary_ingress, patch_data) { true } + end + + it 'returns success' do + expect(subject[:status]).to eq(:success) + expect(subject[:message]).to be_nil + end + end + + context 'when patch request does not succeed' do + before do + allow(environment).to receive(:patch_ingress) { false } + end + + it_behaves_like 'failed request' do + let(:message) { 'Failed to update the Canary Ingress.' } + end + end + end + + context 'when canary ingress is not present in the environment' do + it_behaves_like 'failed request' do + let(:message) { 'Canary Ingress does not exist in the environment.' } + end + end + end +end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 3c67c15f10a..17b2c7b38e1 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe EventCreateService do tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today } expect { subject } - .to change { Gitlab::UsageDataCounters::TrackUniqueEvents.count_unique_events(tracking_params) } + .to change { Gitlab::UsageDataCounters::TrackUniqueEvents.count_unique_events(**tracking_params) } .by(1) end end @@ -386,7 +386,7 @@ RSpec.describe EventCreateService do counter_class = Gitlab::UsageDataCounters::TrackUniqueEvents tracking_params = { event_action: event_action, date_from: Date.yesterday, date_to: Date.today } - expect { subject }.not_to change { counter_class.count_unique_events(tracking_params) } + expect { subject }.not_to change { counter_class.count_unique_events(**tracking_params) } end end end diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb index 17e4645fde6..3823d027812 100644 --- a/spec/services/files/delete_service_spec.rb +++ b/spec/services/files/delete_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe Files::DeleteService do context "when the file's last commit sha does not match the supplied last_commit_sha" do let(:last_commit_sha) { "foo" } - it "returns a hash with the correct error message and a :error status " do + it "returns a hash with the correct error message and a :error status" do expect { subject.execute } .to raise_error(Files::UpdateService::FileChangedError, "You are attempting to delete a file that has been previously updated.") diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index 84d78b4c2bc..6d7459e0b29 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -34,7 +34,7 @@ RSpec.describe Files::UpdateService do context "when the file's last commit sha does not match the supplied last_commit_sha" do let(:last_commit_sha) { "foo" } - it "returns a hash with the correct error message and a :error status " do + it "returns a hash with the correct error message and a :error status" do expect { subject.execute } .to raise_error(Files::UpdateService::FileChangedError, "You are attempting to update a file that has changed since you started editing it.") @@ -44,7 +44,7 @@ RSpec.describe Files::UpdateService do context "when the file's last commit sha does match the supplied last_commit_sha" do let(:last_commit_sha) { Gitlab::Git::Commit.last_for_path(project.repository, project.default_branch, file_path).sha } - it "returns a hash with the :success status " do + it "returns a hash with the :success status" do results = subject.execute expect(results[:status]).to match(:success) @@ -68,7 +68,7 @@ RSpec.describe Files::UpdateService do end context "when the last_commit_sha is not supplied" do - it "returns a hash with the :success status " do + it "returns a hash with the :success status" do results = subject.execute expect(results[:status]).to match(:success) diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 5d73794c1ec..c7bf006dab0 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -718,10 +718,10 @@ RSpec.describe Git::BranchPushService, services: true do end shared_examples 'enqueues Jira sync worker' do - specify do + specify :aggregate_failures do Sidekiq::Testing.fake! do expect(JiraConnect::SyncBranchWorker).to receive(:perform_async) - .with(project.id, branch_to_sync, commits_to_sync) + .with(project.id, branch_to_sync, commits_to_sync, kind_of(Numeric)) .and_call_original expect { subject.execute }.to change(JiraConnect::SyncBranchWorker.jobs, :size).by(1) diff --git a/spec/services/git/tag_hooks_service_spec.rb b/spec/services/git/tag_hooks_service_spec.rb index 4443c46a414..dae2f63f2f9 100644 --- a/spec/services/git/tag_hooks_service_spec.rb +++ b/spec/services/git/tag_hooks_service_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Git::TagHooksService, :service do end describe 'System hooks' do - it 'Executes system hooks' do + it 'executes system hooks' do push_data = service.send(:push_data) expect(project).to receive(:has_active_hooks?).and_return(true) diff --git a/spec/services/groups/import_export/import_service_spec.rb b/spec/services/groups/import_export/import_service_spec.rb index f8cb55a9955..0c7765dcd38 100644 --- a/spec/services/groups/import_export/import_service_spec.rb +++ b/spec/services/groups/import_export/import_service_spec.rb @@ -110,6 +110,7 @@ RSpec.describe Groups::ImportExport::ImportService do allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) allow(import_logger).to receive(:info) + allow(import_logger).to receive(:warn) allow(FileUtils).to receive(:rm_rf).and_call_original end @@ -220,6 +221,7 @@ RSpec.describe Groups::ImportExport::ImportService do allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger) allow(import_logger).to receive(:error) + allow(import_logger).to receive(:warn) allow(import_logger).to receive(:info) allow(FileUtils).to receive(:rm_rf).and_call_original end diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb index ae04eca3a9f..19b746ade34 100644 --- a/spec/services/groups/transfer_service_spec.rb +++ b/spec/services/groups/transfer_service_spec.rb @@ -3,15 +3,15 @@ require 'spec_helper' RSpec.describe Groups::TransferService do - let(:user) { create(:user) } - let(:new_parent_group) { create(:group, :public) } + let_it_be(:user) { create(:user) } + let_it_be(:new_parent_group) { create(:group, :public) } let!(:group_member) { create(:group_member, :owner, group: group, user: user) } let(:transfer_service) { described_class.new(group, user) } context 'handling packages' do let_it_be(:group) { create(:group, :public) } + let_it_be(:new_group) { create(:group, :public) } let(:project) { create(:project, :public, namespace: group) } - let(:new_group) { create(:group, :public) } before do group.add_owner(user) @@ -35,8 +35,8 @@ RSpec.describe Groups::TransferService do it_behaves_like 'transfer not allowed' context 'with a project within subgroup' do - let(:root_group) { create(:group) } - let(:group) { create(:group, parent: root_group) } + let_it_be(:root_group) { create(:group) } + let_it_be(:group) { create(:group, parent: root_group) } before do root_group.add_owner(user) @@ -79,8 +79,6 @@ RSpec.describe Groups::TransferService do shared_examples 'ensuring allowed transfer for a group' do context "when there's an exception on GitLab shell directories" do - let(:new_parent_group) { create(:group, :public) } - before do allow_next_instance_of(described_class) do |instance| allow(instance).to receive(:update_group_attributes).and_raise(Gitlab::UpdatePathError, 'namespace directory cannot be moved') @@ -101,7 +99,7 @@ RSpec.describe Groups::TransferService do describe '#execute' do context 'when transforming a group into a root group' do - let!(:group) { create(:group, :public, :nested) } + let_it_be_with_reload(:group) { create(:group, :public, :nested) } it_behaves_like 'ensuring allowed transfer for a group' @@ -115,7 +113,7 @@ RSpec.describe Groups::TransferService do end context 'when the user does not have the right policies' do - let!(:group_member) { create(:group_member, :guest, group: group, user: user) } + let_it_be(:group_member) { create(:group_member, :guest, group: group, user: user) } it "returns false" do expect(transfer_service.execute(nil)).to be_falsy @@ -128,7 +126,7 @@ RSpec.describe Groups::TransferService do end context 'when there is a group with the same path' do - let!(:group) { create(:group, :public, :nested, path: 'not-unique') } + let_it_be(:group) { create(:group, :public, :nested, path: 'not-unique') } before do create(:group, path: 'not-unique') @@ -145,9 +143,9 @@ RSpec.describe Groups::TransferService do end context 'when the group is a subgroup and the transfer is valid' do - let!(:subgroup1) { create(:group, :private, parent: group) } - let!(:subgroup2) { create(:group, :internal, parent: group) } - let!(:project1) { create(:project, :repository, :private, namespace: group) } + let_it_be(:subgroup1) { create(:group, :private, parent: group) } + let_it_be(:subgroup2) { create(:group, :internal, parent: group) } + let_it_be(:project1) { create(:project, :repository, :private, namespace: group) } before do transfer_service.execute(nil) @@ -173,12 +171,12 @@ RSpec.describe Groups::TransferService do end context 'when transferring a subgroup into another group' do - let(:group) { create(:group, :public, :nested) } + let_it_be_with_reload(:group) { create(:group, :public, :nested) } it_behaves_like 'ensuring allowed transfer for a group' context 'when the new parent group is the same as the previous parent group' do - let(:group) { create(:group, :public, :nested, parent: new_parent_group) } + let_it_be(:group) { create(:group, :public, :nested, parent: new_parent_group) } it 'returns false' do expect(transfer_service.execute(new_parent_group)).to be_falsy @@ -191,7 +189,7 @@ RSpec.describe Groups::TransferService do end context 'when the user does not have the right policies' do - let!(:group_member) { create(:group_member, :guest, group: group, user: user) } + let_it_be(:group_member) { create(:group_member, :guest, group: group, user: user) } it "returns false" do expect(transfer_service.execute(new_parent_group)).to be_falsy @@ -221,7 +219,7 @@ RSpec.describe Groups::TransferService do end context 'when the parent group has a project with the same path' do - let!(:group) { create(:group, :public, :nested, path: 'foo') } + let_it_be_with_reload(:group) { create(:group, :public, :nested, path: 'foo') } before do create(:group_member, :owner, group: new_parent_group, user: user) @@ -240,8 +238,13 @@ RSpec.describe Groups::TransferService do end context 'when the group is allowed to be transferred' do + let_it_be(:new_parent_group_integration) { create(:slack_service, group: new_parent_group, project: nil, webhook: 'http://new-group.slack.com') } + before do + allow(PropagateIntegrationWorker).to receive(:perform_async) + create(:group_member, :owner, group: new_parent_group, user: user) + transfer_service.execute(new_parent_group) end @@ -267,6 +270,30 @@ RSpec.describe Groups::TransferService do end end + context 'with a group integration' do + let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') } + let(:new_created_integration) { Service.find_by(group: group) } + + context 'with an inherited integration' do + let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com', inherit_from_id: instance_integration.id) } + + it 'replaces inherited integrations', :aggregate_failures do + expect(new_created_integration.webhook).to eq(new_parent_group_integration.webhook) + expect(PropagateIntegrationWorker).to have_received(:perform_async).with(new_created_integration.id) + expect(Service.count).to eq(3) + end + end + + context 'with a custom integration' do + let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com') } + + it 'does not updates the integrations', :aggregate_failures do + expect { transfer_service.execute(new_parent_group) }.not_to change { group_integration.webhook } + expect(PropagateIntegrationWorker).not_to have_received(:perform_async) + end + end + end + it 'updates visibility for the group based on the parent group' do expect(group.visibility_level).to eq(new_parent_group.visibility_level) end @@ -464,7 +491,7 @@ RSpec.describe Groups::TransferService do end context 'updated paths' do - let(:group) { create(:group, :public) } + let_it_be_with_reload(:group) { create(:group, :public) } before do transfer_service.execute(new_parent_group) @@ -500,10 +527,10 @@ RSpec.describe Groups::TransferService do end context 'resets project authorizations' do - let(:old_parent_group) { create(:group) } - let(:group) { create(:group, :private, parent: old_parent_group) } - let(:new_group_member) { create(:user) } - let(:old_group_member) { create(:user) } + let_it_be(:old_parent_group) { create(:group) } + let_it_be_with_reload(:group) { create(:group, :private, parent: old_parent_group) } + let_it_be(:new_group_member) { create(:user) } + let_it_be(:old_group_member) { create(:user) } before do new_parent_group.add_maintainer(new_group_member) diff --git a/spec/services/incident_management/incidents/create_service_spec.rb b/spec/services/incident_management/incidents/create_service_spec.rb index 1330f3ae033..4601bd807d0 100644 --- a/spec/services/incident_management/incidents/create_service_spec.rb +++ b/spec/services/incident_management/incidents/create_service_spec.rb @@ -37,6 +37,8 @@ RSpec.describe IncidentManagement::Incidents::CreateService do end let(:issue) { new_issue } + + include_examples 'has incident label' end context 'with default severity' do diff --git a/spec/services/issues/clone_service_spec.rb b/spec/services/issues/clone_service_spec.rb new file mode 100644 index 00000000000..512a60b1382 --- /dev/null +++ b/spec/services/issues/clone_service_spec.rb @@ -0,0 +1,340 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Issues::CloneService do + include DesignManagementTestHelpers + + let_it_be(:user) { create(:user) } + let_it_be(:author) { create(:user) } + let_it_be(:title) { 'Some issue' } + let_it_be(:description) { "Some issue description with mention to #{user.to_reference}" } + let_it_be(:group) { create(:group, :private) } + let_it_be(:sub_group_1) { create(:group, :private, parent: group) } + let_it_be(:sub_group_2) { create(:group, :private, parent: group) } + let_it_be(:old_project) { create(:project, namespace: sub_group_1) } + let_it_be(:new_project) { create(:project, namespace: sub_group_2) } + + let_it_be(:old_issue, reload: true) do + create(:issue, title: title, description: description, project: old_project, author: author) + end + + let(:with_notes) { false } + + subject(:clone_service) do + described_class.new(old_project, user) + end + + shared_context 'user can clone issue' do + before do + old_project.add_reporter(user) + new_project.add_reporter(user) + end + end + + describe '#execute' do + context 'issue movable' do + include_context 'user can clone issue' + + context 'generic issue' do + let!(:new_issue) { clone_service.execute(old_issue, new_project, with_notes: with_notes) } + + it 'creates a new issue in the selected project' do + expect do + clone_service.execute(old_issue, new_project) + end.to change { new_project.issues.count }.by(1) + end + + it 'copies issue title' do + expect(new_issue.title).to eq title + end + + it 'copies issue description' do + expect(new_issue.description).to eq description + end + + it 'adds system note to old issue at the end' do + expect(old_issue.notes.last.note).to start_with 'cloned to' + end + + it 'adds system note to new issue at the end' do + expect(new_issue.notes.last.note).to start_with 'cloned from' + end + + it 'keeps old issue open' do + expect(old_issue.open?).to be true + end + + it 'persists new issue' do + expect(new_issue.persisted?).to be true + end + + it 'persists all changes' do + expect(old_issue.changed?).to be false + expect(new_issue.changed?).to be false + end + + it 'sets the current user as author' do + expect(new_issue.author).to eq user + end + + it 'creates a new internal id for issue' do + expect(new_issue.iid).to be_present + end + + it 'preserves create time' do + expect(old_issue.created_at.strftime('%D')).to eq new_issue.created_at.strftime('%D') + end + + it 'does not copy system notes' do + expect(new_issue.notes.count).to eq(1) + end + + it 'does not set moved_issue' do + expect(old_issue.moved?).to eq(false) + end + + context 'when copying comments' do + let(:with_notes) { true } + + it 'does not create extra system notes' do + new_issue = clone_service.execute(old_issue, new_project, with_notes: with_notes) + + expect(new_issue.notes.count).to eq(old_issue.notes.count) + end + end + end + + context 'issue with award emoji' do + let!(:award_emoji) { create(:award_emoji, awardable: old_issue) } + + it 'copies the award emoji' do + old_issue.reload + new_issue = clone_service.execute(old_issue, new_project) + + expect(old_issue.award_emoji.first.name).to eq new_issue.reload.award_emoji.first.name + end + end + + context 'issue with milestone' do + let(:milestone) { create(:milestone, group: sub_group_1) } + let(:new_project) { create(:project, namespace: sub_group_1) } + + let(:old_issue) do + create(:issue, title: title, description: description, project: old_project, author: author, milestone: milestone) + end + + before do + create(:resource_milestone_event, issue: old_issue, milestone: milestone, action: :add) + end + + it 'does not create extra milestone events' do + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.resource_milestone_events.count).to eq(old_issue.resource_milestone_events.count) + end + end + + context 'issue with due date' do + let(:date) { Date.parse('2020-01-10') } + + let(:old_issue) do + create(:issue, title: title, description: description, project: old_project, author: author, due_date: date) + end + + before do + SystemNoteService.change_due_date(old_issue, old_project, author, old_issue.due_date) + end + + it 'keeps the same due date' do + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.due_date).to eq(date) + end + end + + context 'issue with assignee' do + let_it_be(:assignee) { create(:user) } + + before do + old_issue.assignees = [assignee] + end + + it 'preserves assignee with access to the new issue' do + new_project.add_reporter(assignee) + + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.assignees).to eq([assignee]) + end + + it 'ignores assignee without access to the new issue' do + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.assignees).to be_empty + end + end + + context 'issue is confidential' do + before do + old_issue.update_columns(confidential: true) + end + + it 'preserves the confidential flag' do + new_issue = clone_service.execute(old_issue, new_project) + + expect(new_issue.confidential).to be true + end + end + + context 'moving to same project' do + it 'also works' do + new_issue = clone_service.execute(old_issue, old_project) + + expect(new_issue.project).to eq(old_project) + expect(new_issue.iid).not_to eq(old_issue.iid) + end + end + + context 'project issue hooks' do + let!(:hook) { create(:project_hook, project: old_project, issues_events: true) } + + it 'executes project issue hooks' do + allow_next_instance_of(WebHookService) do |instance| + allow(instance).to receive(:execute) + end + + # Ideally, we'd test that `WebHookWorker.jobs.size` increased by 1, + # but since the entire spec run takes place in a transaction, we never + # actually get to the `after_commit` hook that queues these jobs. + expect { clone_service.execute(old_issue, new_project) } + .not_to raise_error # Sidekiq::Worker::EnqueueFromTransactionError + end + end + + # These tests verify that notes are copied. More thorough tests are in + # the unit test for Notes::CopyService. + context 'issue with notes' do + let_it_be(:notes) do + [ + create(:note, noteable: old_issue, project: old_project, created_at: 2.weeks.ago, updated_at: 1.week.ago), + create(:note, noteable: old_issue, project: old_project) + ] + end + + let(:new_issue) { clone_service.execute(old_issue, new_project, with_notes: with_notes) } + + let(:copied_notes) { new_issue.notes.limit(notes.size) } # Remove the system note added by the copy itself + + it 'does not copy notes' do + # only the system note + expect(copied_notes.order('id ASC').pluck(:note).size).to eq(1) + end + + context 'when copying comments' do + let(:with_notes) { true } + + it 'copies existing notes in order' do + expect(copied_notes.order('id ASC').pluck(:note)).to eq(notes.map(&:note)) + end + end + end + + context 'issue with a design', :clean_gitlab_redis_shared_state do + let_it_be(:new_project) { create(:project) } + let!(:design) { create(:design, :with_lfs_file, issue: old_issue) } + let!(:note) { create(:diff_note_on_design, noteable: design, issue: old_issue, project: old_issue.project) } + let(:subject) { clone_service.execute(old_issue, new_project) } + + before do + enable_design_management + end + + it 'calls CopyDesignCollection::QueueService' do + expect(DesignManagement::CopyDesignCollection::QueueService).to receive(:new) + .with(user, old_issue, kind_of(Issue)) + .and_call_original + + subject + end + + it 'logs if QueueService returns an error', :aggregate_failures do + error_message = 'error' + + expect_next_instance_of(DesignManagement::CopyDesignCollection::QueueService) do |service| + expect(service).to receive(:execute).and_return( + ServiceResponse.error(message: error_message) + ) + end + expect(Gitlab::AppLogger).to receive(:error).with(error_message) + + subject + end + + # Perform a small integration test to ensure the services and worker + # can correctly create designs. + it 'copies the design and its notes', :sidekiq_inline, :aggregate_failures do + new_issue = subject + + expect(new_issue.designs.size).to eq(1) + expect(new_issue.designs.first.notes.size).to eq(1) + end + end + end + + describe 'clone permissions' do + let(:clone) { clone_service.execute(old_issue, new_project) } + + context 'target project is pending deletion' do + include_context 'user can clone issue' + + before do + new_project.update_columns(pending_delete: true) + end + + after do + new_project.update_columns(pending_delete: false) + end + + it { expect { clone }.to raise_error(Issues::CloneService::CloneError, /pending deletion/) } + end + + context 'user is reporter in both projects' do + include_context 'user can clone issue' + it { expect { clone }.not_to raise_error } + end + + context 'user is reporter only in new project' do + before do + new_project.add_reporter(user) + end + + it { expect { clone }.to raise_error(StandardError, /permissions/) } + end + + context 'user is reporter only in old project' do + before do + old_project.add_reporter(user) + end + + it { expect { clone }.to raise_error(StandardError, /permissions/) } + end + + context 'user is reporter in one project and guest in another' do + before do + new_project.add_guest(user) + old_project.add_reporter(user) + end + + it { expect { clone }.to raise_error(StandardError, /permissions/) } + end + + context 'issue is not persisted' do + include_context 'user can clone issue' + let(:old_issue) { build(:issue, project: old_project, author: author) } + + it { expect { clone }.to raise_error(StandardError, /permissions/) } + end + end + end +end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index eeac7fb9923..cc6a49fc4cf 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -63,6 +63,7 @@ RSpec.describe Issues::CreateService do subject { issue } it_behaves_like 'incident issue' + it_behaves_like 'has incident label' it_behaves_like 'an incident management tracked event', :incident_management_incident_created it 'does create an incident label' do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index cfda27795c7..06a6a52bc41 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -99,31 +99,18 @@ RSpec.describe Issues::UpdateService, :mailer do context 'when issue type is incident' do let(:issue) { create(:incident, project: project) } - it 'changes updates the severity' do + before do update_issue(opts) - - expect(issue.severity).to eq('low') end - it_behaves_like 'incident issue' do - before do - update_issue(opts) - end - end - - context 'with existing incident label' do - let_it_be(:incident_label) { create(:label, :incident, project: project) } + it_behaves_like 'incident issue' - before do - opts.delete(:label_ids) # don't override but retain existing labels - issue.labels << incident_label - end + it 'changes updates the severity' do + expect(issue.severity).to eq('low') + end - it_behaves_like 'incident issue' do - before do - update_issue(opts) - end - end + it 'does not apply incident labels' do + expect(issue.labels).to match_array [label] end end @@ -155,7 +142,6 @@ RSpec.describe Issues::UpdateService, :mailer do context 'issue in incident type' do let(:current_user) { user } - let(:incident_label_attributes) { attributes_for(:label, :incident) } before do opts.merge!(issue_type: 'incident', confidential: true) @@ -170,21 +156,6 @@ RSpec.describe Issues::UpdateService, :mailer do subject end end - - it 'does create an incident label' do - expect { subject } - .to change { Label.where(incident_label_attributes).count }.by(1) - end - - context 'when invalid' do - before do - opts.merge!(title: '') - end - - it 'does not create an incident label prematurely' do - expect { subject }.not_to change(Label, :count) - end - end end it 'updates open issue counter for assignees when issue is reassigned' do @@ -968,6 +939,46 @@ RSpec.describe Issues::UpdateService, :mailer do end end + context 'clone an issue' do + context 'valid project' do + let(:target_project) { create(:project) } + + before do + target_project.add_maintainer(user) + end + + it 'calls the move service with the proper issue and project' do + clone_stub = instance_double(Issues::CloneService) + allow(Issues::CloneService).to receive(:new).and_return(clone_stub) + allow(clone_stub).to receive(:execute).with(issue, target_project, with_notes: nil).and_return(issue) + + expect(clone_stub).to receive(:execute).with(issue, target_project, with_notes: nil) + + update_issue(target_clone_project: target_project) + end + end + end + + context 'clone an issue with notes' do + context 'valid project' do + let(:target_project) { create(:project) } + + before do + target_project.add_maintainer(user) + end + + it 'calls the move service with the proper issue and project' do + clone_stub = instance_double(Issues::CloneService) + allow(Issues::CloneService).to receive(:new).and_return(clone_stub) + allow(clone_stub).to receive(:execute).with(issue, target_project, with_notes: true).and_return(issue) + + expect(clone_stub).to receive(:execute).with(issue, target_project, with_notes: true) + + update_issue(target_clone_project: target_project, clone_with_notes: true) + end + end + end + context 'when moving an issue ' do it 'raises an error for invalid move ids within a project' do opts = { move_between_ids: [9000, non_existing_record_id] } diff --git a/spec/services/jira/requests/projects/list_service_spec.rb b/spec/services/jira/requests/projects/list_service_spec.rb index f7bcfa997df..0fff51b1226 100644 --- a/spec/services/jira/requests/projects/list_service_spec.rb +++ b/spec/services/jira/requests/projects/list_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Jira::Requests::Projects::ListService do + include AfterNextHelpers + let(:jira_service) { create(:jira_service) } let(:params) { {} } @@ -33,15 +35,17 @@ RSpec.describe Jira::Requests::Projects::ListService do context 'with jira_service' do context 'when validations and params are ok' do - let(:client) { double(options: { site: 'https://jira.example.com' }) } + let(:response_headers) { { 'content-type' => 'application/json' } } + let(:response_body) { [].to_json } + let(:expected_url_pattern) { /.*jira.example.com\/rest\/api\/2\/project/ } before do - expect(service).to receive(:client).at_least(:once).and_return(client) + stub_request(:get, expected_url_pattern).to_return(status: 200, body: response_body, headers: response_headers) end context 'when the request to Jira returns an error' do before do - expect(client).to receive(:get).and_raise(Timeout::Error) + expect_next(JIRA::Client).to receive(:get).and_raise(Timeout::Error) end it 'returns an error response' do @@ -54,10 +58,17 @@ RSpec.describe Jira::Requests::Projects::ListService do end end - context 'when the request does not return any values' do - before do - expect(client).to receive(:get).and_return([]) + context 'when jira runs on a subpath' do + let(:jira_service) { create(:jira_service, url: 'http://jira.example.com/jira') } + let(:expected_url_pattern) { /.*jira.example.com\/jira\/rest\/api\/2\/project/ } + + it 'takes the subpath into account' do + expect(subject.success?).to be_truthy end + end + + context 'when the request does not return any values' do + let(:response_body) { [].to_json } it 'returns a paylod with no projects returned' do payload = subject.payload @@ -69,9 +80,7 @@ RSpec.describe Jira::Requests::Projects::ListService do end context 'when the request returns values' do - before do - expect(client).to receive(:get).and_return([{ 'key' => 'pr1', 'name' => 'First Project' }, { 'key' => 'pr2', 'name' => 'Second Project' }]) - end + let(:response_body) { [{ 'key' => 'pr1', 'name' => 'First Project' }, { 'key' => 'pr2', 'name' => 'Second Project' }].to_json } it 'returns a paylod with Jira projects' do payload = subject.payload diff --git a/spec/services/jira_connect/sync_service_spec.rb b/spec/services/jira_connect/sync_service_spec.rb index 83088bb2e79..4b434348146 100644 --- a/spec/services/jira_connect/sync_service_spec.rb +++ b/spec/services/jira_connect/sync_service_spec.rb @@ -3,30 +3,23 @@ require 'spec_helper' RSpec.describe JiraConnect::SyncService do + include AfterNextHelpers + describe '#execute' do let_it_be(:project) { create(:project, :repository) } - let(:branches) { [project.repository.find_branch('master')] } - let(:commits) { project.commits_by(oids: %w[b83d6e3 5a62481]) } - let(:merge_requests) { [create(:merge_request, source_project: project, target_project: project)] } + let(:client) { Atlassian::JiraConnect::Client } + let(:info) { { a: 'Some', b: 'Info' } } subject do - described_class.new(project).execute(commits: commits, branches: branches, merge_requests: merge_requests) + described_class.new(project).execute(**info) end before do create(:jira_connect_subscription, namespace: project.namespace) end - def expect_jira_client_call(return_value = { 'status': 'success' }) - expect_next_instance_of(Atlassian::JiraConnect::Client) do |instance| - expect(instance).to receive(:store_dev_info).with( - project: project, - commits: commits, - branches: [instance_of(Gitlab::Git::Branch)], - merge_requests: merge_requests, - update_sequence_id: anything - ).and_return(return_value) - end + def store_info(return_values = [{ 'status': 'success' }]) + receive(:send_info).with(project: project, **info).and_return(return_values) end def expect_log(type, message) @@ -41,20 +34,22 @@ RSpec.describe JiraConnect::SyncService do end it 'calls Atlassian::JiraConnect::Client#store_dev_info and logs the response' do - expect_jira_client_call + expect_next(client).to store_info expect_log(:info, { 'status': 'success' }) subject end - context 'when request returns an error' do + context 'when a request returns an error' do it 'logs the response as an error' do - expect_jira_client_call({ - 'errorMessages' => ['some error message'] - }) + expect_next(client).to store_info([ + { 'errorMessages' => ['some error message'] }, + { 'rejectedBuilds' => ['x'] } + ]) expect_log(:error, { 'errorMessages' => ['some error message'] }) + expect_log(:error, { 'rejectedBuilds' => ['x'] }) subject end diff --git a/spec/services/members/approve_access_request_service_spec.rb b/spec/services/members/approve_access_request_service_spec.rb index e6a94fdaf84..f7fbac612ee 100644 --- a/spec/services/members/approve_access_request_service_spec.rb +++ b/spec/services/members/approve_access_request_service_spec.rb @@ -12,23 +12,23 @@ RSpec.describe Members::ApproveAccessRequestService do shared_examples 'a service raising ActiveRecord::RecordNotFound' do it 'raises ActiveRecord::RecordNotFound' do - expect { described_class.new(current_user).execute(access_requester, opts) }.to raise_error(ActiveRecord::RecordNotFound) + expect { described_class.new(current_user).execute(access_requester, **opts) }.to raise_error(ActiveRecord::RecordNotFound) end end shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do it 'raises Gitlab::Access::AccessDeniedError' do - expect { described_class.new(current_user).execute(access_requester, opts) }.to raise_error(Gitlab::Access::AccessDeniedError) + expect { described_class.new(current_user).execute(access_requester, **opts) }.to raise_error(Gitlab::Access::AccessDeniedError) end end shared_examples 'a service approving an access request' do it 'succeeds' do - expect { described_class.new(current_user).execute(access_requester, opts) }.to change { source.requesters.count }.by(-1) + expect { described_class.new(current_user).execute(access_requester, **opts) }.to change { source.requesters.count }.by(-1) end it 'returns a <Source>Member' do - member = described_class.new(current_user).execute(access_requester, opts) + member = described_class.new(current_user).execute(access_requester, **opts) expect(member).to be_a "#{source.class}Member".constantize expect(member.requested_at).to be_nil @@ -36,7 +36,7 @@ RSpec.describe Members::ApproveAccessRequestService do context 'with a custom access level' do it 'returns a ProjectMember with the custom access level' do - member = described_class.new(current_user, access_level: Gitlab::Access::MAINTAINER).execute(access_requester, opts) + member = described_class.new(current_user, access_level: Gitlab::Access::MAINTAINER).execute(access_requester, **opts) expect(member.access_level).to eq(Gitlab::Access::MAINTAINER) end diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb index 00b5ff59e48..e8a4a798b20 100644 --- a/spec/services/members/create_service_spec.rb +++ b/spec/services/members/create_service_spec.rb @@ -3,59 +3,68 @@ require 'spec_helper' RSpec.describe Members::CreateService do - let(:project) { create(:project) } - let(:user) { create(:user) } - let(:project_user) { create(:user) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:project_user) { create(:user) } + let_it_be(:user_ids) { project_user.id.to_s } + let_it_be(:access_level) { Gitlab::Access::GUEST } + let(:params) { { user_ids: user_ids, access_level: access_level } } + + subject(:execute_service) { described_class.new(user, params).execute(project) } before do project.add_maintainer(user) + allow(Namespaces::OnboardingUserAddedWorker).to receive(:idempotent?).and_return(false) end - it 'adds user to members' do - params = { user_ids: project_user.id.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 + context 'when passing valid parameters' do + it 'adds a user to members' do + expect(execute_service[:status]).to eq(:success) + expect(project.users).to include project_user + expect(Namespaces::OnboardingUserAddedWorker.jobs.last['args'][0]).to eq(project.id) + end end - it 'adds no user to members' do - params = { user_ids: '', access_level: Gitlab::Access::GUEST } - result = described_class.new(user, params).execute(project) + context 'when passing no user ids' do + let(:user_ids) { '' } - expect(result[:status]).to eq(:error) - expect(result[:message]).to be_present - expect(project.users).not_to include project_user + it 'does not add a member' do + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]).to be_present + expect(project.users).not_to include project_user + expect(Namespaces::OnboardingUserAddedWorker.jobs.size).to eq(0) + end end - it 'limits the number of users to 100' do - user_ids = 1.upto(101).to_a.join(',') - params = { user_ids: user_ids, access_level: Gitlab::Access::GUEST } + context 'when passing many user ids' do + let(:user_ids) { 1.upto(101).to_a.join(',') } - result = described_class.new(user, params).execute(project) - - expect(result[:status]).to eq(:error) - expect(result[:message]).to be_present - expect(project.users).not_to include project_user + it 'limits the number of users to 100' do + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]).to be_present + expect(project.users).not_to include project_user + expect(Namespaces::OnboardingUserAddedWorker.jobs.size).to eq(0) + end end - it 'does not add an invalid member' do - params = { user_ids: project_user.id.to_s, access_level: -1 } - result = described_class.new(user, params).execute(project) + context 'when passing an invalid access level' do + let(:access_level) { -1 } - expect(result[:status]).to eq(:error) - expect(result[:message]).to include("#{project_user.username}: Access level is not included in the list") - expect(project.users).not_to include project_user + it 'does not add a member' do + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]).to include("#{project_user.username}: Access level is not included in the list") + expect(project.users).not_to include project_user + expect(Namespaces::OnboardingUserAddedWorker.jobs.size).to eq(0) + end end - it 'does not add a member with an existing invite' do - invited_member = create(:project_member, :invited, project: project) - - params = { user_ids: invited_member.invite_email, - access_level: Gitlab::Access::GUEST } - result = described_class.new(user, params).execute(project) + context 'when passing an existing invite user id' do + let(:user_ids) { create(:project_member, :invited, project: project).invite_email } - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invite email has already been taken') + it 'does not add a member' do + expect(execute_service[:status]).to eq(:error) + expect(execute_service[:message]).to eq('Invite email has already been taken') + expect(Namespaces::OnboardingUserAddedWorker.jobs.size).to eq(0) + end end end diff --git a/spec/services/members/invitation_reminder_email_service_spec.rb b/spec/services/members/invitation_reminder_email_service_spec.rb index 88280869476..768a8719d54 100644 --- a/spec/services/members/invitation_reminder_email_service_spec.rb +++ b/spec/services/members/invitation_reminder_email_service_spec.rb @@ -9,67 +9,49 @@ RSpec.describe Members::InvitationReminderEmailService do let_it_be(:frozen_time) { Date.today.beginning_of_day } let_it_be(:invitation) { build(:group_member, :invited, created_at: frozen_time) } - context 'when the experiment is disabled' do - before do - allow(Gitlab::Experimentation).to receive(:enabled_for_attribute?).and_return(false) - invitation.expires_at = frozen_time + 2.days - end - - it 'does not send an invitation' do - travel_to(frozen_time + 1.day) do - expect(invitation).not_to receive(:send_invitation_reminder) - - subject - end - end + before do + invitation.expires_at = frozen_time + expires_at_days.days if expires_at_days end - context 'when the experiment is enabled' do - before do - allow(Gitlab::Experimentation).to receive(:enabled_for_attribute?).and_return(true) - invitation.expires_at = frozen_time + expires_at_days.days if expires_at_days - end - - using RSpec::Parameterized::TableSyntax - - where(:expires_at_days, :send_reminder_at_days) do - 0 | [] - 1 | [] - 2 | [1] - 3 | [1, 2] - 4 | [1, 2, 3] - 5 | [1, 2, 4] - 6 | [1, 3, 5] - 7 | [1, 3, 5] - 8 | [2, 3, 6] - 9 | [2, 4, 7] - 10 | [2, 4, 8] - 11 | [2, 4, 8] - 12 | [2, 5, 9] - 13 | [2, 5, 10] - 14 | [2, 5, 10] - 15 | [2, 5, 10] - nil | [2, 5, 10] - end - - with_them do - # Create an invitation today with an expiration date from 0 to 10 days in the future or without an expiration date - # We chose 10 days here, because we fetch invitations that were created at most 10 days ago. - (0..10).each do |day| - it 'sends an invitation reminder only on the expected days' do - next if day > (expires_at_days || 10) # We don't need to test after the invitation has already expired - - # We are traveling in a loop from today to 10 days from now - travel_to(frozen_time + day.days) do - # Given an expiration date and the number of days after the creation of the invitation based on the current day in the loop, a reminder may be sent - if (reminder_index = send_reminder_at_days.index(day)) - expect(invitation).to receive(:send_invitation_reminder).with(reminder_index) - else - expect(invitation).not_to receive(:send_invitation_reminder) - end + using RSpec::Parameterized::TableSyntax + + where(:expires_at_days, :send_reminder_at_days) do + 0 | [] + 1 | [] + 2 | [1] + 3 | [1, 2] + 4 | [1, 2, 3] + 5 | [1, 2, 4] + 6 | [1, 3, 5] + 7 | [1, 3, 5] + 8 | [2, 3, 6] + 9 | [2, 4, 7] + 10 | [2, 4, 8] + 11 | [2, 4, 8] + 12 | [2, 5, 9] + 13 | [2, 5, 10] + 14 | [2, 5, 10] + 15 | [2, 5, 10] + nil | [2, 5, 10] + end - subject + with_them do + # Create an invitation today with an expiration date from 0 to 10 days in the future or without an expiration date + # We chose 10 days here, because we fetch invitations that were created at most 10 days ago. + (0..10).each do |day| + it 'sends an invitation reminder only on the expected days' do + next if day > (expires_at_days || 10) # We don't need to test after the invitation has already expired + + # We are traveling in a loop from today to 10 days from now + travel_to(frozen_time + day.days) do + # Given an expiration date and the number of days after the creation of the invitation based on the current day in the loop, a reminder may be sent + if (reminder_index = send_reminder_at_days.index(day)) + expect(invitation).to receive(:send_invitation_reminder).with(reminder_index) + else + expect(invitation).not_to receive(:send_invitation_reminder) end + + subject end end end diff --git a/spec/services/merge_requests/after_create_service_spec.rb b/spec/services/merge_requests/after_create_service_spec.rb index 840b7bc0a1c..69bab3b1ea4 100644 --- a/spec/services/merge_requests/after_create_service_spec.rb +++ b/spec/services/merge_requests/after_create_service_spec.rb @@ -18,32 +18,34 @@ RSpec.describe MergeRequests::AfterCreateService do allow(after_create_service).to receive(:notification_service).and_return(notification_service) end + subject(:execute_service) { after_create_service.execute(merge_request) } + it 'creates a merge request open event' do expect(event_service) .to receive(:open_mr).with(merge_request, merge_request.author) - after_create_service.execute(merge_request) + execute_service end it 'creates a new merge request notification' do expect(notification_service) .to receive(:new_merge_request).with(merge_request, merge_request.author) - after_create_service.execute(merge_request) + execute_service end it 'writes diffs to the cache' do expect(merge_request) .to receive_message_chain(:diffs, :write_cache) - after_create_service.execute(merge_request) + execute_service end it 'creates cross references' do expect(merge_request) .to receive(:create_cross_references!).with(merge_request.author) - after_create_service.execute(merge_request) + execute_service end it 'creates a pipeline and updates the HEAD pipeline' do @@ -51,7 +53,14 @@ RSpec.describe MergeRequests::AfterCreateService do .to receive(:create_pipeline_for).with(merge_request, merge_request.author) expect(merge_request).to receive(:update_head_pipeline) - after_create_service.execute(merge_request) + execute_service + end + + it 'records a namespace onboarding progress action' do + expect(NamespaceOnboardingAction).to receive(:create_action) + .with(merge_request.target_project.namespace, :merge_request_created).and_call_original + + expect { execute_service }.to change(NamespaceOnboardingAction, :count).by(1) end end end diff --git a/spec/services/merge_requests/base_service_spec.rb b/spec/services/merge_requests/base_service_spec.rb index bb7b70f1ba2..83431105545 100644 --- a/spec/services/merge_requests/base_service_spec.rb +++ b/spec/services/merge_requests/base_service_spec.rb @@ -20,7 +20,8 @@ RSpec.describe MergeRequests::BaseService do describe '#execute_hooks' do shared_examples 'enqueues Jira sync worker' do - it do + specify :aggregate_failures do + expect(JiraConnect::SyncMergeRequestWorker).to receive(:perform_async).with(kind_of(Numeric), kind_of(Numeric)).and_call_original Sidekiq::Testing.fake! do expect { subject.execute }.to change(JiraConnect::SyncMergeRequestWorker.jobs, :size).by(1) end diff --git a/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb b/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb index 728b343b801..b326fc1726d 100644 --- a/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb +++ b/spec/services/metrics/dashboard/clone_dashboard_service_spec.rb @@ -146,7 +146,7 @@ RSpec.describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memor it 'extends dashboard template path to absolute url' do allow(::Files::CreateService).to receive(:new).and_return(double(execute: { status: :success })) - expect(File).to receive(:read).with(Rails.root.join('config/prometheus/common_metrics.yml')).and_return('') + expect_file_read(Rails.root.join('config/prometheus/common_metrics.yml'), content: '') service_call end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f9a89c6281e..9431c023850 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -353,7 +353,7 @@ RSpec.describe NotificationService, :mailer do context 'a service-desk issue' do before do - issue.update!(service_desk_reply_to: 'service.desk@example.com') + issue.update!(external_author: 'service.desk@example.com') project.update!(service_desk_enabled: true) end @@ -1549,6 +1549,37 @@ RSpec.describe NotificationService, :mailer do end end + describe '#issue_cloned' do + let(:new_issue) { create(:issue) } + + it 'sends email to issue notification recipients' do + notification.issue_cloned(issue, new_issue, @u_disabled) + + should_email(issue.assignees.first) + should_email(issue.author) + should_email(@u_watcher) + should_email(@u_guest_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) } + end + + it_behaves_like 'project emails are disabled' do + let(:notification_target) { issue } + let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) } + end + end + describe '#issue_due' do before do issue.update!(due_date: Date.today) @@ -2326,6 +2357,20 @@ RSpec.describe NotificationService, :mailer do end end + describe '#user_admin_rejection', :deliver_mails_inline do + let_it_be(:user) { create(:user, :blocked_pending_approval) } + + before do + reset_delivered_emails! + end + + it 'sends the user a rejection email' do + notification.user_admin_rejection(user.name, user.email) + + should_only_email(user) + end + end + describe 'GroupMember', :deliver_mails_inline do let(:added_user) { create(:user) } diff --git a/spec/services/onboarding_progress_service_spec.rb b/spec/services/onboarding_progress_service_spec.rb new file mode 100644 index 00000000000..59b6083d38a --- /dev/null +++ b/spec/services/onboarding_progress_service_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe OnboardingProgressService do + describe '#execute' do + let(:namespace) { create(:namespace, parent: root_namespace) } + + subject(:execute_service) { described_class.new(namespace).execute(action: :subscription_created) } + + context 'when the namespace is a root' do + let(:root_namespace) { nil } + + it 'records a namespace onboarding progress action for the given namespace' do + expect(NamespaceOnboardingAction).to receive(:create_action) + .with(namespace, :subscription_created).and_call_original + + expect { execute_service }.to change(NamespaceOnboardingAction, :count).by(1) + end + end + + context 'when the namespace is not the root' do + let_it_be(:root_namespace) { build(:namespace) } + + it 'records a namespace onboarding progress action for the root namespace' do + expect(NamespaceOnboardingAction).to receive(:create_action) + .with(root_namespace, :subscription_created).and_call_original + + expect { execute_service }.to change(NamespaceOnboardingAction, :count).by(1) + end + end + end +end diff --git a/spec/services/packages/composer/create_package_service_spec.rb b/spec/services/packages/composer/create_package_service_spec.rb index a1fe9a1b918..d10356cfda7 100644 --- a/spec/services/packages/composer/create_package_service_spec.rb +++ b/spec/services/packages/composer/create_package_service_spec.rb @@ -41,6 +41,8 @@ RSpec.describe Packages::Composer::CreatePackageService do it_behaves_like 'assigns the package creator' do let(:package) { created_package } end + + it_behaves_like 'assigns build to package' end context 'with a tag' do @@ -62,6 +64,8 @@ RSpec.describe Packages::Composer::CreatePackageService do it_behaves_like 'assigns the package creator' do let(:package) { created_package } end + + it_behaves_like 'assigns build to package' end end 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 bd6a91c883a..e655b8d1f9e 100644 --- a/spec/services/packages/conan/create_package_file_service_spec.rb +++ b/spec/services/packages/conan/create_package_file_service_spec.rb @@ -5,11 +5,12 @@ RSpec.describe Packages::Conan::CreatePackageFileService do include WorkhorseHelpers let_it_be(:package) { create(:conan_package) } + let_it_be(:user) { create(:user) } describe '#execute' do let(:file_name) { 'foo.tgz' } - subject { described_class.new(package, file, params) } + subject { described_class.new(package, file, params).execute } shared_examples 'a valid package_file' do let(:params) do @@ -27,7 +28,7 @@ RSpec.describe Packages::Conan::CreatePackageFileService do end it 'creates a new package file' do - package_file = subject.execute + package_file = subject expect(package_file).to be_valid expect(package_file.file_name).to eq(file_name) @@ -40,6 +41,8 @@ RSpec.describe Packages::Conan::CreatePackageFileService do expect(package_file.conan_file_metadatum.conan_file_type).to eq('package_file') expect(package_file.file.read).to eq('content') end + + it_behaves_like 'assigns build to package file' end shared_examples 'a valid recipe_file' do @@ -56,7 +59,7 @@ RSpec.describe Packages::Conan::CreatePackageFileService do end it 'creates a new recipe file' do - package_file = subject.execute + package_file = subject expect(package_file).to be_valid expect(package_file.file_name).to eq(file_name) @@ -69,6 +72,8 @@ RSpec.describe Packages::Conan::CreatePackageFileService do expect(package_file.conan_file_metadatum.conan_file_type).to eq('recipe_file') expect(package_file.file.read).to eq('content') end + + it_behaves_like 'assigns build to package file' end context 'with temp file' do @@ -123,7 +128,7 @@ RSpec.describe Packages::Conan::CreatePackageFileService do end it 'raises an error' do - expect { subject.execute }.to raise_error(ActiveRecord::RecordInvalid) + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/packages/conan/create_package_service_spec.rb b/spec/services/packages/conan/create_package_service_spec.rb index b217e570aba..ca783475503 100644 --- a/spec/services/packages/conan/create_package_service_spec.rb +++ b/spec/services/packages/conan/create_package_service_spec.rb @@ -30,6 +30,7 @@ RSpec.describe Packages::Conan::CreatePackageService do end it_behaves_like 'assigns the package creator' + it_behaves_like 'assigns build to package' end context 'invalid params' do diff --git a/spec/services/packages/create_event_service_spec.rb b/spec/services/packages/create_event_service_spec.rb index 4db7687bb24..f581d704087 100644 --- a/spec/services/packages/create_event_service_spec.rb +++ b/spec/services/packages/create_event_service_spec.rb @@ -70,12 +70,34 @@ RSpec.describe Packages::CreateEventService do end it 'tracks the event' do + expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).not_to receive(:count) 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 + shared_examples 'redis package guest event creation' do |originator_type, expected_scope| + context 'with feature flag disabled' do + before do + stub_feature_flags(collect_package_events_redis: false) + end + + it 'does not track the event' do + expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).not_to receive(:count) + + subject + end + end + + it 'tracks the event' do + expect(::Gitlab::UsageDataCounters::HLLRedisCounter).not_to receive(:track_event) + expect(::Gitlab::UsageDataCounters::GuestPackageEventCounter).to receive(:count).with(Packages::Event.allowed_event_name(expected_scope, event_name, originator_type)) + + subject + end + end + context 'with a user' do let(:user) { create(:user) } @@ -94,6 +116,7 @@ RSpec.describe Packages::CreateEventService do let(:user) { nil } it_behaves_like 'db package event creation', 'guest', 'container' + it_behaves_like 'redis package guest event creation', 'guest', 'container' end context 'with a package as scope' do @@ -103,6 +126,7 @@ RSpec.describe Packages::CreateEventService do let(:user) { nil } it_behaves_like 'db package event creation', 'guest', 'npm' + it_behaves_like 'redis package guest event creation', 'guest', 'npm' end context 'with user' do diff --git a/spec/services/packages/create_package_file_service_spec.rb b/spec/services/packages/create_package_file_service_spec.rb index 12fd1039d30..e4b4b15ebf9 100644 --- a/spec/services/packages/create_package_file_service_spec.rb +++ b/spec/services/packages/create_package_file_service_spec.rb @@ -4,10 +4,11 @@ require 'spec_helper' RSpec.describe Packages::CreatePackageFileService do let_it_be(:package) { create(:maven_package) } let_it_be(:user) { create(:user) } - - subject { described_class.new(package, params) } + let(:service) { described_class.new(package, params) } describe '#execute' do + subject { service.execute } + context 'with valid params' do let(:params) do { @@ -17,11 +18,13 @@ RSpec.describe Packages::CreatePackageFileService do end it 'creates a new package file' do - package_file = subject.execute + package_file = subject expect(package_file).to be_valid expect(package_file.file_name).to eq('foo.jar') end + + it_behaves_like 'assigns build to package file' end context 'file is missing' do @@ -32,17 +35,7 @@ RSpec.describe Packages::CreatePackageFileService do end it 'raises an error' do - 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 } } - - it 'creates a build_info' do - expect { subject.execute }.to change { Packages::PackageFileBuildInfo.count }.by(1) + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) 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 907483e3d7f..816e728c342 100644 --- a/spec/services/packages/generic/create_package_file_service_spec.rb +++ b/spec/services/packages/generic/create_package_file_service_spec.rb @@ -23,6 +23,8 @@ RSpec.describe Packages::Generic::CreatePackageFileService do } end + subject { described_class.new(project, user, params).execute } + before do FileUtils.touch(temp_file) end @@ -41,9 +43,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do expect(::Packages::Generic::FindOrCreatePackageService).to receive(:new).with(project, user, package_params).and_return(package_service) expect(package_service).to receive(:execute).and_return(package) - service = described_class.new(project, user, params) - - expect { service.execute }.to change { package.package_files.count }.by(1) + expect { subject }.to change { package.package_files.count }.by(1) .and change { Packages::PackageFileBuildInfo.count }.by(1) package_file = package.package_files.last @@ -54,5 +54,7 @@ RSpec.describe Packages::Generic::CreatePackageFileService do expect(package_file.file_sha256).to eq(sha256) end end + + it_behaves_like 'assigns build to package file' end end diff --git a/spec/services/packages/nuget/create_package_service_spec.rb b/spec/services/packages/nuget/create_package_service_spec.rb index e51bc03f311..5289ad40d61 100644 --- a/spec/services/packages/nuget/create_package_service_spec.rb +++ b/spec/services/packages/nuget/create_package_service_spec.rb @@ -31,5 +31,6 @@ RSpec.describe Packages::Nuget::CreatePackageService do end it_behaves_like 'assigns the package creator' + it_behaves_like 'assigns build to package' end end diff --git a/spec/services/packages/pypi/create_package_service_spec.rb b/spec/services/packages/pypi/create_package_service_spec.rb index c985c1e54ea..28a727c4a09 100644 --- a/spec/services/packages/pypi/create_package_service_spec.rb +++ b/spec/services/packages/pypi/create_package_service_spec.rb @@ -51,6 +51,8 @@ RSpec.describe Packages::Pypi::CreatePackageService do let(:package) { created_package } end + it_behaves_like 'assigns build to package' + context 'with an existing package' do before do described_class.new(project, user, params).execute diff --git a/spec/services/pages/legacy_storage_lease_spec.rb b/spec/services/pages/legacy_storage_lease_spec.rb new file mode 100644 index 00000000000..c022da6f47f --- /dev/null +++ b/spec/services/pages/legacy_storage_lease_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Pages::LegacyStorageLease do + let(:project) { create(:project) } + + let(:implementation) do + Class.new do + include ::Pages::LegacyStorageLease + + attr_reader :project + + def initialize(project) + @project = project + end + + def execute + try_obtain_lease do + execute_unsafe + end + end + + def execute_unsafe + true + end + end + end + + let(:service) { implementation.new(project) } + + it 'allows method to be executed' do + expect(service).to receive(:execute_unsafe).and_call_original + + expect(service.execute).to eq(true) + end + + context 'when another service holds the lease for the same project' do + around do |example| + implementation.new(project).try_obtain_lease do + example.run + end + end + + it 'does not run guarded method' do + expect(service).not_to receive(:execute_unsafe) + + expect(service.execute).to eq(nil) + end + + it 'runs guarded method if feature flag is disabled' do + stub_feature_flags(pages_use_legacy_storage_lease: false) + + expect(service).to receive(:execute_unsafe).and_call_original + + expect(service.execute).to eq(true) + end + end + + context 'when another service holds the lease for the different project' do + around do |example| + implementation.new(create(:project)).try_obtain_lease do + example.run + end + end + + it 'allows method to be executed' do + expect(service).to receive(:execute_unsafe).and_call_original + + expect(service.execute).to eq(true) + end + end +end diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb new file mode 100644 index 00000000000..1568103d102 --- /dev/null +++ b/spec/services/pages/zip_directory_service_spec.rb @@ -0,0 +1,209 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Pages::ZipDirectoryService do + around do |example| + Dir.mktmpdir do |dir| + @work_dir = dir + example.run + end + end + + let(:result) do + described_class.new(@work_dir).execute + end + + let(:archive) { result.first } + let(:entries_count) { result.second } + + it 'raises error if there is no public directory' do + expect { archive }.to raise_error(described_class::InvalidArchiveError) + end + + it 'raises error if public directory is a symlink' do + create_dir('target') + create_file('./target/index.html', 'hello') + create_link("public", "./target") + + expect { archive }.to raise_error(described_class::InvalidArchiveError) + end + + context 'when there is a public directory' do + before do + create_dir('public') + end + + it 'creates the file next the public directory' do + expect(archive).to eq(File.join(@work_dir, "@migrated.zip")) + end + + it 'includes public directory' do + with_zip_file do |zip_file| + entry = zip_file.get_entry("public/") + expect(entry.ftype).to eq(:directory) + end + end + + it 'returns number of entries' do + create_file("public/index.html", "hello") + create_link("public/link.html", "./index.html") + expect(entries_count).to eq(3) # + 'public' directory + end + + it 'removes the old file if it exists' do + # simulate the old run + described_class.new(@work_dir).execute + + with_zip_file do |zip_file| + expect(zip_file.entries.count).to eq(1) + end + end + + it 'ignores other top level files and directories' do + create_file("top_level.html", "hello") + create_dir("public2") + + with_zip_file do |zip_file| + expect { zip_file.get_entry("top_level.html") }.to raise_error(Errno::ENOENT) + expect { zip_file.get_entry("public2/") }.to raise_error(Errno::ENOENT) + end + end + + it 'includes index.html file' do + create_file("public/index.html", "Hello!") + + with_zip_file do |zip_file| + entry = zip_file.get_entry("public/index.html") + expect(zip_file.read(entry)).to eq("Hello!") + end + end + + it 'includes hidden file' do + create_file("public/.hidden.html", "Hello!") + + with_zip_file do |zip_file| + entry = zip_file.get_entry("public/.hidden.html") + expect(zip_file.read(entry)).to eq("Hello!") + end + end + + it 'includes nested directories and files' do + create_dir("public/nested") + create_dir("public/nested/nested2") + create_file("public/nested/nested2/nested.html", "Hello nested") + + with_zip_file do |zip_file| + entry = zip_file.get_entry("public/nested") + expect(entry.ftype).to eq(:directory) + + entry = zip_file.get_entry("public/nested/nested2") + expect(entry.ftype).to eq(:directory) + + entry = zip_file.get_entry("public/nested/nested2/nested.html") + expect(zip_file.read(entry)).to eq("Hello nested") + end + end + + it 'adds a valid symlink' do + create_file("public/target.html", "hello") + create_link("public/link.html", "./target.html") + + with_zip_file do |zip_file| + entry = zip_file.get_entry("public/link.html") + expect(entry.ftype).to eq(:symlink) + expect(zip_file.read(entry)).to eq("./target.html") + end + end + + it 'ignores the symlink pointing outside of public directory' do + create_file("target.html", "hello") + create_link("public/link.html", "../target.html") + + with_zip_file do |zip_file| + expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + end + end + + it 'ignores the symlink if target is absent' do + create_link("public/link.html", "./target.html") + + with_zip_file do |zip_file| + expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + end + end + + it 'ignores symlink if is absolute and points to outside of directory' do + target = File.join(@work_dir, "target") + FileUtils.touch(target) + + create_link("public/link.html", target) + + with_zip_file do |zip_file| + expect { zip_file.get_entry("public/link.html") }.to raise_error(Errno::ENOENT) + end + end + + it "includes raw symlink if it's target is a valid directory" do + create_dir("public/target") + create_file("public/target/index.html", "hello") + create_link("public/link", "./target") + + with_zip_file do |zip_file| + expect(zip_file.entries.count).to eq(4) # /public and 3 created above + + entry = zip_file.get_entry("public/link") + expect(entry.ftype).to eq(:symlink) + expect(zip_file.read(entry)).to eq("./target") + end + end + end + + context "validating fixtures pages archives" do + using RSpec::Parameterized::TableSyntax + + where(:fixture_path) do + ["spec/fixtures/pages.zip", "spec/fixtures/pages_non_writeable.zip"] + end + + with_them do + let(:full_fixture_path) { Rails.root.join(fixture_path) } + + it 'a created archives contains exactly the same entries' do + SafeZip::Extract.new(full_fixture_path).extract(directories: ['public'], to: @work_dir) + + with_zip_file do |created_archive| + Zip::File.open(full_fixture_path) do |original_archive| + original_archive.entries do |original_entry| + created_entry = created_archive.get_entry(original_entry.name) + + expect(created_entry.name).to eq(original_entry.name) + expect(created_entry.ftype).to eq(original_entry.ftype) + expect(created_archive.read(created_entry)).to eq(original_archive.read(original_entry)) + end + end + end + end + end + end + + def create_file(name, content) + File.open(File.join(@work_dir, name), "w") do |f| + f.write(content) + end + end + + def create_dir(dir) + Dir.mkdir(File.join(@work_dir, dir)) + end + + def create_link(new_name, target) + File.symlink(target, File.join(@work_dir, new_name)) + end + + def with_zip_file + Zip::File.open(archive) do |zip_file| + yield zip_file + end + end +end diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 7c4b7f51cc3..4e303bfc20a 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -45,6 +45,12 @@ RSpec.describe PostReceiveService do it 'does not return error' do expect(subject).to be_empty end + + it 'does not record a namespace onboarding progress action' do + expect(NamespaceOnboardingAction).not_to receive(:create_action) + + subject + end end context 'when repository is nil' do @@ -80,6 +86,13 @@ RSpec.describe PostReceiveService do expect(response.reference_counter_decreased).to be(true) end + + it 'records a namespace onboarding progress action' do + expect(NamespaceOnboardingAction).to receive(:create_action) + .with(project.namespace, :git_write) + + subject + end end context 'with Project' do diff --git a/spec/services/projects/alerting/notify_service_spec.rb b/spec/services/projects/alerting/notify_service_spec.rb index 4674f614cf1..4b7b7b0b200 100644 --- a/spec/services/projects/alerting/notify_service_spec.rb +++ b/spec/services/projects/alerting/notify_service_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Projects::Alerting::NotifyService do let(:token) { 'invalid-token' } let(:starts_at) { Time.current.change(usec: 0) } let(:fingerprint) { 'testing' } - let(:service) { described_class.new(project, nil, payload) } + let(:service) { described_class.new(project, payload) } let_it_be(:environment) { create(:environment, project: project) } let(:environment) { create(:environment, project: project) } let(:ended_at) { nil } @@ -54,7 +54,6 @@ RSpec.describe Projects::Alerting::NotifyService do shared_examples 'assigns the alert properties' do it 'ensure that created alert has all data properly assigned' do subject - expect(last_alert_attributes).to match( project_id: project.id, title: payload_raw.fetch(:title), @@ -62,6 +61,7 @@ RSpec.describe Projects::Alerting::NotifyService do severity: payload_raw.fetch(:severity), status: AlertManagement::Alert.status_value(:triggered), events: 1, + domain: 'operations', hosts: payload_raw.fetch(:hosts), payload: payload_raw.with_indifferent_access, issue_id: nil, @@ -187,6 +187,7 @@ RSpec.describe Projects::Alerting::NotifyService do status: AlertManagement::Alert.status_value(:triggered), events: 1, hosts: [], + domain: 'operations', payload: payload_raw.with_indifferent_access, issue_id: nil, description: nil, 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 a012ec29be5..4da416d9698 100644 --- a/spec/services/projects/container_repository/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/delete_tags_service_spec.rb @@ -20,6 +20,7 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do service_class: 'Projects::ContainerRepository::DeleteTagsService', message: 'deleted tags', container_repository_id: repository.id, + project_id: repository.project_id, deleted_tags_count: tags.size ) @@ -32,7 +33,8 @@ RSpec.describe Projects::ContainerRepository::DeleteTagsService do log_data = { service_class: 'Projects::ContainerRepository::DeleteTagsService', message: message, - container_repository_id: repository.id + container_repository_id: repository.id, + project_id: repository.project_id } log_data.merge!(extra_log) if extra_log.any? diff --git a/spec/services/projects/fork_service_spec.rb b/spec/services/projects/fork_service_spec.rb index 555f2f5a5e5..60dfee820ca 100644 --- a/spec/services/projects/fork_service_spec.rb +++ b/spec/services/projects/fork_service_spec.rb @@ -325,7 +325,7 @@ RSpec.describe Projects::ForkService do storage_move = create( :project_repository_storage_move, :scheduled, - project: project, + container: project, destination_storage_name: 'test_second_storage' ) Projects::UpdateRepositoryStorageService.new(storage_move).execute @@ -344,10 +344,6 @@ RSpec.describe Projects::ForkService do let(:fork_from_project) { create(:project, :public) } let(:forker) { create(:user) } - before do - stub_feature_flags(object_pools: true) - end - context 'when no pool exists' do it 'creates a new object pool' do forked_project = fork_project(fork_from_project, forker, using_service: true) diff --git a/spec/services/projects/git_deduplication_service_spec.rb b/spec/services/projects/git_deduplication_service_spec.rb index b98db5bc41b..e6eff936de7 100644 --- a/spec/services/projects/git_deduplication_service_spec.rb +++ b/spec/services/projects/git_deduplication_service_spec.rb @@ -139,7 +139,7 @@ RSpec.describe Projects::GitDeduplicationService do end it 'fails when a lease is already out' do - expect(service).to receive(:log_error).with("Cannot obtain an exclusive lease for #{service.class.name}. There must be another instance already in execution.") + expect(service).to receive(:log_error).with("Cannot obtain an exclusive lease for #{lease_key}. There must be another instance already in execution.") service.execute end diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index f0fd243f0ca..47252bcf7a7 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -28,7 +28,7 @@ RSpec.describe Projects::HashedStorage::MigrateRepositoryService do end it 'fails when a git operation is in progress' do - allow(project).to receive(:repo_reference_count) { 1 } + allow(project).to receive(:git_transfer_in_progress?) { true } expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryInUseError) end diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index 492eb0956aa..af128a532b9 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -28,7 +28,7 @@ RSpec.describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab end it 'fails when a git operation is in progress' do - allow(project).to receive(:repo_reference_count) { 1 } + allow(project).to receive(:git_transfer_in_progress?) { true } expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryInUseError) end diff --git a/spec/services/projects/move_access_service_spec.rb b/spec/services/projects/move_access_service_spec.rb index 02f80988dd1..90167ffebed 100644 --- a/spec/services/projects/move_access_service_spec.rb +++ b/spec/services/projects/move_access_service_spec.rb @@ -91,7 +91,7 @@ RSpec.describe Projects::MoveAccessService do it 'does not remove remaining memberships' do target_project.add_maintainer(maintainer_user) - subject.execute(project_with_access, options) + subject.execute(project_with_access, **options) expect(project_with_access.project_members.count).not_to eq 0 end @@ -99,7 +99,7 @@ RSpec.describe Projects::MoveAccessService do it 'does not remove remaining group links' do target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) - subject.execute(project_with_access, options) + subject.execute(project_with_access, **options) expect(project_with_access.project_group_links.count).not_to eq 0 end @@ -107,7 +107,7 @@ RSpec.describe Projects::MoveAccessService do it 'does not remove remaining authorizations' do target_project.add_developer(developer_user) - subject.execute(project_with_access, options) + subject.execute(project_with_access, **options) expect(project_with_access.project_authorizations.count).not_to eq 0 end diff --git a/spec/services/projects/move_deploy_keys_projects_service_spec.rb b/spec/services/projects/move_deploy_keys_projects_service_spec.rb index e69b4dd4fc7..bd93b80f712 100644 --- a/spec/services/projects/move_deploy_keys_projects_service_spec.rb +++ b/spec/services/projects/move_deploy_keys_projects_service_spec.rb @@ -51,7 +51,7 @@ RSpec.describe Projects::MoveDeployKeysProjectsService do it 'does not remove remaining deploy keys projects' do target_project.deploy_keys << project_with_deploy_keys.deploy_keys.first - subject.execute(project_with_deploy_keys, options) + subject.execute(project_with_deploy_keys, **options) expect(project_with_deploy_keys.deploy_keys_projects.count).not_to eq 0 end diff --git a/spec/services/projects/move_lfs_objects_projects_service_spec.rb b/spec/services/projects/move_lfs_objects_projects_service_spec.rb index b73286fba9a..e3df5fed9cf 100644 --- a/spec/services/projects/move_lfs_objects_projects_service_spec.rb +++ b/spec/services/projects/move_lfs_objects_projects_service_spec.rb @@ -48,7 +48,7 @@ RSpec.describe Projects::MoveLfsObjectsProjectsService do it 'does not remove remaining lfs objects' do target_project.lfs_objects << project_with_lfs_objects.lfs_objects.first(2) - subject.execute(project_with_lfs_objects, options) + subject.execute(project_with_lfs_objects, **options) expect(project_with_lfs_objects.lfs_objects.count).not_to eq 0 end diff --git a/spec/services/projects/move_notification_settings_service_spec.rb b/spec/services/projects/move_notification_settings_service_spec.rb index 7c9f1dd30d2..e381ae7590f 100644 --- a/spec/services/projects/move_notification_settings_service_spec.rb +++ b/spec/services/projects/move_notification_settings_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Projects::MoveNotificationSettingsService do let(:options) { { remove_remaining_elements: false } } it 'does not remove remaining notification settings' do - subject.execute(project_with_notifications, options) + subject.execute(project_with_notifications, **options) expect(project_with_notifications.notification_settings.count).not_to eq 0 end diff --git a/spec/services/projects/move_project_authorizations_service_spec.rb b/spec/services/projects/move_project_authorizations_service_spec.rb index a37b4d807a0..d47b13ca939 100644 --- a/spec/services/projects/move_project_authorizations_service_spec.rb +++ b/spec/services/projects/move_project_authorizations_service_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Projects::MoveProjectAuthorizationsService do target_project.add_maintainer(developer_user) target_project.add_developer(reporter_user) - subject.execute(project_with_users, options) + subject.execute(project_with_users, **options) expect(project_with_users.project_authorizations.count).not_to eq 0 end diff --git a/spec/services/projects/move_project_group_links_service_spec.rb b/spec/services/projects/move_project_group_links_service_spec.rb index 6304eded8d3..1fca96a0367 100644 --- a/spec/services/projects/move_project_group_links_service_spec.rb +++ b/spec/services/projects/move_project_group_links_service_spec.rb @@ -58,7 +58,7 @@ RSpec.describe Projects::MoveProjectGroupLinksService do target_project.project_group_links.create!(group: maintainer_group, group_access: Gitlab::Access::MAINTAINER) target_project.project_group_links.create!(group: developer_group, group_access: Gitlab::Access::DEVELOPER) - subject.execute(project_with_groups, options) + subject.execute(project_with_groups, **options) expect(project_with_groups.project_group_links.count).not_to eq 0 end diff --git a/spec/services/projects/move_project_members_service_spec.rb b/spec/services/projects/move_project_members_service_spec.rb index f14f00e3866..8fbd0ba3270 100644 --- a/spec/services/projects/move_project_members_service_spec.rb +++ b/spec/services/projects/move_project_members_service_spec.rb @@ -58,7 +58,7 @@ RSpec.describe Projects::MoveProjectMembersService do target_project.add_maintainer(developer_user) target_project.add_developer(reporter_user) - subject.execute(project_with_users, options) + subject.execute(project_with_users, **options) expect(project_with_users.project_members.count).not_to eq 0 end diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb index 294c9adcc92..c739fea5ecf 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -10,14 +10,6 @@ RSpec.describe Projects::OpenIssuesCountService, :use_clean_rails_memory_store_c it_behaves_like 'a counter caching service' describe '#count' do - it 'does not count test cases' do - create(:issue, :opened, project: project) - create(:incident, :opened, project: project) - create(:quality_test_case, :opened, project: project) - - expect(described_class.new(project).count).to eq(2) - end - context 'when user is nil' do it 'does not include confidential issues in the issue count' do create(:issue, :opened, project: project) diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb index 33a3e37a2d2..b84e28314f2 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -3,57 +3,100 @@ require 'spec_helper' RSpec.describe Projects::ParticipantsService do - describe '#groups' do + describe '#execute' do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :public) } - let(:service) { described_class.new(project, user) } + let_it_be(:noteable) { create(:issue, project: project) } - it 'avoids N+1 queries' do - group_1 = create(:group) - group_1.add_owner(user) + before_all do + project.add_developer(user) + end - service.groups # Run general application warmup queries - control_count = ActiveRecord::QueryRecorder.new { service.groups }.count + def run_service + described_class.new(project, user).execute(noteable) + end - group_2 = create(:group) - group_2.add_owner(user) + context 'N+1 checks' do + before do + run_service # warmup, runs table cache queries and create queries + BatchLoader::Executor.clear_current + end - expect { service.groups }.not_to exceed_query_limit(control_count) - end + it 'avoids N+1 UserDetail queries' do + project.add_developer(create(:user)) - it 'returns correct user counts for groups' do - group_1 = create(:group) - group_1.add_owner(user) - group_1.add_owner(create(:user)) + control_count = ActiveRecord::QueryRecorder.new { run_service.to_a }.count - group_2 = create(:group) - group_2.add_owner(user) - create(:group_member, :access_request, group: group_2, user: create(:user)) + BatchLoader::Executor.clear_current - expect(service.groups).to contain_exactly( - a_hash_including(name: group_1.full_name, count: 2), - a_hash_including(name: group_2.full_name, count: 1) - ) - end + project.add_developer(create(:user, status: build(:user_status, availability: :busy))) + + expect { run_service.to_a }.not_to exceed_query_limit(control_count) + end - describe 'avatar_url' do - let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) } + it 'avoids N+1 groups queries' do + group_1 = create(:group) + group_1.add_owner(user) - before do - group.add_owner(user) + control_count = ActiveRecord::QueryRecorder.new { run_service }.count + + BatchLoader::Executor.clear_current + + group_2 = create(:group) + group_2.add_owner(user) + + expect { run_service }.not_to exceed_query_limit(control_count) end + end + + it 'does not return duplicate author' do + participants = run_service - it 'returns an url for the avatar' do - expect(service.groups.size).to eq 1 - expect(service.groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png") + expect(participants.count { |p| p[:username] == noteable.author.username }).to eq 1 + end + + describe 'group items' do + subject(:group_items) { run_service.select { |hash| hash[:type].eql?('Group') } } + + describe 'group user counts' do + let(:group_1) { create(:group) } + let(:group_2) { create(:group) } + + before do + group_1.add_owner(user) + group_1.add_owner(create(:user)) + + group_2.add_owner(user) + create(:group_member, :access_request, group: group_2, user: create(:user)) + end + + it 'returns correct user counts for groups' do + expect(group_items).to contain_exactly( + a_hash_including(name: group_1.full_name, count: 2), + a_hash_including(name: group_2.full_name, count: 1) + ) + end end - it 'returns an url for the avatar with relative url' do - stub_config_setting(relative_url_root: '/gitlab') - stub_config_setting(url: Settings.send(:build_gitlab_url)) + describe 'avatar_url' do + let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) } + + before do + group.add_owner(user) + end - expect(service.groups.size).to eq 1 - expect(service.groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png") + it 'returns an url for the avatar' do + expect(group_items.size).to eq 1 + expect(group_items.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png") + end + + it 'returns an url for the avatar with relative url' do + stub_config_setting(relative_url_root: '/gitlab') + stub_config_setting(url: Settings.send(:build_gitlab_url)) + + expect(group_items.size).to eq 1 + expect(group_items.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png") + end end end end diff --git a/spec/services/projects/prometheus/alerts/notify_service_spec.rb b/spec/services/projects/prometheus/alerts/notify_service_spec.rb index 0e5ac7c69e3..8ae47ec266c 100644 --- a/spec/services/projects/prometheus/alerts/notify_service_spec.rb +++ b/spec/services/projects/prometheus/alerts/notify_service_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do let_it_be(:project, reload: true) { create(:project) } - let(:service) { described_class.new(project, nil, payload) } + let(:service) { described_class.new(project, payload) } let(:token_input) { 'token' } let!(:setting) do @@ -138,10 +138,10 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end end - context 'with generic alerts integration' do + context 'with HTTP integration' do using RSpec::Parameterized::TableSyntax - where(:alerts_service, :token, :result) do + where(:active, :token, :result) do :active | :valid | :success :active | :invalid | :failure :active | nil | :failure @@ -150,15 +150,12 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do end with_them do - let(:valid) { project.alerts_service.token } + let(:valid) { integration.token } let(:invalid) { 'invalid token' } let(:token_input) { public_send(token) if token } + let(:integration) { create(:alert_management_http_integration, active, project: project) if active } - before do - if alerts_service - create(:alerts_service, alerts_service, project: project) - end - end + let(:subject) { service.execute(token_input, integration) } case result = params[:result] when :success @@ -221,7 +218,7 @@ RSpec.describe Projects::Prometheus::Alerts::NotifyService do it 'processes Prometheus alerts' do expect(AlertManagement::ProcessPrometheusAlertService) .to receive(:new) - .with(project, nil, kind_of(Hash)) + .with(project, kind_of(Hash)) .exactly(3).times .and_return(process_service) expect(process_service).to receive(:execute).exactly(3).times diff --git a/spec/services/projects/schedule_bulk_repository_shard_moves_service_spec.rb b/spec/services/projects/schedule_bulk_repository_shard_moves_service_spec.rb new file mode 100644 index 00000000000..5b76386bfab --- /dev/null +++ b/spec/services/projects/schedule_bulk_repository_shard_moves_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ScheduleBulkRepositoryShardMovesService do + before do + stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' }) + end + + let!(:project) { create(:project, :repository).tap { |project| project.track_project_repository } } + let(:source_storage_name) { 'default' } + let(:destination_storage_name) { 'test_second_storage' } + + describe '#execute' do + it 'schedules project repository storage moves' do + expect { subject.execute(source_storage_name, destination_storage_name) } + .to change(ProjectRepositoryStorageMove, :count).by(1) + + storage_move = project.repository_storage_moves.last! + + expect(storage_move).to have_attributes( + source_storage_name: source_storage_name, + destination_storage_name: destination_storage_name, + state_name: :scheduled + ) + end + + context 'read-only repository' do + let!(:project) { create(:project, :repository, :read_only).tap { |project| project.track_project_repository } } + + it 'does not get scheduled' do + expect(subject).to receive(:log_info) + .with("Project #{project.full_path} (#{project.id}) was skipped: Project is read only") + expect { subject.execute(source_storage_name, destination_storage_name) } + .to change(ProjectRepositoryStorageMove, :count).by(0) + end + end + end + + describe '.enqueue' do + it 'defers to the worker' do + expect(::ProjectScheduleBulkRepositoryShardMovesWorker).to receive(:perform_async).with(source_storage_name, destination_storage_name) + + described_class.enqueue(source_storage_name, destination_storage_name) + end + end +end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 8e6147e7a3c..5f41ec1d610 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -7,6 +7,7 @@ RSpec.describe Projects::TransferService do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } + let_it_be(:group_integration) { create(:slack_service, group: group, project: nil, webhook: 'http://group.slack.com') } let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) } subject(:execute_transfer) { described_class.new(project, user).execute(group).tap { project.reload } } @@ -117,6 +118,30 @@ RSpec.describe Projects::TransferService do shard_name: project.repository_storage ) end + + context 'with a project integration' do + let_it_be_with_reload(:project) { create(:project, namespace: user.namespace) } + let_it_be(:instance_integration) { create(:slack_service, :instance, webhook: 'http://project.slack.com') } + + context 'with an inherited integration' do + let_it_be(:project_integration) { create(:slack_service, project: project, webhook: 'http://project.slack.com', inherit_from_id: instance_integration.id) } + + it 'replaces inherited integrations', :aggregate_failures do + execute_transfer + + expect(project.slack_service.webhook).to eq(group_integration.webhook) + expect(Service.count).to eq(3) + end + end + + context 'with a custom integration' do + let_it_be(:project_integration) { create(:slack_service, project: project, webhook: 'http://project.slack.com') } + + it 'does not updates the integrations' do + expect { execute_transfer }.not_to change { project.slack_service.webhook } + end + end + end end context 'when transfer fails' do @@ -527,7 +552,7 @@ RSpec.describe Projects::TransferService do group.add_owner(user) end - it 'schedules a job when pages are deployed' do + it 'schedules a job when pages are deployed' do project.mark_pages_as_deployed expect(PagesTransferWorker).to receive(:perform_async) diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index d2c6c0eb971..a15f6bdbe2c 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -35,13 +35,11 @@ RSpec.describe Projects::UpdatePagesService do build.reload end - describe 'pages artifacts' do - it "doesn't delete artifacts after deploying" do - expect(execute).to eq(:success) + it "doesn't delete artifacts after deploying" do + expect(execute).to eq(:success) - expect(project.pages_metadatum).to be_deployed - expect(build.artifacts?).to eq(true) - end + expect(project.pages_metadatum).to be_deployed + expect(build.artifacts?).to eq(true) end it 'succeeds' do @@ -71,6 +69,16 @@ RSpec.describe Projects::UpdatePagesService do expect(project.pages_metadatum.reload.pages_deployment_id).to eq(deployment.id) end + it 'fails if another deployment is in progress' do + subject.try_obtain_lease do + expect do + execute + end.to raise_error("Failed to deploy pages - other deployment is in progress") + + expect(GenericCommitStatus.last.description).to eq("Failed to deploy pages - other deployment is in progress") + end + end + it 'does not fail if pages_metadata is absent' do project.pages_metadatum.destroy! project.reload @@ -105,16 +113,6 @@ RSpec.describe Projects::UpdatePagesService do end end - it 'does not create deployment when zip_pages_deployments feature flag is disabled' do - stub_feature_flags(zip_pages_deployments: false) - - expect do - expect(execute).to eq(:success) - end.not_to change { project.pages_deployments.count } - - expect(project.pages_metadatum.reload.pages_deployment_id).to be_nil - end - it 'limits pages size' do stub_application_setting(max_pages_size: 1) expect(execute).not_to eq(:success) diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index 123f604e7a4..ef8f166cc3f 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do context 'without wiki and design repository' do let(:project) { create(:project, :repository, wiki_enabled: false) } let(:destination) { 'test_second_storage' } - let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, container: project, destination_storage_name: destination) } let!(:checksum) { project.repository.checksum } let(:project_repository_double) { double(:repository) } let(:original_project_repository_double) { double(:repository) } @@ -144,7 +144,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do end context 'when the repository move is finished' do - let(:repository_storage_move) { create(:project_repository_storage_move, :finished, project: project, destination_storage_name: destination) } + let(:repository_storage_move) { create(:project_repository_storage_move, :finished, container: project, destination_storage_name: destination) } it 'is idempotent' do expect do @@ -156,7 +156,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do end context 'when the repository move is failed' do - let(:repository_storage_move) { create(:project_repository_storage_move, :failed, project: project, destination_storage_name: destination) } + let(:repository_storage_move) { create(:project_repository_storage_move, :failed, container: project, destination_storage_name: destination) } it 'is idempotent' do expect do @@ -170,7 +170,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do 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') } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, container: project, destination_storage_name: 'test_second_storage') } it 'updates the database' do allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original @@ -191,7 +191,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do let(:project) { create(:project, :repository, wiki_enabled: true) } let(:repository) { project.wiki.repository } let(:destination) { 'test_second_storage' } - let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, container: project, destination_storage_name: destination) } before do project.create_wiki @@ -204,7 +204,7 @@ RSpec.describe Projects::UpdateRepositoryStorageService do let(:project) { create(:project, :repository) } let(:repository) { project.design_repository } let(:destination) { 'test_second_storage' } - let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, container: project, destination_storage_name: destination) } before do project.design_repository.create_if_not_exists diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index 1f521ed4a93..e6d1d0e90a7 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1584,7 +1584,7 @@ RSpec.describe QuickActions::InterpretService do end end - it 'limits to commands passed ' do + it 'limits to commands passed' do content = "/shrug test\n/close" text, commands = service.execute(content, issue, only: [:shrug]) @@ -1593,7 +1593,7 @@ RSpec.describe QuickActions::InterpretService do expect(text).to eq("test #{described_class::SHRUG}\n/close") end - it 'preserves leading whitespace ' do + it 'preserves leading whitespace' do content = " - list\n\n/close\n\ntest\n\n" text, _ = service.execute(content, issue) diff --git a/spec/services/releases/create_service_spec.rb b/spec/services/releases/create_service_spec.rb index b9294182883..7287825a0be 100644 --- a/spec/services/releases/create_service_spec.rb +++ b/spec/services/releases/create_service_spec.rb @@ -167,28 +167,47 @@ RSpec.describe Releases::CreateService do end end - context 'when no milestone is passed in' do - it 'creates a release without a milestone tied to it' do - expect(params.key?(:milestones)).to be_falsey + context 'no milestone association behavior' do + let(:title_1) { 'v1.0' } + let(:title_2) { 'v1.0-rc' } + let!(:milestone_1) { create(:milestone, :active, project: project, title: title_1) } + let!(:milestone_2) { create(:milestone, :active, project: project, title: title_2) } - service.execute - release = project.releases.last + context 'when no milestones parameter is passed' do + it 'creates a release without a milestone tied to it' do + expect(service.param_for_milestone_titles_provided?).to be_falsey - expect(release.milestones).to be_empty + service.execute + release = project.releases.last + + expect(release.milestones).to be_empty + end + + it 'does not create any new MilestoneRelease object' do + expect { service.execute }.not_to change { MilestoneRelease.count } + end end - it 'does not create any new MilestoneRelease object' do - expect { service.execute }.not_to change { MilestoneRelease.count } + context 'when an empty array is passed as the milestones parameter' do + it 'creates a release without a milestone tied to it' do + service = described_class.new(project, user, params.merge!({ milestones: [] })) + service.execute + release = project.releases.last + + expect(release.milestones).to be_empty + end end - end - context 'when an empty value is passed as a milestone' do - it 'creates a release without a milestone tied to it' do - service = described_class.new(project, user, params.merge!({ milestones: [] })) - service.execute - release = project.releases.last + context 'when nil is passed as the milestones parameter' do + it 'creates a release without a milestone tied to it' do + expect(service.param_for_milestone_titles_provided?).to be_falsey - expect(release.milestones).to be_empty + service = described_class.new(project, user, params.merge!({ milestones: nil })) + service.execute + release = project.releases.last + + expect(release.milestones).to be_empty + end end end end @@ -217,7 +236,7 @@ RSpec.describe Releases::CreateService do let(:released_at) { 3.weeks.ago } it 'does not execute CreateEvidenceWorker' do - expect { subject }.not_to change(CreateEvidenceWorker.jobs, :size) + expect { subject }.not_to change(Releases::CreateEvidenceWorker.jobs, :size) end it 'does not create an Evidence object', :sidekiq_inline do @@ -316,7 +335,7 @@ RSpec.describe Releases::CreateService do end it 'queues CreateEvidenceWorker' do - expect { subject }.to change(CreateEvidenceWorker.jobs, :size).by(1) + expect { subject }.to change(Releases::CreateEvidenceWorker.jobs, :size).by(1) end it 'creates Evidence', :sidekiq_inline do @@ -341,18 +360,12 @@ RSpec.describe Releases::CreateService do context 'upcoming release' do let(:released_at) { 1.day.from_now } - it 'queues CreateEvidenceWorker' do - expect { subject }.to change(CreateEvidenceWorker.jobs, :size).by(1) - end - - it 'queues CreateEvidenceWorker at the released_at timestamp' do - subject - - expect(CreateEvidenceWorker.jobs.last['at'].to_i).to eq(released_at.to_i) + it 'does not execute CreateEvidenceWorker' do + expect { subject }.not_to change(Releases::CreateEvidenceWorker.jobs, :size) end - it 'creates Evidence', :sidekiq_inline do - expect { subject }.to change(Releases::Evidence, :count).by(1) + it 'does not create an Evidence object', :sidekiq_inline do + expect { subject }.not_to change(Releases::Evidence, :count) end it 'is not a historical release' do @@ -366,8 +379,6 @@ RSpec.describe Releases::CreateService do expect(last_release.upcoming_release?).to be_truthy end - - include_examples 'uses the right pipeline for evidence' end end end diff --git a/spec/services/resource_events/change_labels_service_spec.rb b/spec/services/resource_events/change_labels_service_spec.rb index efee185669e..8eac6ae0b49 100644 --- a/spec/services/resource_events/change_labels_service_spec.rb +++ b/spec/services/resource_events/change_labels_service_spec.rb @@ -10,7 +10,7 @@ RSpec.describe ResourceEvents::ChangeLabelsService do describe '.change_labels' do subject { described_class.new(resource, author).execute(added_labels: added, removed_labels: removed) } - let(:labels) { create_list(:label, 2, project: project) } + let_it_be(:labels) { create_list(:label, 2, project: project) } def expect_label_event(event, label, action) expect(event.user).to eq(author) @@ -57,5 +57,28 @@ RSpec.describe ResourceEvents::ChangeLabelsService do expect { subject }.to change { resource.resource_label_events.count }.from(0).to(2) end end + + describe 'usage data' do + let(:added) { [labels[0]] } + let(:removed) { [labels[1]] } + + context 'when resource is an issue' do + it 'tracks changed labels' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).to receive(:track_issue_label_changed_action) + + subject + end + end + + context 'when resource is a merge request' do + let(:resource) { create(:merge_request, source_project: project) } + + it 'does not track changed labels' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter).not_to receive(:track_issue_label_changed_action) + + subject + end + end + end end end diff --git a/spec/services/submit_usage_ping_service_spec.rb b/spec/services/submit_usage_ping_service_spec.rb index 2082a163b29..24afa83ef2c 100644 --- a/spec/services/submit_usage_ping_service_spec.rb +++ b/spec/services/submit_usage_ping_service_spec.rb @@ -134,10 +134,9 @@ RSpec.describe SubmitUsagePingService do it_behaves_like 'saves DevOps report data from the response' end - context 'with save_raw_usage_data feature enabled' do + context 'with saving raw_usage_data' do before do stub_response(body: with_dev_ops_score_params) - stub_feature_flags(save_raw_usage_data: true) end it 'creates a raw_usage_data record' do @@ -159,18 +158,6 @@ RSpec.describe SubmitUsagePingService do end end - context 'with save_raw_usage_data feature disabled' do - before do - stub_response(body: with_dev_ops_score_params) - end - - it 'does not create a raw_usage_data record' do - stub_feature_flags(save_raw_usage_data: false) - - expect { subject.execute }.to change(RawUsageData, :count).by(0) - end - end - context 'and usage ping response has unsuccessful status' do before do stub_response(body: nil, status: 504) diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index d8ade0fbbda..3e7594bd30f 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -20,7 +20,7 @@ RSpec.describe Suggestions::ApplyService do position_args = args.slice(:old_path, :new_path, :old_line, :new_line) content_args = args.slice(:from_content, :to_content) - position = build_position(position_args) + position = build_position(**position_args) diff_note = create(:diff_note_on_merge_request, noteable: merge_request, diff --git a/spec/services/system_hooks_service_spec.rb b/spec/services/system_hooks_service_spec.rb index b25837ca260..2020c67f465 100644 --- a/spec/services/system_hooks_service_spec.rb +++ b/spec/services/system_hooks_service_spec.rb @@ -159,9 +159,6 @@ RSpec.describe SystemHooksService do it { expect(event_name(group, :create)).to eq 'group_create' } it { expect(event_name(group, :destroy)).to eq 'group_destroy' } it { expect(event_name(group, :rename)).to eq 'group_rename' } - it { expect(event_name(group_member, :create)).to eq 'user_add_to_group' } - it { expect(event_name(group_member, :destroy)).to eq 'user_remove_from_group' } - it { expect(event_name(group_member, :update)).to eq 'user_update_for_group' } end def event_data(*args) diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index a4ae7e42958..9c35f9e3817 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -333,6 +333,19 @@ RSpec.describe SystemNoteService do end end + describe '.noteable_cloned' do + let(:noteable_ref) { double } + let(:direction) { double } + + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:noteable_cloned).with(noteable_ref, direction) + end + + described_class.noteable_cloned(double, double, noteable_ref, double, direction: direction) + end + end + describe 'Jira integration' do include JiraServiceHelper diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index b70c5e899fc..96afca2f2cb 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -522,6 +522,91 @@ RSpec.describe ::SystemNotes::IssuablesService do end end + describe '#noteable_cloned' do + let(:new_project) { create(:project) } + let(:new_noteable) { create(:issue, project: new_project) } + + subject do + service.noteable_cloned(new_noteable, direction) + end + + shared_examples 'cross project mentionable' do + include MarkupHelper + + it 'contains cross reference to new noteable' do + expect(subject.note).to include cross_project_reference(new_project, new_noteable) + end + + it 'mentions referenced noteable' do + expect(subject.note).to include new_noteable.to_reference + end + + it 'mentions referenced project' do + expect(subject.note).to include new_project.full_path + end + end + + context 'cloned to' do + let(:direction) { :to } + + it_behaves_like 'cross project mentionable' + + it_behaves_like 'a system note' do + let(:action) { 'cloned' } + end + + it 'notifies about noteable being cloned to' do + expect(subject.note).to match('cloned to') + end + end + + context 'cloned from' do + let(:direction) { :from } + + it_behaves_like 'cross project mentionable' + + it_behaves_like 'a system note' do + let(:action) { 'cloned' } + end + + it 'notifies about noteable being cloned from' do + expect(subject.note).to match('cloned from') + end + end + + context 'invalid direction' do + let(:direction) { :invalid } + + it 'raises error' do + expect { subject }.to raise_error StandardError, /Invalid direction/ + end + end + + context 'metrics' do + context 'cloned from' do + let(:direction) { :from } + + it 'does not tracks usage' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter) + .not_to receive(:track_issue_cloned_action).with(author: author) + + subject + end + end + + context 'cloned to' do + let(:direction) { :to } + + it 'tracks usage' do + expect(Gitlab::UsageDataCounters::IssueActivityUniqueCounter) + .to receive(:track_issue_cloned_action).with(author: author) + + subject + end + end + end + end + describe '#mark_duplicate_issue' do subject { service.mark_duplicate_issue(canonical_issue) } diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb index 9040966215c..74340bac055 100644 --- a/spec/services/users/create_service_spec.rb +++ b/spec/services/users/create_service_spec.rb @@ -59,7 +59,7 @@ RSpec.describe Users::CreateService do service.execute end - it 'executes system hooks ' do + it 'executes system hooks' do system_hook_service = spy(:system_hook_service) expect(service).to receive(:system_hook_service).and_return(system_hook_service) diff --git a/spec/services/users/reject_service_spec.rb b/spec/services/users/reject_service_spec.rb new file mode 100644 index 00000000000..07863d1a290 --- /dev/null +++ b/spec/services/users/reject_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::RejectService do + let_it_be(:current_user) { create(:admin) } + let(:user) { create(:user, :blocked_pending_approval) } + + subject(:execute) { described_class.new(current_user).execute(user) } + + describe '#execute' do + context 'failures' do + context 'when the executor user is not allowed to reject users' do + let(:current_user) { create(:user) } + + it 'returns error result' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to match(/You are not allowed to reject a user/) + end + 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(/This user does not have a pending request/) + end + end + end + end + + context 'success' do + context 'when the executor user is an admin in admin mode', :enable_admin_mode do + it 'deletes the user', :sidekiq_inline do + subject + + expect(subject[:status]).to eq(:success) + expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'emails the user on rejection' do + expect_next_instance_of(NotificationService) do |notification| + allow(notification).to receive(:user_admin_rejection).with(user.name, user.notification_email) + end + + subject + end + end + end + end +end diff --git a/spec/services/users/set_status_service_spec.rb b/spec/services/users/set_status_service_spec.rb index 69cd647eaeb..2c776a0eeb4 100644 --- a/spec/services/users/set_status_service_spec.rb +++ b/spec/services/users/set_status_service_spec.rb @@ -31,19 +31,28 @@ RSpec.describe Users::SetStatusService do expect(service.execute).to be(true) end + context 'when setting availability to not_set' do + before do + params[:availability] = 'not_set' + + create(:user_status, user: current_user, availability: 'busy') + end + + it 'updates the availability' do + expect { service.execute }.to change { current_user.status.availability }.from('busy').to('not_set') + end + end + context 'when the given availability value is not valid' do - let(:params) { { availability: 'not a valid value' } } + before do + params[:availability] = 'not a valid value' + end 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 @@ -69,11 +78,21 @@ RSpec.describe Users::SetStatusService do context 'without params' do let(:params) { {} } - it 'deletes the status' do - status = create(:user_status, user: current_user) + shared_examples 'removes user status record' do + it 'deletes the status' do + status = create(:user_status, user: current_user) + + expect { service.execute } + .to change { current_user.reload.status }.from(status).to(nil) + end + end + + it_behaves_like 'removes user status record' + + context 'when not_set is given for availability' do + let(:params) { { availability: 'not_set' } } - expect { service.execute } - .to change { current_user.reload.status }.from(status).to(nil) + it_behaves_like 'removes user status record' end end end diff --git a/spec/services/users/validate_otp_service_spec.rb b/spec/services/users/validate_otp_service_spec.rb index 826755d6145..42f0c10488c 100644 --- a/spec/services/users/validate_otp_service_spec.rb +++ b/spec/services/users/validate_otp_service_spec.rb @@ -20,7 +20,8 @@ RSpec.describe Users::ValidateOtpService do context 'FortiAuthenticator' do before do - stub_feature_flags(forti_authenticator: true) + stub_feature_flags(forti_authenticator: user) + allow(::Gitlab.config.forti_authenticator).to receive(:enabled).and_return(true) end it 'calls FortiAuthenticator strategy' do @@ -31,4 +32,19 @@ RSpec.describe Users::ValidateOtpService do validate end end + + context 'FortiTokenCloud' do + before do + stub_feature_flags(forti_token_cloud: user) + allow(::Gitlab.config.forti_token_cloud).to receive(:enabled).and_return(true) + end + + it 'calls FortiTokenCloud strategy' do + expect_next_instance_of(::Gitlab::Auth::Otp::Strategies::FortiTokenCloud) do |strategy| + expect(strategy).to receive(:validate).with(otp_code).once + end + + validate + end + end end |