diff options
Diffstat (limited to 'spec/models')
73 files changed, 2983 insertions, 812 deletions
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index bb8d476f257..5bd69ad9fad 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -394,7 +394,7 @@ RSpec.describe Ability do describe '.project_disabled_features_rules' do let(:project) { create(:project, :wiki_disabled) } - subject { described_class.policy_for(project.owner, project) } + subject { described_class.policy_for(project.first_owner, project) } context 'wiki named abilities' do it 'disables wiki abilities if the project has no wiki' do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 0ece212d692..a962703d460 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -141,7 +141,7 @@ RSpec.describe ApplicationSetting do it { is_expected.not_to allow_value('default' => 101).for(:repository_storages_weighted).with_message("value for 'default' must be between 0 and 100") } it { is_expected.not_to allow_value('default' => 100, shouldntexist: 50).for(:repository_storages_weighted).with_message("can't include: shouldntexist") } - %i[notes_create_limit user_email_lookup_limit].each do |setting| + %i[notes_create_limit user_email_lookup_limit users_get_by_id_limit].each do |setting| it { is_expected.to allow_value(400).for(setting) } it { is_expected.not_to allow_value('two').for(setting) } it { is_expected.not_to allow_value(nil).for(setting) } @@ -158,6 +158,11 @@ RSpec.describe ApplicationSetting do it { is_expected.not_to allow_value(nil).for(:notes_create_limit_allowlist) } it { is_expected.to allow_value([]).for(:notes_create_limit_allowlist) } + it { is_expected.to allow_value(many_usernames(100)).for(:users_get_by_id_limit_allowlist) } + it { is_expected.not_to allow_value(many_usernames(101)).for(:users_get_by_id_limit_allowlist) } + it { is_expected.not_to allow_value(nil).for(:users_get_by_id_limit_allowlist) } + it { is_expected.to allow_value([]).for(:users_get_by_id_limit_allowlist) } + it { is_expected.to allow_value('all_tiers').for(:whats_new_variant) } it { is_expected.to allow_value('current_tier').for(:whats_new_variant) } it { is_expected.to allow_value('disabled').for(:whats_new_variant) } diff --git a/spec/models/audit_event_spec.rb b/spec/models/audit_event_spec.rb index 4fba5fddc92..9f2724cebee 100644 --- a/spec/models/audit_event_spec.rb +++ b/spec/models/audit_event_spec.rb @@ -93,4 +93,37 @@ RSpec.describe AuditEvent do end end end + + describe '#author' do + subject { audit_event.author } + + context "when the target type is not Ci::Runner" do + let(:audit_event) { build(:project_audit_event, target_id: 678) } + + it 'returns a NullAuthor' do + expect(::Gitlab::Audit::NullAuthor).to receive(:for) + .and_call_original + .once + + is_expected.to be_a_kind_of(::Gitlab::Audit::NullAuthor) + end + end + + context 'when the target type is Ci::Runner and details contain runner_registration_token' do + let(:audit_event) { build(:project_audit_event, target_type: ::Ci::Runner.name, target_id: 678, details: { runner_registration_token: 'abc123' }) } + + it 'returns a CiRunnerTokenAuthor' do + expect(::Gitlab::Audit::CiRunnerTokenAuthor).to receive(:new) + .with(audit_event) + .and_call_original + .once + + is_expected.to be_an_instance_of(::Gitlab::Audit::CiRunnerTokenAuthor) + end + + it 'name consists of prefix and token' do + expect(subject.name).to eq('Registration token: abc123') + end + end + end end diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index bd4832bd978..6eba9ca63b0 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -215,6 +215,20 @@ RSpec.describe Blob do end end + describe '#symlink?' do + it 'is true for symlinks' do + symlink_blob = fake_blob(path: 'file', mode: '120000') + + expect(symlink_blob.symlink?).to eq true + end + + it 'is false for non-symlinks' do + non_symlink_blob = fake_blob(path: 'file', mode: '100755') + + expect(non_symlink_blob.symlink?).to eq false + end + end + describe '#extension' do it 'returns the extension' do blob = fake_blob(path: 'file.md') diff --git a/spec/models/board_spec.rb b/spec/models/board_spec.rb index 0b7c21fd0c3..90a06b80f9c 100644 --- a/spec/models/board_spec.rb +++ b/spec/models/board_spec.rb @@ -16,6 +16,10 @@ RSpec.describe Board do it { is_expected.to validate_presence_of(:project) } end + describe 'constants' do + it { expect(described_class::RECENT_BOARDS_SIZE).to be_a(Integer) } + end + describe '#order_by_name_asc' do let!(:board_B) { create(:board, project: project, name: 'B') } let!(:board_C) { create(:board, project: project, name: 'C') } diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb index b2ffb34da1d..5e30f9160cd 100644 --- a/spec/models/ci/build_metadata_spec.rb +++ b/spec/models/ci/build_metadata_spec.rb @@ -133,4 +133,11 @@ RSpec.describe Ci::BuildMetadata do expect(build.cancel_gracefully?).to be false end end + + context 'loose foreign key on ci_builds_metadata.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_build_metadata, project: parent) } + end + end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b8c5af5a911..90298f0e973 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3618,20 +3618,6 @@ RSpec.describe Ci::Build do build.scoped_variables end - - context 'when variables builder is used' do - it 'returns the same variables' do - build.user = create(:user) - - allow(build.pipeline).to receive(:use_variables_builder_definitions?).and_return(false) - legacy_variables = build.scoped_variables.to_hash - - allow(build.pipeline).to receive(:use_variables_builder_definitions?).and_return(true) - new_variables = build.scoped_variables.to_hash - - expect(new_variables).to eq(legacy_variables) - end - end end describe '#simple_variables_without_dependencies' do @@ -3642,160 +3628,6 @@ RSpec.describe Ci::Build do end end - shared_examples "secret CI variables" do - context 'when ref is branch' do - let(:pipeline) { create(:ci_pipeline, project: project) } - let(:build) { create(:ci_build, ref: 'master', tag: false, pipeline: pipeline, project: project) } - - context 'when ref is protected' do - before do - create(:protected_branch, :developers_can_merge, name: 'master', project: project) - end - - it { is_expected.to include(variable) } - end - - context 'when ref is not protected' do - it { is_expected.not_to include(variable) } - end - end - - context 'when ref is tag' do - let(:pipeline) { create(:ci_pipeline, project: project) } - let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, pipeline: pipeline, project: project) } - - context 'when ref is protected' do - before do - create(:protected_tag, project: project, name: 'v*') - end - - it { is_expected.to include(variable) } - end - - context 'when ref is not protected' do - it { is_expected.not_to include(variable) } - end - end - - context 'when ref is merge request' do - let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } - let(:pipeline) { merge_request.pipelines_for_merge_request.first } - let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) } - - context 'when ref is protected' do - before do - create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project) - end - - it 'does not return protected variables as it is not supported for merge request pipelines' do - is_expected.not_to include(variable) - end - end - - context 'when ref is not protected' do - it { is_expected.not_to include(variable) } - end - end - end - - describe '#secret_instance_variables' do - subject { build.secret_instance_variables } - - let_it_be(:variable) { create(:ci_instance_variable, protected: true) } - - include_examples "secret CI variables" - end - - describe '#secret_group_variables' do - subject { build.secret_group_variables } - - let_it_be(:variable) { create(:ci_group_variable, protected: true, group: group) } - - include_examples "secret CI variables" - end - - describe '#secret_project_variables' do - subject { build.secret_project_variables } - - let_it_be(:variable) { create(:ci_variable, protected: true, project: project) } - - include_examples "secret CI variables" - end - - describe '#kubernetes_variables' do - let(:build) { create(:ci_build) } - let(:service) { double(execute: template) } - let(:template) { double(to_yaml: 'example-kubeconfig', valid?: template_valid) } - let(:template_valid) { true } - - subject { build.kubernetes_variables } - - before do - allow(Ci::GenerateKubeconfigService).to receive(:new).with(build).and_return(service) - end - - it { is_expected.to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) } - - context 'generated config is invalid' do - let(:template_valid) { false } - - it { is_expected.not_to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) } - end - end - - describe '#deployment_variables' do - let(:build) { create(:ci_build, environment: environment) } - let(:environment) { 'production' } - let(:kubernetes_namespace) { 'namespace' } - let(:project_variables) { double } - - subject { build.deployment_variables(environment: environment) } - - before do - allow(build).to receive(:expanded_kubernetes_namespace) - .and_return(kubernetes_namespace) - - allow(build.project).to receive(:deployment_variables) - .with(environment: environment, kubernetes_namespace: kubernetes_namespace) - .and_return(project_variables) - end - - context 'environment is nil' do - let(:environment) { nil } - - it { is_expected.to be_empty } - end - end - - describe '#user_variables' do - subject { build.user_variables.to_hash } - - context 'with user' do - let(:expected_variables) do - { - 'GITLAB_USER_EMAIL' => user.email, - 'GITLAB_USER_ID' => user.id.to_s, - 'GITLAB_USER_LOGIN' => user.username, - 'GITLAB_USER_NAME' => user.name - } - end - - before do - build.user = user - end - - it { is_expected.to eq(expected_variables) } - end - - context 'without user' do - before do - expect(build).to receive(:user).and_return(nil) - end - - it { is_expected.to be_empty } - end - end - describe '#any_unmet_prerequisites?' do let(:build) { create(:ci_build, :created) } diff --git a/spec/models/ci/build_trace_chunk_spec.rb b/spec/models/ci/build_trace_chunk_spec.rb index 31c7c7a44bc..e08fe196d65 100644 --- a/spec/models/ci/build_trace_chunk_spec.rb +++ b/spec/models/ci/build_trace_chunk_spec.rb @@ -851,7 +851,7 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state, :clean_git context 'when project is destroyed' do let(:subject) do - Projects::DestroyService.new(project, project.owner).execute + Projects::DestroyService.new(project, project.first_owner).execute end it_behaves_like 'deletes all build_trace_chunk and data in redis' diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 2e8c41b410a..bd0397e0396 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -703,4 +703,11 @@ RSpec.describe Ci::JobArtifact do it_behaves_like 'it has loose foreign keys' do let(:factory_name) { :ci_job_artifact } end + + context 'loose foreign key on ci_job_artifacts.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_job_artifact, project: parent) } + end + end end diff --git a/spec/models/ci/job_token/project_scope_link_spec.rb b/spec/models/ci/job_token/project_scope_link_spec.rb index 8d7bb44bd16..c000a3e29f7 100644 --- a/spec/models/ci/job_token/project_scope_link_spec.rb +++ b/spec/models/ci/job_token/project_scope_link_spec.rb @@ -88,4 +88,18 @@ RSpec.describe Ci::JobToken::ProjectScopeLink do it { is_expected.to be_nil } end end + + context 'loose foreign key on ci_job_token_project_scope_links.source_project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_job_token_project_scope_link, source_project: parent) } + end + end + + context 'loose foreign key on ci_job_token_project_scope_links.target_project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_job_token_project_scope_link, target_project: parent) } + end + end end diff --git a/spec/models/ci/namespace_mirror_spec.rb b/spec/models/ci/namespace_mirror_spec.rb index a9d916115fc..38471f15849 100644 --- a/spec/models/ci/namespace_mirror_spec.rb +++ b/spec/models/ci/namespace_mirror_spec.rb @@ -20,10 +20,10 @@ RSpec.describe Ci::NamespaceMirror do end context 'scopes' do - describe '.contains_namespace' do + describe '.by_group_and_descendants' do let_it_be(:another_group) { create(:group) } - subject(:result) { described_class.contains_namespace(group2.id) } + subject(:result) { described_class.by_group_and_descendants(group2.id) } it 'returns groups having group2.id in traversal_ids' do expect(result.pluck(:namespace_id)).to contain_exactly(group2.id, group3.id, group4.id) diff --git a/spec/models/ci/pipeline_schedule_spec.rb b/spec/models/ci/pipeline_schedule_spec.rb index 0f1cb721e95..0f4f148775e 100644 --- a/spec/models/ci/pipeline_schedule_spec.rb +++ b/spec/models/ci/pipeline_schedule_spec.rb @@ -227,4 +227,11 @@ RSpec.describe Ci::PipelineSchedule do it { is_expected.to eq(144) } end end + + context 'loose foreign key on ci_pipeline_schedules.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_pipeline_schedule, project: parent) } + end + end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 90f56c1e0a4..c29cc04e0e9 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -390,20 +390,63 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end describe '#merge_request?' do - let(:pipeline) { create(:ci_pipeline, merge_request: merge_request) } - let(:merge_request) { create(:merge_request) } + let_it_be(:merge_request) { create(:merge_request) } + let_it_be_with_reload(:pipeline) do + create(:ci_pipeline, project: project, merge_request_id: merge_request.id) + end + + it { expect(pipeline).to be_merge_request } - it 'returns true' do - expect(pipeline).to be_merge_request + context 'when merge request is already loaded' do + it 'does not reload the record and returns true' do + expect(pipeline.merge_request).to be_present + + expect { pipeline.merge_request? }.not_to exceed_query_limit(0) + expect(pipeline).to be_merge_request + end end - context 'when merge request is nil' do - let(:merge_request) { nil } + context 'when merge request is not loaded' do + it 'executes a database query and returns true' do + expect(pipeline).to be_present - it 'returns false' do + expect { pipeline.merge_request? }.not_to exceed_query_limit(1) + expect(pipeline).to be_merge_request + end + + it 'caches the result' do + expect(pipeline).to be_merge_request + expect { pipeline.merge_request? }.not_to exceed_query_limit(0) + end + end + + context 'when merge request was removed' do + before do + pipeline.update!(merge_request_id: non_existing_record_id) + end + + it 'executes a database query and returns false' do + expect { pipeline.merge_request? }.not_to exceed_query_limit(1) expect(pipeline).not_to be_merge_request end end + + context 'when merge request id is not present' do + before do + pipeline.update!(merge_request_id: nil) + end + + it { expect(pipeline).not_to be_merge_request } + end + + context 'when the feature flag is disabled' do + before do + stub_feature_flags(ci_pipeline_merge_request_presence_check: false) + pipeline.update!(merge_request_id: non_existing_record_id) + end + + it { expect(pipeline).to be_merge_request } + end end describe '#detached_merge_request_pipeline?' do @@ -3384,7 +3427,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do create(:ci_pipeline, project: project, sha: project.commit('master').sha, - user: project.owner) + user: project.first_owner) end before do @@ -4459,7 +4502,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do describe '#reset_source_bridge!' do let(:pipeline) { create(:ci_pipeline, :created, project: project) } - subject(:reset_bridge) { pipeline.reset_source_bridge!(project.owner) } + subject(:reset_bridge) { pipeline.reset_source_bridge!(project.first_owner) } context 'when the pipeline is a child pipeline and the bridge is depended' do let!(:parent_pipeline) { create(:ci_pipeline) } @@ -4656,9 +4699,11 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do let(:factory_name) { :ci_pipeline } end - it_behaves_like 'cleanup by a loose foreign key' do - let!(:model) { create(:ci_pipeline, user: create(:user)) } - let!(:parent) { model.user } + context 'loose foreign key on ci_pipelines.user_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:model) { create(:ci_pipeline, user: create(:user)) } + let!(:parent) { model.user } + end end describe 'tags count' do @@ -4679,4 +4724,55 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { expect(pipeline.distinct_tags_count).to eq(3) } end end + + context 'loose foreign key on ci_pipelines.merge_request_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:merge_request) } + let!(:model) { create(:ci_pipeline, merge_request: parent) } + end + end + + context 'loose foreign key on ci_pipelines.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_pipeline, project: parent) } + end + end + + describe '#jobs_git_ref' do + subject { pipeline.jobs_git_ref } + + context 'when tag is true' do + let(:pipeline) { build(:ci_pipeline, tag: true) } + + it 'returns a tag ref' do + is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) + end + end + + context 'when tag is false' do + let(:pipeline) { build(:ci_pipeline, tag: false) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when tag is nil' do + let(:pipeline) { build(:ci_pipeline, tag: nil) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when it is triggered by a merge request' do + let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } + + it 'returns nil' do + is_expected.to be_nil + end + end + end end diff --git a/spec/models/ci/ref_spec.rb b/spec/models/ci/ref_spec.rb index 0a9cd5ef2ec..ffbda4b459f 100644 --- a/spec/models/ci/ref_spec.rb +++ b/spec/models/ci/ref_spec.rb @@ -231,4 +231,11 @@ RSpec.describe Ci::Ref do it_behaves_like 'no-op' end end + + context 'loose foreign key on ci_refs.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_ref, project: parent) } + end + end end diff --git a/spec/models/ci/runner_project_spec.rb b/spec/models/ci/runner_project_spec.rb index 13369dba2cf..40ad79c7295 100644 --- a/spec/models/ci/runner_project_spec.rb +++ b/spec/models/ci/runner_project_spec.rb @@ -6,4 +6,11 @@ RSpec.describe Ci::RunnerProject do it_behaves_like 'includes Limitable concern' do subject { build(:ci_runner_project, project: create(:project), runner: create(:ci_runner, :project)) } end + + context 'loose foreign key on ci_runner_projects.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_runner_project, project: parent) } + end + end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 6830a8daa3b..eb29db697a5 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe Ci::Runner do + include StubGitlabCalls + it_behaves_like 'having unique enum values' it_behaves_like 'it has loose foreign keys' do @@ -23,6 +25,16 @@ RSpec.describe Ci::Runner do end end + describe 'projects association' do + let(:runner) { create(:ci_runner, :project) } + + it 'does not create a cross-database query' do + with_cross_joins_prevented do + expect(runner.projects.count).to eq(1) + end + end + end + describe 'validation' do it { is_expected.to validate_presence_of(:access_level) } it { is_expected.to validate_presence_of(:runner_type) } @@ -255,22 +267,89 @@ RSpec.describe Ci::Runner do it_behaves_like '.belonging_to_parent_group_of_project' end - describe '.owned_or_instance_wide' do - it 'returns a globally shared, a project specific and a group specific runner' do - # group specific - group = create(:group) - project = create(:project, group: group) - group_runner = create(:ci_runner, :group, groups: [group]) + context 'with instance runners sharing enabled' do + # group specific + let_it_be(:group) { create(:group, shared_runners_enabled: true) } + let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) } + let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } - # project specific - project_runner = create(:ci_runner, :project, projects: [project]) + # project specific + let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project]) } - # globally shared - shared_runner = create(:ci_runner, :instance) + # globally shared + let_it_be(:shared_runner) { create(:ci_runner, :instance) } - expect(described_class.owned_or_instance_wide(project.id)).to contain_exactly( - group_runner, project_runner, shared_runner - ) + describe '.owned_or_instance_wide' do + subject { described_class.owned_or_instance_wide(project.id) } + + it 'returns a globally shared, a project specific and a group specific runner' do + is_expected.to contain_exactly(group_runner, project_runner, shared_runner) + end + end + + describe '.group_or_instance_wide' do + subject { described_class.group_or_instance_wide(group) } + + before do + # Ensure the project runner is instantiated + project_runner + end + + it 'returns a globally shared and a group specific runner' do + is_expected.to contain_exactly(group_runner, shared_runner) + end + end + end + + context 'with instance runners sharing disabled' do + # group specific + let_it_be(:group) { create(:group, shared_runners_enabled: false) } + let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) } + + let(:group_runners_enabled) { true } + let(:project) { create(:project, group: group, shared_runners_enabled: false) } + + # project specific + let(:project_runner) { create(:ci_runner, :project, projects: [project]) } + + # globally shared + let_it_be(:shared_runner) { create(:ci_runner, :instance) } + + before do + project.update!(group_runners_enabled: group_runners_enabled) + end + + describe '.owned_or_instance_wide' do + subject { described_class.owned_or_instance_wide(project.id) } + + context 'with group runners disabled' do + let(:group_runners_enabled) { false } + + it 'returns only the project specific runner' do + is_expected.to contain_exactly(project_runner) + end + end + + context 'with group runners enabled' do + let(:group_runners_enabled) { true } + + it 'returns a project specific and a group specific runner' do + is_expected.to contain_exactly(group_runner, project_runner) + end + end + end + + describe '.group_or_instance_wide' do + subject { described_class.group_or_instance_wide(group) } + + before do + # Ensure the project runner is instantiated + project_runner + end + + it 'returns a group specific runner' do + is_expected.to contain_exactly(group_runner) + end end end @@ -291,6 +370,30 @@ RSpec.describe Ci::Runner do end end + describe '#only_for' do + let_it_be_with_reload(:runner) { create(:ci_runner, :project) } + let_it_be(:project) { runner.projects.first } + + subject { runner.only_for?(project) } + + context 'with matching project' do + it { is_expected.to be_truthy } + end + + context 'without matching project' do + let_it_be(:project) { create(:project) } + + it { is_expected.to be_falsey } + end + + context 'with runner having multiple projects' do + let_it_be(:other_project) { create(:project) } + let_it_be(:runner_project) { create(:ci_runner_project, project: other_project, runner: runner) } + + it { is_expected.to be_falsey } + end + end + describe '#assign_to' do let(:project) { create(:project) } @@ -544,7 +647,7 @@ RSpec.describe Ci::Runner do end end - describe '#can_pick?' do + describe '#matches_build?' do using RSpec::Parameterized::TableSyntax let_it_be(:pipeline) { create(:ci_pipeline) } @@ -555,31 +658,15 @@ RSpec.describe Ci::Runner do let(:tag_list) { [] } let(:run_untagged) { true } - subject { runner.can_pick?(build) } - - context 'a different runner' do - let(:other_project) { create(:project) } - let(:other_runner) { create(:ci_runner, :project, projects: [other_project], tag_list: tag_list, run_untagged: run_untagged) } - - before do - # `can_pick?` is not used outside the runners available for the project - stub_feature_flags(ci_runners_short_circuit_assignable_for: false) - end - - it 'cannot handle builds' do - expect(other_runner.can_pick?(build)).to be_falsey - end - end + subject { runner.matches_build?(build) } context 'when runner does not have tags' do - it 'can handle builds without tags' do - expect(runner.can_pick?(build)).to be_truthy - end + it { is_expected.to be_truthy } it 'cannot handle build with tags' do build.tag_list = ['aa'] - expect(runner.can_pick?(build)).to be_falsey + is_expected.to be_falsey end end @@ -590,20 +677,18 @@ RSpec.describe Ci::Runner do it 'can handle build with matching tags' do build.tag_list = ['bb'] - expect(runner.can_pick?(build)).to be_truthy + is_expected.to be_truthy end it 'cannot handle build without matching tags' do build.tag_list = ['aa'] - expect(runner.can_pick?(build)).to be_falsey + is_expected.to be_falsey end end context 'when runner can pick untagged jobs' do - it 'can handle builds without tags' do - expect(runner.can_pick?(build)).to be_truthy - end + it { is_expected.to be_truthy } it_behaves_like 'tagged build picker' end @@ -611,9 +696,7 @@ RSpec.describe Ci::Runner do context 'when runner cannot pick untagged jobs' do let(:run_untagged) { false } - it 'cannot handle builds without tags' do - expect(runner.can_pick?(build)).to be_falsey - end + it { is_expected.to be_falsey } it_behaves_like 'tagged build picker' end @@ -622,64 +705,31 @@ RSpec.describe Ci::Runner do context 'when runner is shared' do let(:runner) { create(:ci_runner, :instance) } - it 'can handle builds' do - expect(runner.can_pick?(build)).to be_truthy - end + it { is_expected.to be_truthy } context 'when runner is locked' do let(:runner) { create(:ci_runner, :instance, locked: true) } - it 'can handle builds' do - expect(runner.can_pick?(build)).to be_truthy - end + it { is_expected.to be_truthy } end it 'does not query for owned or instance runners' do expect(described_class).not_to receive(:owned_or_instance_wide) - runner.can_pick?(build) - end - - context 'when feature flag ci_runners_short_circuit_assignable_for is disabled' do - before do - stub_feature_flags(ci_runners_short_circuit_assignable_for: false) - end - - it 'does not query for owned or instance runners' do - expect(described_class).to receive(:owned_or_instance_wide).and_call_original - - runner.can_pick?(build) - end + subject end end context 'when runner is not shared' do - before do - # `can_pick?` is not used outside the runners available for the project - stub_feature_flags(ci_runners_short_circuit_assignable_for: false) - end - context 'when runner is assigned to a project' do - it 'can handle builds' do - expect(runner.can_pick?(build)).to be_truthy - end - end - - context 'when runner is assigned to another project' do - let(:runner_project) { create(:project) } - - it 'cannot handle builds' do - expect(runner.can_pick?(build)).to be_falsey - end + it { is_expected.to be_truthy } end context 'when runner is assigned to a group' do let(:group) { create(:group, projects: [build.project]) } let(:runner) { create(:ci_runner, :group, tag_list: tag_list, run_untagged: run_untagged, groups: [group]) } - it 'can handle builds' do - expect(runner.can_pick?(build)).to be_truthy - end + it { is_expected.to be_truthy } it 'knows namespace id it is assigned to' do expect(runner.namespace_ids).to eq [group.id] @@ -1220,14 +1270,6 @@ RSpec.describe Ci::Runner do runner.pick_build!(build) end end - - context 'build picking improvement' do - it 'does not check if the build is assignable to a runner' do - expect(runner).not_to receive(:can_pick?) - - runner.pick_build!(build) - end - end end describe 'project runner without projects is destroyable' do @@ -1259,6 +1301,20 @@ RSpec.describe Ci::Runner do expect(runners).to eq([runner2, runner1]) end + + it 'supports ordering by the token expiration' do + runner1 = create(:ci_runner) + runner1.update!(token_expires_at: 1.year.from_now) + runner2 = create(:ci_runner) + runner3 = create(:ci_runner) + runner3.update!(token_expires_at: 1.month.from_now) + + runners = described_class.order_by('token_expires_at_asc') + expect(runners).to eq([runner3, runner1, runner2]) + + runners = described_class.order_by('token_expires_at_desc') + expect(runners).to eq([runner2, runner1, runner3]) + end end describe '.runner_matchers' do @@ -1386,47 +1442,6 @@ RSpec.describe Ci::Runner do it { is_expected.to eq(contacted_at_stored) } end - describe '.legacy_belonging_to_group' do - shared_examples 'returns group runners' do - it 'returns the specific group runner' do - group = create(:group) - runner = create(:ci_runner, :group, groups: [group]) - unrelated_group = create(:group) - create(:ci_runner, :group, groups: [unrelated_group]) - - expect(described_class.legacy_belonging_to_group(group.id)).to contain_exactly(runner) - end - - context 'runner belonging to parent group' do - let_it_be(:parent_group) { create(:group) } - let_it_be(:parent_runner) { create(:ci_runner, :group, groups: [parent_group]) } - let_it_be(:group) { create(:group, parent: parent_group) } - - context 'when include_parent option is passed' do - it 'returns the group runner from the parent group' do - expect(described_class.legacy_belonging_to_group(group.id, include_ancestors: true)).to contain_exactly(parent_runner) - end - end - - context 'when include_parent option is not passed' do - it 'does not return the group runner from the parent group' do - expect(described_class.legacy_belonging_to_group(group.id)).to be_empty - end - end - end - end - - it_behaves_like 'returns group runners' - - context 'when feature flag :linear_runner_ancestor_scopes is disabled' do - before do - stub_feature_flags(linear_runner_ancestor_scopes: false) - end - - it_behaves_like 'returns group runners' - end - end - describe '.belonging_to_group' do it 'returns the specific group runner' do group = create(:group) @@ -1470,4 +1485,182 @@ RSpec.describe Ci::Runner do ) end end + + describe '#token_expires_at', :freeze_time do + shared_examples 'expiring token' do |interval:| + it 'expires' do + expect(runner.token_expires_at).to eq(interval.from_now) + end + end + + shared_examples 'non-expiring token' do + it 'does not expire' do + expect(runner.token_expires_at).to be_nil + end + end + + context 'no expiration' do + let(:runner) { create(:ci_runner) } + + it_behaves_like 'non-expiring token' + end + + context 'system-wide shared expiration' do + before do + stub_application_setting(runner_token_expiration_interval: 5.days.to_i) + end + + let(:runner) { create(:ci_runner) } + + it_behaves_like 'expiring token', interval: 5.days + end + + context 'system-wide group expiration' do + before do + stub_application_setting(group_runner_token_expiration_interval: 5.days.to_i) + end + + let(:runner) { create(:ci_runner) } + + it_behaves_like 'non-expiring token' + end + + context 'system-wide project expiration' do + before do + stub_application_setting(project_runner_token_expiration_interval: 5.days.to_i) + end + + let(:runner) { create(:ci_runner) } + + it_behaves_like 'non-expiring token' + end + + context 'group expiration' do + let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 6.days.to_i) } + let(:group) { create(:group, namespace_settings: group_settings) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 6.days + end + + context 'human-readable group expiration' do + let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval_human_readable: '7 days') } + let(:group) { create(:group, namespace_settings: group_settings) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 7.days + end + + context 'project expiration' do + let(:project) { create(:project, runner_token_expiration_interval: 4.days.to_i).tap(&:save!) } + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 4.days + end + + context 'human-readable project expiration' do + let(:project) { create(:project, runner_token_expiration_interval_human_readable: '5 days').tap(&:save!) } + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 5.days + end + + context 'multiple projects' do + let(:project1) { create(:project, runner_token_expiration_interval: 8.days.to_i).tap(&:save!) } + let(:project2) { create(:project, runner_token_expiration_interval: 7.days.to_i).tap(&:save!) } + let(:project3) { create(:project, runner_token_expiration_interval: 9.days.to_i).tap(&:save!) } + let(:runner) { create(:ci_runner, :project, projects: [project1, project2, project3]) } + + it_behaves_like 'expiring token', interval: 7.days + end + + context 'with project runner token expiring' do + let_it_be(:project) { create(:project, runner_token_expiration_interval: 4.days.to_i).tap(&:save!) } + + context 'project overrides system' do + before do + stub_application_setting(project_runner_token_expiration_interval: 5.days.to_i) + end + + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 4.days + end + + context 'system overrides project' do + before do + stub_application_setting(project_runner_token_expiration_interval: 3.days.to_i) + end + + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 3.days + end + end + + context 'with group runner token expiring' do + let_it_be(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 4.days.to_i) } + let_it_be(:group) { create(:group, namespace_settings: group_settings) } + + context 'group overrides system' do + before do + stub_application_setting(group_runner_token_expiration_interval: 5.days.to_i) + end + + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 4.days + end + + context 'system overrides group' do + before do + stub_application_setting(group_runner_token_expiration_interval: 3.days.to_i) + end + + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 3.days + end + end + + context "with group's project runner token expiring" do + let_it_be(:parent_group_settings) { create(:namespace_settings, subgroup_runner_token_expiration_interval: 2.days.to_i) } + let_it_be(:parent_group) { create(:group, namespace_settings: parent_group_settings) } + + context 'parent group overrides subgroup' do + let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 3.days.to_i) } + let(:group) { create(:group, parent: parent_group, namespace_settings: group_settings) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 2.days + end + + context 'subgroup overrides parent group' do + let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 1.day.to_i) } + let(:group) { create(:group, parent: parent_group, namespace_settings: group_settings) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } + + it_behaves_like 'expiring token', interval: 1.day + end + end + + context "with group's project runner token expiring" do + let_it_be(:group_settings) { create(:namespace_settings, project_runner_token_expiration_interval: 2.days.to_i) } + let_it_be(:group) { create(:group, namespace_settings: group_settings) } + + context 'group overrides project' do + let(:project) { create(:project, group: group, runner_token_expiration_interval: 3.days.to_i).tap(&:save!) } + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 2.days + end + + context 'project overrides group' do + let(:project) { create(:project, group: group, runner_token_expiration_interval: 1.day.to_i).tap(&:save!) } + let(:runner) { create(:ci_runner, :project, projects: [project]) } + + it_behaves_like 'expiring token', interval: 1.day + end + end + end end diff --git a/spec/models/ci/sources/pipeline_spec.rb b/spec/models/ci/sources/pipeline_spec.rb index ccf3140650b..73f7cfa739f 100644 --- a/spec/models/ci/sources/pipeline_spec.rb +++ b/spec/models/ci/sources/pipeline_spec.rb @@ -17,4 +17,18 @@ RSpec.describe Ci::Sources::Pipeline do it { is_expected.to validate_presence_of(:source_project) } it { is_expected.to validate_presence_of(:source_job) } it { is_expected.to validate_presence_of(:source_pipeline) } + + context 'loose foreign key on ci_sources_pipelines.source_project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_sources_pipeline, source_project: parent) } + end + end + + context 'loose foreign key on ci_sources_pipelines.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_sources_pipeline, project: parent) } + end + end end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index 2b6f22e68f1..b91348eb408 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -362,4 +362,11 @@ RSpec.describe Ci::Stage, :models do end it_behaves_like 'manual playable stage', :ci_stage_entity + + context 'loose foreign key on ci_stages.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_stage_entity, project: parent) } + end + end end diff --git a/spec/models/ci/trigger_spec.rb b/spec/models/ci/trigger_spec.rb index c254279a32f..4ac8720780c 100644 --- a/spec/models/ci/trigger_spec.rb +++ b/spec/models/ci/trigger_spec.rb @@ -59,6 +59,20 @@ RSpec.describe Ci::Trigger do end it_behaves_like 'includes Limitable concern' do - subject { build(:ci_trigger, owner: project.owner, project: project) } + subject { build(:ci_trigger, owner: project.first_owner, project: project) } + end + + context 'loose foreign key on ci_triggers.owner_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:user) } + let!(:model) { create(:ci_trigger, owner: parent) } + end + end + + context 'loose foreign key on ci_triggers.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_trigger, project: parent) } + end end end diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 93a24ba9157..29ca088ee04 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -44,4 +44,11 @@ RSpec.describe Ci::Variable do end end end + + context 'loose foreign key on ci_variables.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_variable, project: parent) } + end + end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 2176eea75bc..7c67b9a3d63 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -486,7 +486,7 @@ eos it 'uses the CachedMarkdownField cache instead of the Mentionable cache', :use_clean_rails_redis_caching do expect(commit.title_html).not_to be_present - commit.all_references(project.owner).all + commit.all_references(project.first_owner).all expect(commit.title_html).to be_present expect(Rails.cache.read("banzai/commit:#{commit.id}/safe_message/single_line")).to be_nil @@ -748,29 +748,23 @@ eos describe '#work_in_progress?' do [ - 'squash! ', 'fixup! ', 'wip: ', 'WIP: ', '[WIP] ', + 'squash! ', 'fixup! ', 'draft: ', '[Draft] ', '(draft) ', 'Draft: ' - ].each do |wip_prefix| - it "detects the '#{wip_prefix}' prefix" do - commit.message = "#{wip_prefix}#{commit.message}" + ].each do |draft_prefix| + it "detects the '#{draft_prefix}' prefix" do + commit.message = "#{draft_prefix}#{commit.message}" expect(commit).to be_work_in_progress end end - it "detects WIP for a commit just saying 'wip'" do - commit.message = "wip" - - expect(commit).to be_work_in_progress - end - it "does not detect WIP for a commit just saying 'draft'" do commit.message = "draft" expect(commit).not_to be_work_in_progress end - ["FIXUP!", "Draft - ", "Wipeout"].each do |draft_prefix| + ["FIXUP!", "Draft - ", "Wipeout", "WIP: ", "[WIP] ", "wip: "].each do |draft_prefix| it "doesn't detect '#{draft_prefix}' at the start of the title as a draft" do commit.message = "#{draft_prefix} #{commit.message}" diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index d5e74d36b58..86ee159b97e 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -987,4 +987,11 @@ RSpec.describe CommitStatus do commit_status.expire_etag_cache! end end + + context 'loose foreign key on ci_builds.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:ci_build, project: parent) } + end + end end diff --git a/spec/models/concerns/after_commit_queue_spec.rb b/spec/models/concerns/after_commit_queue_spec.rb index 40cddde333e..8f091081dce 100644 --- a/spec/models/concerns/after_commit_queue_spec.rb +++ b/spec/models/concerns/after_commit_queue_spec.rb @@ -75,7 +75,7 @@ RSpec.describe AfterCommitQueue do skip_if_multiple_databases_not_setup table_sql = <<~SQL - CREATE TABLE _test_ci_after_commit_queue ( + CREATE TABLE _test_gitlab_ci_after_commit_queue ( id serial NOT NULL PRIMARY KEY); SQL @@ -84,7 +84,7 @@ RSpec.describe AfterCommitQueue do let(:ci_klass) do Class.new(Ci::ApplicationRecord) do - self.table_name = '_test_ci_after_commit_queue' + self.table_name = '_test_gitlab_ci_after_commit_queue' include AfterCommitQueue diff --git a/spec/models/concerns/ci/has_variable_spec.rb b/spec/models/concerns/ci/has_variable_spec.rb index e917ec6b802..bf699119a37 100644 --- a/spec/models/concerns/ci/has_variable_spec.rb +++ b/spec/models/concerns/ci/has_variable_spec.rb @@ -68,9 +68,48 @@ RSpec.describe Ci::HasVariable do end describe '#to_runner_variable' do + let_it_be(:ci_variable) { create(:ci_variable) } + + subject { ci_variable } + it 'returns a hash for the runner' do expect(subject.to_runner_variable) .to include(key: subject.key, value: subject.value, public: false) end + + context 'with RequestStore enabled', :request_store do + let(:expected) do + { + file: false, + key: subject.key, + value: subject.value, + public: false, + masked: false + } + end + + it 'decrypts once' do + expect(OpenSSL::PKCS5).to receive(:pbkdf2_hmac).once.and_call_original + + 2.times { expect(subject.reload.to_runner_variable).to eq(expected) } + end + + it 'does not cache similar keys', :aggregate_failures do + group_var = create(:ci_group_variable, key: subject.key, value: 'group') + project_var = create(:ci_variable, key: subject.key, value: 'project') + + expect(subject.to_runner_variable).to include(key: subject.key, value: subject.value) + expect(group_var.to_runner_variable).to include(key: subject.key, value: 'group') + expect(project_var.to_runner_variable).to include(key: subject.key, value: 'project') + end + + it 'does not cache unpersisted values' do + new_variable = Ci::Variable.new(key: SecureRandom.hex, value: "12345") + old_value = new_variable.to_runner_variable + new_variable.value = '98765' + + expect(new_variable.to_runner_variable).not_to eq(old_value) + end + end end end diff --git a/spec/models/concerns/cross_database_modification_spec.rb b/spec/models/concerns/cross_database_modification_spec.rb new file mode 100644 index 00000000000..72544536953 --- /dev/null +++ b/spec/models/concerns/cross_database_modification_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe CrossDatabaseModification do + describe '.transaction' do + context 'feature flag disabled' do + before do + stub_feature_flags(track_gitlab_schema_in_current_transaction: false) + end + + it 'does not add to gitlab_transactions_stack' do + ApplicationRecord.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + end + end + + context 'feature flag is not yet setup' do + before do + allow(Feature::FlipperFeature).to receive(:table_exists?).and_raise(ActiveRecord::NoDatabaseError) + end + + it 'does not add to gitlab_transactions_stack' do + ApplicationRecord.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + end + end + + it 'adds the current gitlab schema to gitlab_transactions_stack', :aggregate_failures do + ApplicationRecord.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_main) + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + Ci::ApplicationRecord.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_ci) + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + Project.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_main) + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + Ci::Pipeline.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_ci) + + Project.first + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + + ApplicationRecord.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_main) + + Ci::Pipeline.transaction do + expect(ApplicationRecord.gitlab_transactions_stack).to contain_exactly(:gitlab_main, :gitlab_ci) + + Project.first + end + end + + expect(ApplicationRecord.gitlab_transactions_stack).to be_empty + end + + it 'yields' do + expect { |block| ApplicationRecord.transaction(&block) }.to yield_control + end + end +end diff --git a/spec/models/concerns/has_environment_scope_spec.rb b/spec/models/concerns/has_environment_scope_spec.rb index 0cc997709c9..6e8394b6fa5 100644 --- a/spec/models/concerns/has_environment_scope_spec.rb +++ b/spec/models/concerns/has_environment_scope_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe HasEnvironmentScope do + let_it_be(:project) { create(:project) } + subject { build(:ci_variable) } it { is_expected.to allow_value('*').for(:environment_scope) } @@ -17,8 +19,6 @@ RSpec.describe HasEnvironmentScope do end describe '.on_environment' do - let(:project) { create(:project) } - it 'returns scoped objects' do variable1 = create(:ci_variable, project: project, environment_scope: '*') variable2 = create(:ci_variable, project: project, environment_scope: 'product/*') @@ -63,4 +63,32 @@ RSpec.describe HasEnvironmentScope do end end end + + describe '.for_environment' do + subject { project.variables.for_environment(environment) } + + let_it_be(:variable1) do + create(:ci_variable, project: project, environment_scope: '*') + end + + let_it_be(:variable2) do + create(:ci_variable, project: project, environment_scope: 'production/*') + end + + let_it_be(:variable3) do + create(:ci_variable, project: project, environment_scope: 'staging/*') + end + + context 'when the environment is present' do + let(:environment) { 'production/canary-1' } + + it { is_expected.to eq([variable1, variable2]) } + end + + context 'when the environment is nil' do + let(:environment) {} + + it { is_expected.to eq([variable1]) } + end + end end diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index e9c3d1dc646..832d5b44e5d 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -935,6 +935,14 @@ RSpec.describe Issuable do subject { issuable.supports_escalation? } it { is_expected.to eq(supports_escalation) } + + context 'with feature disabled' do + before do + stub_feature_flags(incident_escalations: false) + end + + it { is_expected.to eq(false) } + end end end diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb index fc154738f11..7e08f47fb5a 100644 --- a/spec/models/concerns/resolvable_discussion_spec.rb +++ b/spec/models/concerns/resolvable_discussion_spec.rb @@ -584,4 +584,14 @@ RSpec.describe Discussion, ResolvableDiscussion do expect(subject.last_resolved_note).to eq(second_note) end end + + describe '#clear_memoized_values' do + it 'resets the memoized values' do + described_class.memoized_values.each do |memo| + subject.instance_variable_set("@#{memo}", 'memoized') + expect { subject.clear_memoized_values }.to change { subject.instance_variable_get("@#{memo}") } + .from('memoized').to(nil) + end + end + end end diff --git a/spec/models/concerns/taskable_spec.rb b/spec/models/concerns/taskable_spec.rb new file mode 100644 index 00000000000..6b41174a046 --- /dev/null +++ b/spec/models/concerns/taskable_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Taskable do + using RSpec::Parameterized::TableSyntax + + describe '.get_tasks' do + let(:description) do + <<~MARKDOWN + Any text before the list + - [ ] First item + - [x] Second item + * [x] First item + * [ ] Second item + MARKDOWN + end + + let(:expected_result) do + [ + TaskList::Item.new('- [ ]', 'First item'), + TaskList::Item.new('- [x]', 'Second item'), + TaskList::Item.new('* [x]', 'First item'), + TaskList::Item.new('* [ ]', 'Second item') + ] + end + + subject { described_class.get_tasks(description) } + + it { is_expected.to match(expected_result) } + end + + describe '#task_list_items' do + where(issuable_type: [:issue, :merge_request]) + + with_them do + let(:issuable) { build(issuable_type, description: description) } + + subject(:result) { issuable.task_list_items } + + context 'when description is present' do + let(:description) { 'markdown' } + + it 'gets tasks from markdown' do + expect(described_class).to receive(:get_tasks) + + result + end + end + + context 'when description is blank' do + let(:description) { '' } + + it 'returns empty array' do + expect(result).to be_empty + end + + it 'does not try to get tasks from markdown' do + expect(described_class).not_to receive(:get_tasks) + + result + end + end + end + end +end diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index 4bdb3e0a32a..2e82a12a61a 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -289,4 +289,142 @@ RSpec.describe Ci::Build, 'TokenAuthenticatable' do expect(build.read_attribute('token')).to be_nil end end + + describe '#token_with_expiration' do + describe '#expirable?' do + subject { build.token_with_expiration.expirable? } + + it { is_expected.to eq(false) } + end + end +end + +RSpec.describe Ci::Runner, 'TokenAuthenticatable', :freeze_time do + let_it_be(:non_expirable_runner) { create(:ci_runner) } + let_it_be(:non_expired_runner) { create(:ci_runner).tap { |r| r.update!(token_expires_at: 5.seconds.from_now) } } + let_it_be(:expired_runner) { create(:ci_runner).tap { |r| r.update!(token_expires_at: 5.seconds.ago) } } + + describe '#token_expired?' do + subject { runner.token_expired? } + + context 'when enforce_runner_token_expires_at feature flag is disabled' do + before do + stub_feature_flags(enforce_runner_token_expires_at: false) + end + + context 'when runner has no token expiration' do + let(:runner) { non_expirable_runner } + + it { is_expected.to eq(false) } + end + + context 'when runner token is not expired' do + let(:runner) { non_expired_runner } + + it { is_expected.to eq(false) } + end + + context 'when runner token is expired' do + let(:runner) { expired_runner } + + it { is_expected.to eq(false) } + end + end + + context 'when enforce_runner_token_expires_at feature flag is enabled' do + before do + stub_feature_flags(enforce_runner_token_expires_at: true) + end + + context 'when runner has no token expiration' do + let(:runner) { non_expirable_runner } + + it { is_expected.to eq(false) } + end + + context 'when runner token is not expired' do + let(:runner) { non_expired_runner } + + it { is_expected.to eq(false) } + end + + context 'when runner token is expired' do + let(:runner) { expired_runner } + + it { is_expected.to eq(true) } + end + end + end + + describe '#token_with_expiration' do + describe '#token' do + subject { non_expired_runner.token_with_expiration.token } + + it { is_expected.to eq(non_expired_runner.token) } + end + + describe '#token_expires_at' do + subject { non_expired_runner.token_with_expiration.token_expires_at } + + it { is_expected.to eq(non_expired_runner.token_expires_at) } + end + + describe '#expirable?' do + subject { non_expired_runner.token_with_expiration.expirable? } + + it { is_expected.to eq(true) } + end + end + + describe '.find_by_token' do + subject { Ci::Runner.find_by_token(runner.token) } + + context 'when enforce_runner_token_expires_at feature flag is disabled' do + before do + stub_feature_flags(enforce_runner_token_expires_at: false) + end + + context 'when runner has no token expiration' do + let(:runner) { non_expirable_runner } + + it { is_expected.to eq(non_expirable_runner) } + end + + context 'when runner token is not expired' do + let(:runner) { non_expired_runner } + + it { is_expected.to eq(non_expired_runner) } + end + + context 'when runner token is expired' do + let(:runner) { expired_runner } + + it { is_expected.to eq(expired_runner) } + end + end + + context 'when enforce_runner_token_expires_at feature flag is enabled' do + before do + stub_feature_flags(enforce_runner_token_expires_at: true) + end + + context 'when runner has no token expiration' do + let(:runner) { non_expirable_runner } + + it { is_expected.to eq(non_expirable_runner) } + end + + context 'when runner token is not expired' do + let(:runner) { non_expired_runner } + + it { is_expected.to eq(non_expired_runner) } + end + + context 'when runner token is expired' do + let(:runner) { expired_runner } + + it { is_expected.to be_nil } + end + end + end end diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb index b311e302a31..1772fd0ff95 100644 --- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb +++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb @@ -23,6 +23,8 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do let(:options) { { encrypted: :required } } it 'finds the encrypted resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) allow(model).to receive(:find_by) .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv]) .and_return('encrypted resource') @@ -36,6 +38,8 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do let(:options) { { encrypted: :optional } } it 'finds the encrypted resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) allow(model).to receive(:find_by) .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv]) .and_return('encrypted resource') @@ -49,6 +53,8 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do .to receive(:find_token_authenticatable) .and_return('plaintext resource') + allow(model).to receive(:where) + .and_return(model) allow(model).to receive(:find_by) .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv]) .and_return(nil) @@ -62,6 +68,8 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do let(:options) { { encrypted: :migrating } } it 'finds the cleartext resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) allow(model).to receive(:find_by) .with('some_field' => 'my-value') .and_return('cleartext resource') @@ -71,6 +79,8 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do end it 'returns nil if resource cannot be found' do + allow(model).to receive(:where) + .and_return(model) allow(model).to receive(:find_by) .with('some_field' => 'my-value') .and_return(nil) diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 8f7c13d7ae6..7c0ae51223b 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe ContainerRepository do +RSpec.describe ContainerRepository, :aggregate_failures do using RSpec::Parameterized::TableSyntax let(:group) { create(:group, name: 'group') } @@ -36,7 +36,467 @@ RSpec.describe ContainerRepository do describe 'validations' do it { is_expected.to validate_presence_of(:migration_retries_count) } it { is_expected.to validate_numericality_of(:migration_retries_count).is_greater_than_or_equal_to(0) } - it { is_expected.to validate_presence_of(:migration_state) } + + it { is_expected.to validate_inclusion_of(:migration_aborted_in_state).in_array(described_class::ABORTABLE_MIGRATION_STATES) } + it { is_expected.to allow_value(nil).for(:migration_aborted_in_state) } + + context 'migration_state' do + it { is_expected.to validate_presence_of(:migration_state) } + it { is_expected.to validate_inclusion_of(:migration_state).in_array(described_class::MIGRATION_STATES) } + + describe 'pre_importing' do + it 'validates expected attributes' do + expect(build(:container_repository, migration_state: 'pre_importing')).to be_invalid + expect(build(:container_repository, :pre_importing)).to be_valid + end + end + + describe 'pre_import_done' do + it 'validates expected attributes' do + expect(build(:container_repository, migration_state: 'pre_import_done')).to be_invalid + expect(build(:container_repository, :pre_import_done)).to be_valid + end + end + + describe 'importing' do + it 'validates expected attributes' do + expect(build(:container_repository, migration_state: 'importing')).to be_invalid + expect(build(:container_repository, :importing)).to be_valid + end + end + + describe 'import_skipped' do + it 'validates expected attributes' do + expect(build(:container_repository, migration_state: 'import_skipped')).to be_invalid + expect(build(:container_repository, :import_skipped)).to be_valid + end + end + + describe 'import_aborted' do + it 'validates expected attributes' do + expect(build(:container_repository, migration_state: 'import_aborted')).to be_invalid + expect(build(:container_repository, :import_aborted)).to be_valid + end + end + end + end + + context ':migration_state state_machine' do + shared_examples 'no action when feature flag is disabled' do + context 'feature flag disabled' do + before do + stub_feature_flags(container_registry_migration_phase2_enabled: false) + end + + it { is_expected.to eq(false) } + end + end + + shared_examples 'transitioning to pre_importing', skip_pre_import_success: true do + before do + repository.update_column(:migration_pre_import_done_at, Time.zone.now) + end + + it_behaves_like 'no action when feature flag is disabled' + + context 'successful pre_import request' do + it 'sets migration_pre_import_started_at and resets migration_pre_import_done_at' do + expect(repository).to receive(:migration_pre_import).and_return(:ok) + + expect { subject }.to change { repository.reload.migration_pre_import_started_at } + .and change { repository.migration_pre_import_done_at }.to(nil) + + expect(repository).to be_pre_importing + end + end + + context 'failed pre_import request' do + it 'sets migration_pre_import_started_at and resets migration_pre_import_done_at' do + expect(repository).to receive(:migration_pre_import).and_return(:error) + + expect { subject }.to change { repository.reload.migration_pre_import_started_at } + .and change { repository.migration_aborted_at } + .and change { repository.migration_pre_import_done_at }.to(nil) + + expect(repository.migration_aborted_in_state).to eq('pre_importing') + expect(repository).to be_import_aborted + end + end + end + + shared_examples 'transitioning to importing', skip_import_success: true do + before do + repository.update_columns(migration_import_done_at: Time.zone.now) + end + + context 'successful import request' do + it 'sets migration_import_started_at and resets migration_import_done_at' do + expect(repository).to receive(:migration_import).and_return(:ok) + + expect { subject }.to change { repository.reload.migration_import_started_at } + .and change { repository.migration_import_done_at }.to(nil) + + expect(repository).to be_importing + end + end + + context 'failed import request' do + it 'sets migration_import_started_at and resets migration_import_done_at' do + expect(repository).to receive(:migration_import).and_return(:error) + + expect { subject }.to change { repository.reload.migration_import_started_at } + .and change { repository.migration_aborted_at } + + expect(repository.migration_aborted_in_state).to eq('importing') + expect(repository).to be_import_aborted + end + end + end + + shared_examples 'transitioning out of import_aborted' do + it 'resets migration_aborted_at and migration_aborted_in_state' do + expect { subject }.to change { repository.reload.migration_aborted_in_state }.to(nil) + .and change { repository.migration_aborted_at }.to(nil) + end + end + + shared_examples 'transitioning from allowed states' do |allowed_states| + described_class::MIGRATION_STATES.each do |state| + result = allowed_states.include?(state) + + context "when transitioning from #{state}" do + let(:repository) { create(:container_repository, state.to_sym) } + + it "returns #{result}" do + expect(subject).to eq(result) + end + end + end + end + + shared_examples 'queueing the next import' do + it 'starts the worker' do + expect(::ContainerRegistry::Migration::EnqueuerWorker).to receive(:perform_async) + + subject + end + end + + describe '#start_pre_import' do + let_it_be_with_reload(:repository) { create(:container_repository) } + + subject { repository.start_pre_import } + + before do |example| + unless example.metadata[:skip_pre_import_success] + allow(repository).to receive(:migration_pre_import).and_return(:ok) + end + end + + it_behaves_like 'transitioning from allowed states', %w[default] + it_behaves_like 'transitioning to pre_importing' + end + + describe '#retry_pre_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :import_aborted) } + + subject { repository.retry_pre_import } + + before do |example| + unless example.metadata[:skip_pre_import_success] + allow(repository).to receive(:migration_pre_import).and_return(:ok) + end + end + + it_behaves_like 'transitioning from allowed states', %w[import_aborted] + it_behaves_like 'transitioning to pre_importing' + it_behaves_like 'transitioning out of import_aborted' + end + + describe '#finish_pre_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :pre_importing) } + + subject { repository.finish_pre_import } + + it_behaves_like 'transitioning from allowed states', %w[pre_importing import_aborted] + + it 'sets migration_pre_import_done_at' do + expect { subject }.to change { repository.reload.migration_pre_import_done_at } + + expect(repository).to be_pre_import_done + end + end + + describe '#start_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :pre_import_done) } + + subject { repository.start_import } + + before do |example| + unless example.metadata[:skip_import_success] + allow(repository).to receive(:migration_import).and_return(:ok) + end + end + + it_behaves_like 'transitioning from allowed states', %w[pre_import_done] + it_behaves_like 'transitioning to importing' + end + + describe '#retry_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :import_aborted) } + + subject { repository.retry_import } + + before do |example| + unless example.metadata[:skip_import_success] + allow(repository).to receive(:migration_import).and_return(:ok) + end + end + + it_behaves_like 'transitioning from allowed states', %w[import_aborted] + it_behaves_like 'transitioning to importing' + it_behaves_like 'no action when feature flag is disabled' + end + + describe '#finish_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :importing) } + + subject { repository.finish_import } + + it_behaves_like 'transitioning from allowed states', %w[importing import_aborted] + it_behaves_like 'queueing the next import' + + it 'sets migration_import_done_at and queues the next import' do + expect { subject }.to change { repository.reload.migration_import_done_at } + + expect(repository).to be_import_done + end + end + + describe '#already_migrated' do + let_it_be_with_reload(:repository) { create(:container_repository) } + + subject { repository.already_migrated } + + it_behaves_like 'transitioning from allowed states', %w[default] + + it 'sets migration_import_done_at' do + subject + + expect(repository).to be_import_done + end + end + + describe '#abort_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :importing) } + + subject { repository.abort_import } + + it_behaves_like 'transitioning from allowed states', ContainerRepository::ABORTABLE_MIGRATION_STATES + it_behaves_like 'queueing the next import' + + it 'sets migration_aborted_at and migration_aborted_at, increments the retry count, and queues the next import' do + expect { subject }.to change { repository.migration_aborted_at } + .and change { repository.reload.migration_retries_count }.by(1) + + expect(repository.migration_aborted_in_state).to eq('importing') + expect(repository).to be_import_aborted + end + end + + describe '#skip_import' do + let_it_be_with_reload(:repository) { create(:container_repository) } + + subject { repository.skip_import(reason: :too_many_retries) } + + it_behaves_like 'transitioning from allowed states', ContainerRepository::ABORTABLE_MIGRATION_STATES + + it 'sets migration_skipped_at and migration_skipped_reason' do + expect { subject }.to change { repository.reload.migration_skipped_at } + + expect(repository.migration_skipped_reason).to eq('too_many_retries') + expect(repository).to be_import_skipped + end + + it 'raises and error if a reason is not given' do + expect { repository.skip_import }.to raise_error(ArgumentError) + end + end + + describe '#finish_pre_import_and_start_import' do + let_it_be_with_reload(:repository) { create(:container_repository, :pre_importing) } + + subject { repository.finish_pre_import_and_start_import } + + before do |example| + unless example.metadata[:skip_import_success] + allow(repository).to receive(:migration_import).and_return(:ok) + end + end + + it_behaves_like 'transitioning from allowed states', %w[pre_importing import_aborted] + it_behaves_like 'transitioning to importing' + end + end + + context 'when triggering registry API requests' do + let(:repository_state) { nil } + let(:repository) { create(:container_repository, repository_state) } + + shared_examples 'a state machine configured with use_transactions: false' do + it 'executes the registry API request outside of a transaction', :delete do + expect(repository).to receive(:save).and_call_original do + expect(ApplicationRecord.connection.transaction_open?).to be true + end + + expect(repository).to receive(:try_import) do + expect(ApplicationRecord.connection.transaction_open?).to be false + end + + subject + end + end + + context 'when responding to a start_pre_import event' do + subject { repository.start_pre_import } + + it_behaves_like 'a state machine configured with use_transactions: false' + end + + context 'when responding to a retry_pre_import event' do + let(:repository_state) { :import_aborted } + + subject { repository.retry_pre_import } + + it_behaves_like 'a state machine configured with use_transactions: false' + end + + context 'when responding to a start_import event' do + let(:repository_state) { :pre_import_done } + + subject { repository.start_import } + + it_behaves_like 'a state machine configured with use_transactions: false' + end + + context 'when responding to a retry_import event' do + let(:repository_state) { :import_aborted } + + subject { repository.retry_import } + + it_behaves_like 'a state machine configured with use_transactions: false' + end + end + + describe '#retry_aborted_migration' do + subject { repository.retry_aborted_migration } + + shared_examples 'no action' do + it 'does nothing' do + expect { subject }.not_to change { repository.reload.migration_state } + + expect(subject).to eq(nil) + end + end + + shared_examples 'retrying the pre_import' do + it 'retries the pre_import' do + expect(repository).to receive(:migration_pre_import).and_return(:ok) + + expect { subject }.to change { repository.reload.migration_state }.to('pre_importing') + end + end + + shared_examples 'retrying the import' do + it 'retries the import' do + expect(repository).to receive(:migration_import).and_return(:ok) + + expect { subject }.to change { repository.reload.migration_state }.to('importing') + end + end + + context 'when migration_state is not aborted' do + it_behaves_like 'no action' + end + + context 'when migration_state is aborted' do + before do + repository.abort_import + + allow(repository.gitlab_api_client) + .to receive(:import_status).with(repository.path).and_return(client_response) + end + + context 'native response' do + let(:client_response) { 'native' } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::NativeImportError) + end + end + + context 'import_in_progress response' do + let(:client_response) { 'import_in_progress' } + + it_behaves_like 'no action' + end + + context 'import_complete response' do + let(:client_response) { 'import_complete' } + + it 'finishes the import' do + expect { subject }.to change { repository.reload.migration_state }.to('import_done') + end + end + + context 'import_failed response' do + let(:client_response) { 'import_failed' } + + it_behaves_like 'retrying the import' + end + + context 'pre_import_in_progress response' do + let(:client_response) { 'pre_import_in_progress' } + + it_behaves_like 'no action' + end + + context 'pre_import_complete response' do + let(:client_response) { 'pre_import_complete' } + + it 'finishes the pre_import and starts the import' do + expect(repository).to receive(:finish_pre_import).and_call_original + expect(repository).to receive(:migration_import).and_return(:ok) + + expect { subject }.to change { repository.reload.migration_state }.to('importing') + end + end + + context 'pre_import_failed response' do + let(:client_response) { 'pre_import_failed' } + + it_behaves_like 'retrying the pre_import' + end + + context 'error response' do + let(:client_response) { 'error' } + + context 'migration_pre_import_done_at is NULL' do + it_behaves_like 'retrying the pre_import' + end + + context 'migration_pre_import_done_at is not NULL' do + before do + repository.update_columns( + migration_pre_import_started_at: 5.minutes.ago, + migration_pre_import_done_at: Time.zone.now + ) + end + + it_behaves_like 'retrying the import' + end + end + end end describe '#tag' do @@ -209,6 +669,54 @@ RSpec.describe ContainerRepository do end end + context 'registry migration' do + shared_examples 'handling the migration step' do |step| + let(:client_response) { :foobar } + + before do + allow(repository.gitlab_api_client).to receive(:supports_gitlab_api?).and_return(true) + end + + it 'returns the same response as the client' do + expect(repository.gitlab_api_client) + .to receive(step).with(repository.path).and_return(client_response) + expect(subject).to eq(client_response) + end + + context 'when the gitlab_api feature is not supported' do + before do + allow(repository.gitlab_api_client).to receive(:supports_gitlab_api?).and_return(false) + end + + it 'returns :error' do + expect(repository.gitlab_api_client).not_to receive(step) + + expect(subject).to eq(:error) + end + end + + context 'too many imports' do + it 'raises an error when it receives too_many_imports as a response' do + expect(repository.gitlab_api_client) + .to receive(step).with(repository.path).and_return(:too_many_imports) + expect { subject }.to raise_error(described_class::TooManyImportsError) + end + end + end + + describe '#migration_pre_import' do + subject { repository.migration_pre_import } + + it_behaves_like 'handling the migration step', :pre_import_repository + end + + describe '#migration_import' do + subject { repository.migration_import } + + it_behaves_like 'handling the migration step', :import_repository + end + end + describe '.build_from_path' do let(:registry_path) do ContainerRegistry::Path.new(project.full_path + '/some/image') @@ -304,7 +812,7 @@ RSpec.describe ContainerRepository do let(:path) { ContainerRegistry::Path.new(project.full_path + '/some/image') } it 'does not throw validation errors and only creates one repository' do - expect { repository_creation_race(path) }.to change { ContainerRepository.count }.by(1) + expect { repository_creation_race(path) }.to change { described_class.count }.by(1) end it 'retrieves a persisted repository for all concurrent calls' do @@ -322,7 +830,7 @@ RSpec.describe ContainerRepository do Thread.new do true while wait_for_it - ::ContainerRepository.find_or_create_from_path(path) + described_class.find_or_create_from_path(path) end end wait_for_it = false @@ -330,6 +838,52 @@ RSpec.describe ContainerRepository do end end + describe '.find_by_path' do + let_it_be(:container_repository) { create(:container_repository) } + let_it_be(:repository_path) { container_repository.project.full_path } + + let(:path) { ContainerRegistry::Path.new(repository_path + '/' + container_repository.name) } + + subject { described_class.find_by_path(path) } + + context 'when repository exists' do + it 'finds the repository' do + expect(subject).to eq(container_repository) + end + end + + context 'when repository does not exist' do + let(:path) { ContainerRegistry::Path.new(repository_path + '/some/image') } + + it 'returns nil' do + expect(subject).to be_nil + end + end + end + + describe '.find_by_path!' do + let_it_be(:container_repository) { create(:container_repository) } + let_it_be(:repository_path) { container_repository.project.full_path } + + let(:path) { ContainerRegistry::Path.new(repository_path + '/' + container_repository.name) } + + subject { described_class.find_by_path!(path) } + + context 'when repository exists' do + it 'finds the repository' do + expect(subject).to eq(container_repository) + end + end + + context 'when repository does not exist' do + let(:path) { ContainerRegistry::Path.new(repository_path + '/some/image') } + + it 'raises an exception' do + expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end + describe '.build_root_repository' do let(:repository) do described_class.build_root_repository(project) @@ -412,6 +966,36 @@ RSpec.describe ContainerRepository do it { is_expected.to contain_exactly(repository1, repository2, repository4) } end + describe '.with_migration_import_started_at_nil_or_before' do + let_it_be(:repository1) { create(:container_repository, migration_import_started_at: 5.minutes.ago) } + let_it_be(:repository2) { create(:container_repository, migration_import_started_at: nil) } + let_it_be(:repository3) { create(:container_repository, migration_import_started_at: 10.minutes.ago) } + + subject { described_class.with_migration_import_started_at_nil_or_before(7.minutes.ago) } + + it { is_expected.to contain_exactly(repository2, repository3) } + end + + describe '.with_migration_pre_import_started_at_nil_or_before' do + let_it_be(:repository1) { create(:container_repository, migration_pre_import_started_at: 5.minutes.ago) } + let_it_be(:repository2) { create(:container_repository, migration_pre_import_started_at: nil) } + let_it_be(:repository3) { create(:container_repository, migration_pre_import_started_at: 10.minutes.ago) } + + subject { described_class.with_migration_pre_import_started_at_nil_or_before(7.minutes.ago) } + + it { is_expected.to contain_exactly(repository2, repository3) } + end + + describe '.with_migration_pre_import_done_at_nil_or_before' do + let_it_be(:repository1) { create(:container_repository, migration_pre_import_done_at: 5.minutes.ago) } + let_it_be(:repository2) { create(:container_repository, migration_pre_import_done_at: nil) } + let_it_be(:repository3) { create(:container_repository, migration_pre_import_done_at: 10.minutes.ago) } + + subject { described_class.with_migration_pre_import_done_at_nil_or_before(7.minutes.ago) } + + it { is_expected.to contain_exactly(repository2, repository3) } + end + describe '.with_stale_ongoing_cleanup' do let_it_be(:repository1) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 1.day.ago) } let_it_be(:repository2) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 25.minutes.ago) } @@ -458,6 +1042,97 @@ RSpec.describe ContainerRepository do it { is_expected.to eq([repository]) } end + describe '#migration_in_active_state?' do + subject { container_repository.migration_in_active_state? } + + described_class::MIGRATION_STATES.each do |state| + context "when in #{state} migration_state" do + let(:container_repository) { create(:container_repository, state.to_sym)} + + it { is_expected.to eq(state == 'importing' || state == 'pre_importing') } + end + end + end + + describe '#migration_importing?' do + subject { container_repository.migration_importing? } + + described_class::MIGRATION_STATES.each do |state| + context "when in #{state} migration_state" do + let(:container_repository) { create(:container_repository, state.to_sym)} + + it { is_expected.to eq(state == 'importing') } + end + end + end + + describe '#migration_pre_importing?' do + subject { container_repository.migration_pre_importing? } + + described_class::MIGRATION_STATES.each do |state| + context "when in #{state} migration_state" do + let(:container_repository) { create(:container_repository, state.to_sym)} + + it { is_expected.to eq(state == 'pre_importing') } + end + end + end + + describe '#try_import' do + let_it_be_with_reload(:container_repository) { create(:container_repository) } + + let(:response) { nil } + + subject do + container_repository.try_import do + container_repository.foo + end + end + + before do + allow(container_repository).to receive(:foo).and_return(response) + end + + context 'successful request' do + let(:response) { :ok } + + it { is_expected.to eq(true) } + end + + context 'TooManyImportsError' do + before do + stub_application_setting(container_registry_import_start_max_retries: 3) + allow(container_repository).to receive(:foo).and_raise(described_class::TooManyImportsError) + end + + it 'tries again exponentially and aborts the migration' do + expect(container_repository).to receive(:sleep).with(a_value_within(0.01).of(0.1)) + expect(container_repository).to receive(:sleep).with(a_value_within(0.01).of(0.2)) + expect(container_repository).to receive(:sleep).with(a_value_within(0.01).of(0.3)) + + expect(subject).to eq(false) + + expect(container_repository).to be_import_aborted + end + end + + context 'other response' do + let(:response) { :error } + + it 'aborts the migration' do + expect(subject).to eq(false) + + expect(container_repository).to be_import_aborted + end + end + + context 'with no block given' do + it 'raises an error' do + expect { container_repository.try_import }.to raise_error(ArgumentError) + end + end + end + context 'with repositories' do let_it_be_with_reload(:repository) { create(:container_repository, :cleanup_unscheduled) } let_it_be(:other_repository) { create(:container_repository, :cleanup_unscheduled) } @@ -509,5 +1184,87 @@ RSpec.describe ContainerRepository do it { is_expected.to eq([repository]) } end end + + describe '.recently_done_migration_step' do + let_it_be(:import_done_repository) { create(:container_repository, :import_done, migration_pre_import_done_at: 3.days.ago, migration_import_done_at: 2.days.ago) } + let_it_be(:import_aborted_repository) { create(:container_repository, :import_aborted, migration_pre_import_done_at: 5.days.ago, migration_aborted_at: 1.day.ago) } + let_it_be(:pre_import_done_repository) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 1.hour.ago) } + + subject { described_class.recently_done_migration_step } + + it 'returns completed imports by done_at date' do + expect(subject.to_a).to eq([pre_import_done_repository, import_aborted_repository, import_done_repository]) + end + end + + describe '.ready_for_import' do + include_context 'importable repositories' + + subject { described_class.ready_for_import } + + before do + stub_application_setting(container_registry_import_target_plan: project.namespace.actual_plan_name) + end + + it 'works' do + expect(subject).to contain_exactly(valid_container_repository, valid_container_repository2) + end + end + + describe '#last_import_step_done_at' do + let_it_be(:aborted_at) { Time.zone.now - 1.hour } + let_it_be(:pre_import_done_at) { Time.zone.now - 2.hours } + + subject { repository.last_import_step_done_at } + + before do + repository.update_columns( + migration_pre_import_done_at: pre_import_done_at, + migration_aborted_at: aborted_at + ) + end + + it { is_expected.to eq(aborted_at) } + end + end + + describe '#external_import_status' do + subject { repository.external_import_status } + + it 'returns the response from the client' do + expect(repository.gitlab_api_client).to receive(:import_status).with(repository.path).and_return('test') + + expect(subject).to eq('test') + end + end + + describe '.with_stale_migration' do + let_it_be(:repository) { create(:container_repository) } + let_it_be(:stale_pre_importing_old_timestamp) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 10.minutes.ago) } + let_it_be(:stale_pre_importing_nil_timestamp) { create(:container_repository, :pre_importing).tap { |r| r.update_column(:migration_pre_import_started_at, nil) } } + let_it_be(:stale_pre_importing_recent_timestamp) { create(:container_repository, :pre_importing, migration_pre_import_started_at: 2.minutes.ago) } + + let_it_be(:stale_pre_import_done_old_timestamp) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 10.minutes.ago) } + let_it_be(:stale_pre_import_done_nil_timestamp) { create(:container_repository, :pre_import_done).tap { |r| r.update_column(:migration_pre_import_done_at, nil) } } + let_it_be(:stale_pre_import_done_recent_timestamp) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 2.minutes.ago) } + + let_it_be(:stale_importing_old_timestamp) { create(:container_repository, :importing, migration_import_started_at: 10.minutes.ago) } + let_it_be(:stale_importing_nil_timestamp) { create(:container_repository, :importing).tap { |r| r.update_column(:migration_import_started_at, nil) } } + let_it_be(:stale_importing_recent_timestamp) { create(:container_repository, :importing, migration_import_started_at: 2.minutes.ago) } + + let(:stale_migrations) do + [ + stale_pre_importing_old_timestamp, + stale_pre_importing_nil_timestamp, + stale_pre_import_done_old_timestamp, + stale_pre_import_done_nil_timestamp, + stale_importing_old_timestamp, + stale_importing_nil_timestamp + ] + end + + subject { described_class.with_stale_migration(5.minutes.ago) } + + it { is_expected.to contain_exactly(*stale_migrations) } end end diff --git a/spec/models/customer_relations/contact_spec.rb b/spec/models/customer_relations/contact_spec.rb index 1225f9d089b..c7b0f1bd3d4 100644 --- a/spec/models/customer_relations/contact_spec.rb +++ b/spec/models/customer_relations/contact_spec.rb @@ -26,6 +26,18 @@ RSpec.describe CustomerRelations::Contact, type: :model do it_behaves_like 'an object with RFC3696 compliant email-formatted attributes', :email end + describe '.reference_prefix' do + it { expect(described_class.reference_prefix).to eq('[contact:') } + end + + describe '.reference_prefix_quoted' do + it { expect(described_class.reference_prefix_quoted).to eq('["contact:') } + end + + describe '.reference_postfix' do + it { expect(described_class.reference_postfix).to eq(']') } + end + describe '#unique_email_for_group_hierarchy' do let_it_be(:parent) { create(:group) } let_it_be(:group) { create(:group, parent: parent) } @@ -98,4 +110,31 @@ RSpec.describe CustomerRelations::Contact, type: :model do expect { described_class.find_ids_by_emails(group, Array(0..too_many_emails)) }.to raise_error(ArgumentError) end end + + describe '#self.exists_for_group?' do + let(:group) { create(:group) } + let(:subgroup) { create(:group, parent: group) } + + context 'with no contacts in group or parent' do + it 'returns false' do + expect(described_class.exists_for_group?(subgroup)).to be_falsey + end + end + + context 'with contacts in group' do + it 'returns true' do + create(:contact, group: subgroup) + + expect(described_class.exists_for_group?(subgroup)).to be_truthy + end + end + + context 'with contacts in parent' do + it 'returns true' do + create(:contact, group: group) + + expect(described_class.exists_for_group?(subgroup)).to be_truthy + end + end + end end diff --git a/spec/models/customer_relations/issue_contact_spec.rb b/spec/models/customer_relations/issue_contact_spec.rb index c6373fddbfb..39da0b64ea0 100644 --- a/spec/models/customer_relations/issue_contact_spec.rb +++ b/spec/models/customer_relations/issue_contact_spec.rb @@ -80,4 +80,12 @@ RSpec.describe CustomerRelations::IssueContact do expect { described_class.find_contact_ids_by_emails(issue.id, Array(0..too_many_emails)) }.to raise_error(ArgumentError) end end + + describe '.delete_for_project' do + let_it_be(:issue_contacts) { create_list(:issue_customer_relations_contact, 3, :for_issue, issue: create(:issue, project: project)) } + + it 'destroys all issue_contacts for project' do + expect { described_class.delete_for_project(project.id) }.to change { described_class.count }.by(-3) + end + end end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 29b37ef7371..47c246d12cc 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -369,38 +369,6 @@ RSpec.describe Deployment do end end - describe '#finished_at' do - subject { deployment.finished_at } - - context 'when deployment status is created' do - let(:deployment) { create(:deployment) } - - it { is_expected.to be_nil } - end - - context 'when deployment status is success' do - let(:deployment) { create(:deployment, :success) } - - it { is_expected.to eq(deployment.read_attribute(:finished_at)) } - end - - context 'when deployment status is success' do - let(:deployment) { create(:deployment, :success, finished_at: nil) } - - before do - deployment.update_column(:finished_at, nil) - end - - it { is_expected.to eq(deployment.read_attribute(:created_at)) } - end - - context 'when deployment status is running' do - let(:deployment) { create(:deployment, :running) } - - it { is_expected.to be_nil } - end - end - describe '#deployed_at' do subject { deployment.deployed_at } @@ -615,7 +583,7 @@ RSpec.describe Deployment do it 'returns false' do commit = project.commit('feature') - expect(deployment.includes_commit?(commit)).to be false + expect(deployment.includes_commit?(commit.id)).to be false end end @@ -623,7 +591,7 @@ RSpec.describe Deployment do it 'returns true' do commit = project.commit - expect(deployment.includes_commit?(commit)).to be true + expect(deployment.includes_commit?(commit.id)).to be true end end @@ -632,7 +600,7 @@ RSpec.describe Deployment do deployment.update!(sha: Gitlab::Git::BLANK_SHA) commit = project.commit - expect(deployment.includes_commit?(commit)).to be false + expect(deployment.includes_commit?(commit.id)).to be false end end end diff --git a/spec/models/design_management/design_action_spec.rb b/spec/models/design_management/design_action_spec.rb index 958b1dd9124..4d60ef77025 100644 --- a/spec/models/design_management/design_action_spec.rb +++ b/spec/models/design_management/design_action_spec.rb @@ -46,7 +46,7 @@ RSpec.describe DesignManagement::DesignAction do describe '#gitaly_action' do let(:path) { 'some/path/somewhere' } - let(:design) { OpenStruct.new(full_path: path) } + let(:design) { double('path', full_path: path) } subject { described_class.new(design, action, content) } @@ -75,7 +75,7 @@ RSpec.describe DesignManagement::DesignAction do describe '#issue_id' do let(:issue_id) { :foo } - let(:design) { OpenStruct.new(issue_id: issue_id) } + let(:design) { double('id', issue_id: issue_id) } subject { described_class.new(design, :delete) } diff --git a/spec/models/design_management/design_at_version_spec.rb b/spec/models/design_management/design_at_version_spec.rb index a7cf6a9652b..7f1fe7b1e13 100644 --- a/spec/models/design_management/design_at_version_spec.rb +++ b/spec/models/design_management/design_at_version_spec.rb @@ -59,7 +59,7 @@ RSpec.describe DesignManagement::DesignAtVersion do it 'rejects objects with the same id and the wrong class' do dav = build_stubbed(:design_at_version) - expect(dav).not_to eq(OpenStruct.new(id: dav.id)) + expect(dav).not_to eq(double('id', id: dav.id)) end it 'expects objects to be of the same type, not subtypes' do diff --git a/spec/models/draft_note_spec.rb b/spec/models/draft_note_spec.rb index 580a588ae1d..0f85871fd9e 100644 --- a/spec/models/draft_note_spec.rb +++ b/spec/models/draft_note_spec.rb @@ -20,6 +20,28 @@ RSpec.describe DraftNote do it { is_expected.to delegate_method(:file_identifier_hash).to(:diff_file).allow_nil } end + describe '#line_code' do + describe 'stored line_code' do + let(:draft_note) { build(:draft_note, merge_request: merge_request, line_code: '1234567890') } + + it 'returns stored line_code' do + expect(draft_note.line_code).to eq('1234567890') + end + end + + describe 'none stored line_code' do + let(:draft_note) { build(:draft_note, merge_request: merge_request) } + + before do + allow(draft_note).to receive(:find_line_code).and_return('none stored line_code') + end + + it 'returns found line_code' do + expect(draft_note.line_code).to eq('none stored line_code') + end + end + end + describe '#diff_file' do let(:draft_note) { build(:draft_note, merge_request: merge_request) } diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 3dd0e01d7b3..112dc93658f 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -412,7 +412,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do context 'in the same branch' do it 'returns true' do - expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be true + expect(environment.includes_commit?(RepoHelpers.sample_commit.id)).to be true end end @@ -422,7 +422,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end it 'returns false' do - expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be false + expect(environment.includes_commit?(RepoHelpers.sample_commit.id)).to be false end end end diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb index 1b9b38a0932..1db1171401c 100644 --- a/spec/models/environment_status_spec.rb +++ b/spec/models/environment_status_spec.rb @@ -161,7 +161,7 @@ RSpec.describe EnvironmentStatus do let!(:build) { create(:ci_build, :with_deployment, :deploy_to_production, pipeline: pipeline) } let(:environment) { build.deployment.environment } - let(:user) { project.owner } + let(:user) { project.first_owner } context 'when environment is created on a forked project', :sidekiq_inline do let(:project) { create(:project, :repository) } diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 97854086162..f099015e63e 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -25,20 +25,21 @@ RSpec.describe Event do expect(instance).to receive(:reset_project_activity) end - create_push_event(project, project.owner) + create_push_event(project, project.first_owner) end end describe 'after_create :set_last_repository_updated_at' do context 'with a push event' do - it 'updates the project last_repository_updated_at' do - project.update!(last_repository_updated_at: 1.year.ago) + it 'updates the project last_repository_updated_at and updated_at' do + project.touch(:last_repository_updated_at, time: 1.year.ago) # rubocop: disable Rails/SkipsModelValidations - create_push_event(project, project.owner) + event = create_push_event(project, project.first_owner) project.reload - expect(project.last_repository_updated_at).to be_within(1.minute).of(Time.current) + expect(project.last_repository_updated_at).to be_like_time(event.created_at) + expect(project.updated_at).to be_like_time(event.created_at) end end @@ -46,7 +47,7 @@ RSpec.describe Event do it 'does not update the project last_repository_updated_at' do project.update!(last_repository_updated_at: 1.year.ago) - create(:closed_issue_event, project: project, author: project.owner) + create(:closed_issue_event, project: project, author: project.first_owner) project.reload @@ -62,14 +63,14 @@ RSpec.describe Event do project.reload # a reload removes fractions of seconds expect do - create_push_event(project, project.owner) + create_push_event(project, project.first_owner) project.reload end.not_to change { project.last_repository_updated_at } end end describe 'after_create UserInteractedProject.track' do - let(:event) { build(:push_event, project: project, author: project.owner) } + let(:event) { build(:push_event, project: project, author: project.first_owner) } it 'passes event to UserInteractedProject.track' do expect(UserInteractedProject).to receive(:track).with(event) @@ -156,7 +157,7 @@ RSpec.describe Event do describe "Push event" do let(:project) { create(:project, :private) } - let(:user) { project.owner } + let(:user) { project.first_owner } let(:event) { create_push_event(project, user) } it do @@ -172,7 +173,7 @@ RSpec.describe Event do describe '#target_title' do let_it_be(:project) { create(:project) } - let(:author) { project.owner } + let(:author) { project.first_owner } let(:target) { nil } let(:event) do @@ -745,7 +746,7 @@ RSpec.describe Event do target = kind == :project ? nil : build(kind, **extra_data) - [kind, build(:event, :created, author: project.owner, project: project, target: target)] + [kind, build(:event, :created, author: project.first_owner, project: project, target: target)] end end @@ -829,19 +830,20 @@ RSpec.describe Event do expect(project).not_to receive(:update_column) .with(:last_activity_at, a_kind_of(Time)) - create_push_event(project, project.owner) + create_push_event(project, project.first_owner) end end context 'when a project was updated more than 1 hour ago' do it 'updates the project' do - project.update!(last_activity_at: 1.year.ago) + project.touch(:last_activity_at, time: 1.year.ago) # rubocop: disable Rails/SkipsModelValidations - create_push_event(project, project.owner) + event = create_push_event(project, project.first_owner) project.reload - expect(project.last_activity_at).to be_within(1.minute).of(Time.current) + expect(project.last_activity_at).to be_like_time(event.created_at) + expect(project.updated_at).to be_like_time(event.created_at) end end end diff --git a/spec/models/external_pull_request_spec.rb b/spec/models/external_pull_request_spec.rb index b141600c4fd..82da7cdf34b 100644 --- a/spec/models/external_pull_request_spec.rb +++ b/spec/models/external_pull_request_spec.rb @@ -236,4 +236,11 @@ RSpec.describe ExternalPullRequest do it_behaves_like 'it has loose foreign keys' do let(:factory_name) { :external_pull_request } end + + context 'loose foreign key on external_pull_requests.project_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:project) } + let!(:model) { create(:external_pull_request, project: parent) } + end + end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 05ee2166245..4bc4df02c24 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1249,7 +1249,7 @@ RSpec.describe Group do let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 } let!(:group) { create(:group, id: common_id) } let!(:unrelated_project) { create(:project, id: common_id) } - let(:user) { unrelated_project.owner } + let(:user) { unrelated_project.first_owner } it 'returns correct access level' do expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS) diff --git a/spec/models/hooks/service_hook_spec.rb b/spec/models/hooks/service_hook_spec.rb index 85f433f5f81..0d65fe302e1 100644 --- a/spec/models/hooks/service_hook_spec.rb +++ b/spec/models/hooks/service_hook_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ServiceHook do let(:data) { { key: 'value' } } it '#execute' do - expect(WebHookService).to receive(:new).with(hook, data, 'service_hook').and_call_original + expect(WebHookService).to receive(:new).with(hook, data, 'service_hook', force: false).and_call_original expect_any_instance_of(WebHookService).to receive(:execute) hook.execute(data) diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb index 89bfb742f5d..a3d36058b74 100644 --- a/spec/models/hooks/system_hook_spec.rb +++ b/spec/models/hooks/system_hook_spec.rb @@ -168,17 +168,17 @@ RSpec.describe SystemHook do let(:data) { { key: 'value' } } let(:hook_name) { 'system_hook' } - before do - expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original - end - it '#execute' do + expect(WebHookService).to receive(:new).with(hook, data, hook_name, force: false).and_call_original + expect_any_instance_of(WebHookService).to receive(:execute) hook.execute(data, hook_name) end it '#async_execute' do + expect(WebHookService).to receive(:new).with(hook, data, hook_name).and_call_original + expect_any_instance_of(WebHookService).to receive(:async_execute) hook.async_execute(data, hook_name) diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index c292e78b32d..482e372543c 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -100,12 +100,18 @@ RSpec.describe WebHook do hook.execute(data, hook_name) end - it 'does not execute non-executable hooks' do - hook.update!(disabled_until: 1.day.from_now) + it 'passes force: false to the web hook service by default' do + expect(WebHookService) + .to receive(:new).with(hook, data, hook_name, force: false).and_return(double(execute: :done)) - expect(WebHookService).not_to receive(:new) + expect(hook.execute(data, hook_name)).to eq :done + end - hook.execute(data, hook_name) + it 'passes force: true to the web hook service if required' do + expect(WebHookService) + .to receive(:new).with(hook, data, hook_name, force: true).and_return(double(execute: :forced)) + + expect(hook.execute(data, hook_name, force: true)).to eq :forced end it '#async_execute' do diff --git a/spec/models/instance_configuration_spec.rb b/spec/models/instance_configuration_spec.rb index a47bc6a5b6d..6b0d8d7ca4a 100644 --- a/spec/models/instance_configuration_spec.rb +++ b/spec/models/instance_configuration_spec.rb @@ -206,7 +206,8 @@ RSpec.describe InstanceConfiguration do group_download_export_limit: 1019, group_import_limit: 1020, raw_blob_request_limit: 1021, - user_email_lookup_limit: 1022 + user_email_lookup_limit: 1022, + users_get_by_id_limit: 1023 ) end @@ -230,6 +231,7 @@ RSpec.describe InstanceConfiguration do expect(rate_limits[:group_import]).to eq({ enabled: true, requests_per_period: 1020, period_in_seconds: 60 }) expect(rate_limits[:raw_blob]).to eq({ enabled: true, requests_per_period: 1021, period_in_seconds: 60 }) expect(rate_limits[:user_email_lookup]).to eq({ enabled: true, requests_per_period: 1022, period_in_seconds: 60 }) + expect(rate_limits[:users_get_by_id]).to eq({ enabled: true, requests_per_period: 1023, period_in_seconds: 600 }) end end end diff --git a/spec/models/instance_metadata_spec.rb b/spec/models/instance_metadata_spec.rb index e3a9167620b..5fc073c392d 100644 --- a/spec/models/instance_metadata_spec.rb +++ b/spec/models/instance_metadata_spec.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true require 'fast_spec_helper' +require_relative '../../app/models/instance_metadata' +require_relative '../../app/models/instance_metadata/kas' RSpec.describe InstanceMetadata do it 'has the correct properties' do diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb index 7bc670302f1..e822620ab80 100644 --- a/spec/models/integration_spec.rb +++ b/spec/models/integration_spec.rb @@ -710,30 +710,21 @@ RSpec.describe Integration do [ { name: 'token' }, { name: 'api_token' }, + { name: 'token_api' }, + { name: 'safe_token' }, { name: 'key' }, { name: 'api_key' }, { name: 'password' }, { name: 'password_field' }, + { name: 'some_safe_field' }, { name: 'safe_field' } - ] + ].shuffle end end end - let(:integration) do - fake_integration.new(properties: [ - { token: 'token-value' }, - { api_token: 'api_token-value' }, - { key: 'key-value' }, - { api_key: 'api_key-value' }, - { password: 'password-value' }, - { password_field: 'password_field-value' }, - { safe_field: 'safe_field-value' } - ]) - end - it 'filters out sensitive fields' do - expect(integration.api_field_names).to eq(['safe_field']) + expect(fake_integration.new).to have_attributes(api_field_names: match_array(%w[some_safe_field safe_field])) end end diff --git a/spec/models/integrations/datadog_spec.rb b/spec/models/integrations/datadog_spec.rb index 9856c53a390..cfc44b22a84 100644 --- a/spec/models/integrations/datadog_spec.rb +++ b/spec/models/integrations/datadog_spec.rb @@ -16,6 +16,7 @@ RSpec.describe Integrations::Datadog do let(:api_key) { SecureRandom.hex(32) } let(:dd_env) { 'ci' } let(:dd_service) { 'awesome-gitlab' } + let(:dd_tags) { '' } let(:expected_hook_url) { default_url + "?dd-api-key=#{api_key}&env=#{dd_env}&service=#{dd_service}" } @@ -27,7 +28,8 @@ RSpec.describe Integrations::Datadog do api_url: api_url, api_key: api_key, datadog_env: dd_env, - datadog_service: dd_service + datadog_service: dd_service, + datadog_tags: dd_tags ) end @@ -95,6 +97,20 @@ RSpec.describe Integrations::Datadog do it { is_expected.not_to allow_value('datadog hq.com').for(:datadog_site) } it { is_expected.not_to allow_value('example.com').for(:api_url) } end + + context 'with custom tags' do + it { is_expected.to allow_value('').for(:datadog_tags) } + it { is_expected.to allow_value('key:value').for(:datadog_tags) } + it { is_expected.to allow_value("key:value\nkey2:value2").for(:datadog_tags) } + it { is_expected.to allow_value("key:value\nkey2:value with spaces and 123?&$").for(:datadog_tags) } + it { is_expected.to allow_value("key:value\n\n\n\nkey2:value2\n").for(:datadog_tags) } + + it { is_expected.not_to allow_value('value').for(:datadog_tags) } + it { is_expected.not_to allow_value('key:').for(:datadog_tags) } + it { is_expected.not_to allow_value('key: ').for(:datadog_tags) } + it { is_expected.not_to allow_value(':value').for(:datadog_tags) } + it { is_expected.not_to allow_value("key:value\nINVALID").for(:datadog_tags) } + end end context 'when integration is not active' do @@ -134,9 +150,23 @@ RSpec.describe Integrations::Datadog do context 'without optional params' do let(:dd_service) { '' } let(:dd_env) { '' } + let(:dd_tags) { '' } it { is_expected.to eq(default_url + "?dd-api-key=#{api_key}") } end + + context 'with custom tags' do + let(:dd_tags) { "key:value\nkey2:value, 2" } + let(:escaped_tags) { CGI.escape("key:value,\"key2:value, 2\"") } + + it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + + context 'and empty lines' do + let(:dd_tags) { "key:value\r\n\n\n\nkey2:value, 2\n" } + + it { is_expected.to eq(expected_hook_url + "&tags=#{escaped_tags}") } + end + end end describe '#test' do diff --git a/spec/models/issue_collection_spec.rb b/spec/models/issue_collection_spec.rb index d67bd8debce..183082bab26 100644 --- a/spec/models/issue_collection_spec.rb +++ b/spec/models/issue_collection_spec.rb @@ -50,7 +50,9 @@ RSpec.describe IssueCollection do end end - context 'using a user that is the owner of a project' do + # TODO update when we have multiple owners of a project + # https://gitlab.com/gitlab-org/gitlab/-/issues/350605 + context 'using a user that is an owner of a project' do it 'returns the issues of the project' do expect(collection.updatable_by_user(project.namespace.owner)) .to eq([issue1, issue2]) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index c105f6c3439..5af42cc67ea 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -887,6 +887,8 @@ RSpec.describe Issue do end end + # TODO update when we have multiple owners of a project + # https://gitlab.com/gitlab-org/gitlab/-/issues/350605 context 'with an owner' do before do project.add_maintainer(user) @@ -1431,26 +1433,6 @@ RSpec.describe Issue do end end - describe '.with_label_attributes' do - subject { described_class.with_label_attributes(label_attributes) } - - let(:label_attributes) { { title: 'hello world', description: 'hi' } } - - it 'gets issues with given label attributes' do - label = create(:label, **label_attributes) - labeled_issue = create(:labeled_issue, project: label.project, labels: [label]) - - expect(subject).to include(labeled_issue) - end - - it 'excludes issues without given label attributes' do - label = create(:label, title: 'GitLab', description: 'tanuki') - labeled_issue = create(:labeled_issue, project: label.project, labels: [label]) - - expect(subject).not_to include(labeled_issue) - end - end - describe 'banzai_render_context' do let(:project) { build(:project_empty_repo) } let(:issue) { build :issue, project: project } diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index 19459561edf..6cf73de6cef 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -20,6 +20,8 @@ RSpec.describe Key, :mailer do it { is_expected.to allow_value(attributes_for(:dsa_key_2048)[:key]).for(:key) } it { is_expected.to allow_value(attributes_for(:ecdsa_key_256)[:key]).for(:key) } it { is_expected.to allow_value(attributes_for(:ed25519_key_256)[:key]).for(:key) } + it { is_expected.to allow_value(attributes_for(:ecdsa_sk_key_256)[:key]).for(:key) } + it { is_expected.to allow_value(attributes_for(:ed25519_sk_key_256)[:key]).for(:key) } it { is_expected.not_to allow_value('foo-bar').for(:key) } context 'key format' do @@ -187,10 +189,12 @@ RSpec.describe Key, :mailer do forbidden = ApplicationSetting::FORBIDDEN_KEY_VALUE [ - [:rsa_key_2048, 0, true], - [:dsa_key_2048, 0, true], - [:ecdsa_key_256, 0, true], - [:ed25519_key_256, 0, true], + [:rsa_key_2048, 0, true], + [:dsa_key_2048, 0, true], + [:ecdsa_key_256, 0, true], + [:ed25519_key_256, 0, true], + [:ecdsa_sk_key_256, 0, true], + [:ed25519_sk_key_256, 0, true], [:rsa_key_2048, 1024, true], [:rsa_key_2048, 2048, true], @@ -206,10 +210,18 @@ RSpec.describe Key, :mailer do [:ed25519_key_256, 256, true], [:ed25519_key_256, 384, false], - [:rsa_key_2048, forbidden, false], - [:dsa_key_2048, forbidden, false], - [:ecdsa_key_256, forbidden, false], - [:ed25519_key_256, forbidden, false] + [:ecdsa_sk_key_256, 256, true], + [:ecdsa_sk_key_256, 384, false], + + [:ed25519_sk_key_256, 256, true], + [:ed25519_sk_key_256, 384, false], + + [:rsa_key_2048, forbidden, false], + [:dsa_key_2048, forbidden, false], + [:ecdsa_key_256, forbidden, false], + [:ed25519_key_256, forbidden, false], + [:ecdsa_sk_key_256, forbidden, false], + [:ed25519_sk_key_256, forbidden, false] ] end diff --git a/spec/models/label_note_spec.rb b/spec/models/label_note_spec.rb index ee4822c653d..145ddd44834 100644 --- a/spec/models/label_note_spec.rb +++ b/spec/models/label_note_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe LabelNote do + include Gitlab::Routing.url_helpers + let_it_be(:project) { create(:project, :repository) } let_it_be(:user) { create(:user) } let_it_be(:label) { create(:label, project: project) } @@ -14,11 +16,27 @@ RSpec.describe LabelNote do let_it_be(:resource) { create(:issue, project: project) } it_behaves_like 'label note created from events' + + it 'includes a link to the list of issues filtered by the label' do + note = described_class.from_events([ + create(:resource_label_event, label: label, issue: resource) + ]) + + expect(note.note_html).to include(project_issues_path(project, label_name: label.title)) + end end context 'when resource is merge request' do let_it_be(:resource) { create(:merge_request, source_project: project, target_project: project) } it_behaves_like 'label note created from events' + + it 'includes a link to the list of merge requests filtered by the label' do + note = described_class.from_events([ + create(:resource_label_event, label: label, merge_request: resource) + ]) + + expect(note.note_html).to include(project_merge_requests_path(project, label_name: label.title)) + end end end diff --git a/spec/models/loose_foreign_keys/deleted_record_spec.rb b/spec/models/loose_foreign_keys/deleted_record_spec.rb index 07ffff746a5..23e0ed1f39d 100644 --- a/spec/models/loose_foreign_keys/deleted_record_spec.rb +++ b/spec/models/loose_foreign_keys/deleted_record_spec.rb @@ -6,15 +6,15 @@ RSpec.describe LooseForeignKeys::DeletedRecord, type: :model do let_it_be(:table) { 'public.projects' } describe 'class methods' do - let_it_be(:deleted_record_1) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 5) } - let_it_be(:deleted_record_2) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 1) } + let_it_be(:deleted_record_1) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 5, cleanup_attempts: 2) } + let_it_be(:deleted_record_2) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 1, cleanup_attempts: 0) } let_it_be(:deleted_record_3) { described_class.create!(fully_qualified_table_name: 'public.other_table', primary_key_value: 3) } - let_it_be(:deleted_record_4) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 1) } # duplicate + let_it_be(:deleted_record_4) { described_class.create!(fully_qualified_table_name: table, primary_key_value: 1, cleanup_attempts: 1) } # duplicate + + let(:records) { described_class.load_batch_for_table(table, 10) } describe '.load_batch_for_table' do it 'loads records and orders them by creation date' do - records = described_class.load_batch_for_table(table, 10) - expect(records).to eq([deleted_record_1, deleted_record_2, deleted_record_4]) end @@ -27,13 +27,38 @@ RSpec.describe LooseForeignKeys::DeletedRecord, type: :model do describe '.mark_records_processed' do it 'updates all records' do - records = described_class.load_batch_for_table(table, 10) described_class.mark_records_processed(records) expect(described_class.status_pending.count).to eq(1) expect(described_class.status_processed.count).to eq(3) end end + + describe '.reschedule' do + it 'reschedules all records' do + time = Time.zone.parse('2022-01-01').utc + update_count = described_class.reschedule(records, time) + + expect(update_count).to eq(records.size) + + records.each(&:reload) + + expect(records).to all(have_attributes( + cleanup_attempts: 0, + consume_after: time + )) + end + end + + describe '.increment_attempts' do + it 'increaments the cleanup_attempts column' do + described_class.increment_attempts(records) + + expect(deleted_record_1.reload.cleanup_attempts).to eq(3) + expect(deleted_record_2.reload.cleanup_attempts).to eq(1) + expect(deleted_record_4.reload.cleanup_attempts).to eq(2) + end + end end describe 'sliding_list partitioning' do diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 1957c58ec81..79491edba94 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -173,6 +173,8 @@ RSpec.describe Member do let_it_be(:group) { create(:group) } let_it_be(:blocked_pending_approval_user) { create(:user, :blocked_pending_approval ) } let_it_be(:blocked_pending_approval_project_member) { create(:project_member, :invited, :developer, project: project, invite_email: blocked_pending_approval_user.email) } + let_it_be(:awaiting_group_member) { create(:group_member, :awaiting, group: group) } + let_it_be(:awaiting_project_member) { create(:project_member, :awaiting, project: project) } before_all do @owner_user = create(:user).tap { |u| group.add_owner(u) } @@ -471,6 +473,8 @@ RSpec.describe Member do it { is_expected.to include @blocked_maintainer } it { is_expected.to include @blocked_developer } it { is_expected.not_to include @member_with_minimal_access } + it { is_expected.not_to include awaiting_group_member } + it { is_expected.not_to include awaiting_project_member } end describe '.connected_to_user' do @@ -509,6 +513,8 @@ RSpec.describe Member do it { is_expected.not_to include @invited_member } it { is_expected.not_to include @requested_member } it { is_expected.not_to include @member_with_minimal_access } + it { is_expected.not_to include awaiting_group_member } + it { is_expected.not_to include awaiting_project_member } end describe '.distinct_on_user_with_max_access_level' do @@ -561,6 +567,21 @@ RSpec.describe Member do end end end + + describe '.active_state' do + let_it_be(:active_group_member) { create(:group_member, group: group) } + let_it_be(:active_project_member) { create(:project_member, project: project) } + + it 'includes members with an active state' do + expect(group.members.active_state).to include active_group_member + expect(project.members.active_state).to include active_project_member + end + + it 'does not include members with an awaiting state' do + expect(group.members.active_state).not_to include awaiting_group_member + expect(project.members.active_state).not_to include awaiting_project_member + end + end end describe 'Delegate methods' do @@ -894,4 +915,15 @@ RSpec.describe Member do end end end + + describe '#set_member_namespace_id' do + let(:group) { create(:group) } + let(:member) { create(:group_member, group: group) } + + describe 'on create' do + it 'sets the member_namespace_id' do + expect(member.member_namespace_id).to eq group.id + end + end + end end diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 031caefbd43..3923f4161cc 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -257,4 +257,15 @@ RSpec.describe ProjectMember do it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' end end + + describe '#set_member_namespace_id' do + let(:project) { create(:project) } + let(:member) { create(:project_member, project: project) } + + context 'on create' do + it 'sets the member_namespace_id' do + expect(member.member_namespace_id).to eq project.project_namespace_id + end + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 4005a2ec6da..f2f2023a992 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -144,6 +144,20 @@ RSpec.describe MergeRequest, factory_default: :keep do end end + describe '.attention' do + let_it_be(:merge_request5) { create(:merge_request, :unique_branches, assignees: [user2])} + let_it_be(:merge_request6) { create(:merge_request, :unique_branches, assignees: [user2])} + + before do + assignee = merge_request6.find_assignee(user2) + assignee.update!(state: :reviewed) + end + + it 'returns MRs that have any attention requests' do + expect(described_class.attention(user2)).to eq([merge_request2, merge_request5]) + end + end + describe '.drafts' do it 'returns MRs where draft == true' do expect(described_class.drafts).to eq([merge_request4]) @@ -585,6 +599,21 @@ RSpec.describe MergeRequest, factory_default: :keep do expect(merge_requests).to eq([older_mr, newer_mr]) end end + + context 'title' do + let_it_be(:first_mr) { create(:merge_request, :closed, title: 'One') } + let_it_be(:second_mr) { create(:merge_request, :closed, title: 'Two') } + + it 'sorts asc' do + merge_requests = described_class.sort_by_attribute(:title_asc) + expect(merge_requests).to eq([first_mr, second_mr]) + end + + it 'sorts desc' do + merge_requests = described_class.sort_by_attribute(:title_desc) + expect(merge_requests).to eq([second_mr, first_mr]) + end + end end describe 'time to merge calculations' do @@ -1354,17 +1383,17 @@ RSpec.describe MergeRequest, factory_default: :keep do subject { build_stubbed(:merge_request) } [ - 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', ' [WIP] WIP: [WIP] WIP:', 'draft:', 'Draft: ', '[Draft]', '[DRAFT] ' - ].each do |wip_prefix| - it "detects the '#{wip_prefix}' prefix" do - subject.title = "#{wip_prefix}#{subject.title}" + ].each do |draft_prefix| + it "detects the '#{draft_prefix}' prefix" do + subject.title = "#{draft_prefix}#{subject.title}" expect(subject.work_in_progress?).to eq true end end [ + 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', ' [WIP] WIP: [WIP] WIP:', "WIP ", "(WIP)", "draft", "Draft", "Draft -", "draft - ", "Draft ", "draft " ].each do |draft_prefix| @@ -1375,10 +1404,10 @@ RSpec.describe MergeRequest, factory_default: :keep do end end - it "detects merge request title just saying 'wip'" do + it "doesn't detect merge request title just saying 'wip'" do subject.title = "wip" - expect(subject.work_in_progress?).to eq true + expect(subject.work_in_progress?).to eq false end it "does not detect merge request title just saying 'draft'" do @@ -1444,29 +1473,30 @@ RSpec.describe MergeRequest, factory_default: :keep do describe "#wipless_title" do subject { build_stubbed(:merge_request) } - [ - 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', '[WIP] WIP: [WIP] WIP:', - 'draft:', 'Draft: ', '[Draft]', '[DRAFT] ' - ].each do |wip_prefix| - it "removes the '#{wip_prefix}' prefix" do + ['draft:', 'Draft: ', '[Draft]', '[DRAFT] '].each do |draft_prefix| + it "removes a '#{draft_prefix}' prefix" do wipless_title = subject.title - subject.title = "#{wip_prefix}#{subject.title}" + subject.title = "#{draft_prefix}#{subject.title}" expect(subject.wipless_title).to eq wipless_title end it "is satisfies the #work_in_progress? method" do - subject.title = "#{wip_prefix}#{subject.title}" + subject.title = "#{draft_prefix}#{subject.title}" subject.title = subject.wipless_title expect(subject.work_in_progress?).to eq false end end - it 'removes only WIP prefix from the MR title' do - subject.title = 'WIP: Implement feature called WIP' + [ + 'WIP:', 'WIP: ', '[WIP]', '[WIP] ', '[WIP] WIP: [WIP] WIP:' + ].each do |wip_prefix| + it "doesn't remove a '#{wip_prefix}' prefix" do + subject.title = "#{wip_prefix}#{subject.title}" - expect(subject.wipless_title).to eq 'Implement feature called WIP' + expect(subject.wipless_title).to eq subject.title + end end it 'removes only draft prefix from the MR title' do @@ -1522,6 +1552,42 @@ RSpec.describe MergeRequest, factory_default: :keep do end end + describe '#permits_force_push?' do + let_it_be(:merge_request) { build_stubbed(:merge_request) } + + subject { merge_request.permits_force_push? } + + context 'when source branch is not protected' do + before do + allow(ProtectedBranch).to receive(:protected?).and_return(false) + end + + it { is_expected.to be_truthy } + end + + context 'when source branch is protected' do + before do + allow(ProtectedBranch).to receive(:protected?).and_return(true) + end + + context 'when force push is not allowed' do + before do + allow(ProtectedBranch).to receive(:allow_force_push?) { false } + end + + it { is_expected.to be_falsey } + end + + context 'when force push is allowed' do + before do + allow(ProtectedBranch).to receive(:allow_force_push?) { true } + end + + it { is_expected.to be_truthy } + end + end + end + describe '#can_remove_source_branch?' do let_it_be(:user) { create(:user) } let_it_be(:merge_request, reload: true) { create(:merge_request, :simple) } @@ -3509,7 +3575,7 @@ RSpec.describe MergeRequest, factory_default: :keep do let!(:job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) } it 'returns environments' do - is_expected.to eq(pipeline.environments) + is_expected.to eq(pipeline.environments_in_self_and_descendants.to_a) expect(subject.count).to be(1) end @@ -3562,21 +3628,38 @@ RSpec.describe MergeRequest, factory_default: :keep do end describe '#update_diff_discussion_positions' do - let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion } - let(:commit) { subject.project.commit(sample_commit.id) } - let(:old_diff_refs) { subject.diff_refs } + subject { create(:merge_request, source_project: project) } - before do - # Update merge_request_diff so that #diff_refs will return commit.diff_refs - allow(subject).to receive(:create_merge_request_diff) do - subject.merge_request_diffs.create!( - base_commit_sha: commit.parent_id, - start_commit_sha: commit.parent_id, - head_commit_sha: commit.sha - ) + let(:project) { create(:project, :repository) } + let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") } + let(:modify_commit) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e") } + let(:edit_commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } + let(:discussion) { create(:diff_note_on_merge_request, noteable: subject, project: project, position: old_position).to_discussion } + let(:path) { "files/ruby/popen.rb" } + let(:new_line) { 9 } + + let(:old_diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: create_commit.parent_id, + head_sha: modify_commit.sha + ) + end - subject.reload_merge_request_diff - end + let(:new_diff_refs) do + Gitlab::Diff::DiffRefs.new( + base_sha: create_commit.parent_id, + head_sha: edit_commit.sha + ) + end + + let(:old_position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + old_line: nil, + new_line: new_line, + diff_refs: old_diff_refs + ) end it "updates diff discussion positions" do @@ -3584,36 +3667,67 @@ RSpec.describe MergeRequest, factory_default: :keep do subject.project, subject.author, old_diff_refs: old_diff_refs, - new_diff_refs: commit.diff_refs, + new_diff_refs: new_diff_refs, paths: discussion.position.paths ).and_call_original expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original - expect_any_instance_of(DiffNote).to receive(:save).once subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, - new_diff_refs: commit.diff_refs, + new_diff_refs: new_diff_refs, current_user: subject.author) end - context 'when resolve_outdated_diff_discussions is set' do - let(:project) { create(:project, :repository) } + it 'does not call the resolve method' do + expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new) - subject { create(:merge_request, source_project: project) } + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + current_user: subject.author) + end + context 'when resolve_outdated_diff_discussions is set' do before do discussion subject.project.update!(resolve_outdated_diff_discussions: true) end - it 'calls MergeRequests::ResolvedDiscussionNotificationService' do - expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService) - .to receive(:execute).with(subject) + context 'when the active discussion is resolved in the update' do + it 'calls MergeRequests::ResolvedDiscussionNotificationService' do + expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService) + .to receive(:execute).with(subject) - subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, - new_diff_refs: commit.diff_refs, - current_user: subject.author) + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + current_user: subject.author) + end + end + + context 'when the active discussion does not have resolved in the update' do + let(:new_line) { 16 } + + it 'does not call the resolve method' do + expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new) + + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + current_user: subject.author) + end + end + + context 'when the active discussion was already resolved' do + before do + discussion.resolve!(subject.author) + end + + it 'does not call the resolve method' do + expect(MergeRequests::ResolvedDiscussionNotificationService).not_to receive(:new) + + subject.update_diff_discussion_positions(old_diff_refs: old_diff_refs, + new_diff_refs: new_diff_refs, + current_user: subject.author) + end end end end @@ -4965,4 +5079,11 @@ RSpec.describe MergeRequest, factory_default: :keep do it_behaves_like 'it has loose foreign keys' do let(:factory_name) { :merge_request } end + + context 'loose foreign key on merge_requests.head_pipeline_id' do + it_behaves_like 'cleanup by a loose foreign key' do + let!(:parent) { create(:ci_pipeline) } + let!(:model) { create(:merge_request, head_pipeline: parent) } + end + end end diff --git a/spec/models/namespace/root_storage_statistics_spec.rb b/spec/models/namespace/root_storage_statistics_spec.rb index 51c191069ec..11852828eab 100644 --- a/spec/models/namespace/root_storage_statistics_spec.rb +++ b/spec/models/namespace/root_storage_statistics_spec.rb @@ -28,24 +28,24 @@ RSpec.describe Namespace::RootStorageStatistics, type: :model do let(:project1) { create(:project, namespace: namespace) } let(:project2) { create(:project, namespace: namespace) } - let!(:stat1) { create(:project_statistics, project: project1, with_data: true, size_multiplier: 100) } - let!(:stat2) { create(:project_statistics, project: project2, with_data: true, size_multiplier: 200) } + let!(:project_stat1) { create(:project_statistics, project: project1, with_data: true, size_multiplier: 100) } + let!(:project_stat2) { create(:project_statistics, project: project2, with_data: true, size_multiplier: 200) } - shared_examples 'data refresh' do + shared_examples 'project data refresh' do it 'aggregates project statistics' do root_storage_statistics.recalculate! root_storage_statistics.reload - total_repository_size = stat1.repository_size + stat2.repository_size - total_wiki_size = stat1.wiki_size + stat2.wiki_size - total_lfs_objects_size = stat1.lfs_objects_size + stat2.lfs_objects_size - total_build_artifacts_size = stat1.build_artifacts_size + stat2.build_artifacts_size - total_packages_size = stat1.packages_size + stat2.packages_size - total_storage_size = stat1.storage_size + stat2.storage_size - total_snippets_size = stat1.snippets_size + stat2.snippets_size - total_pipeline_artifacts_size = stat1.pipeline_artifacts_size + stat2.pipeline_artifacts_size - total_uploads_size = stat1.uploads_size + stat2.uploads_size + total_repository_size = project_stat1.repository_size + project_stat2.repository_size + total_wiki_size = project_stat1.wiki_size + project_stat2.wiki_size + total_lfs_objects_size = project_stat1.lfs_objects_size + project_stat2.lfs_objects_size + total_build_artifacts_size = project_stat1.build_artifacts_size + project_stat2.build_artifacts_size + total_packages_size = project_stat1.packages_size + project_stat2.packages_size + total_storage_size = project_stat1.storage_size + project_stat2.storage_size + total_snippets_size = project_stat1.snippets_size + project_stat2.snippets_size + total_pipeline_artifacts_size = project_stat1.pipeline_artifacts_size + project_stat2.pipeline_artifacts_size + total_uploads_size = project_stat1.uploads_size + project_stat2.uploads_size expect(root_storage_statistics.repository_size).to eq(total_repository_size) expect(root_storage_statistics.wiki_size).to eq(total_wiki_size) @@ -83,7 +83,7 @@ RSpec.describe Namespace::RootStorageStatistics, type: :model do end end - it_behaves_like 'data refresh' + it_behaves_like 'project data refresh' it_behaves_like 'does not include personal snippets' context 'with subgroups' do @@ -93,19 +93,81 @@ RSpec.describe Namespace::RootStorageStatistics, type: :model do let(:project1) { create(:project, namespace: subgroup1) } let(:project2) { create(:project, namespace: subgroup2) } - it_behaves_like 'data refresh' + it_behaves_like 'project data refresh' it_behaves_like 'does not include personal snippets' end + context 'with a group namespace' do + let_it_be(:root_group) { create(:group) } + let_it_be(:group1) { create(:group, parent: root_group) } + let_it_be(:subgroup1) { create(:group, parent: group1) } + let_it_be(:group2) { create(:group, parent: root_group) } + let_it_be(:root_namespace_stat) { create(:namespace_statistics, namespace: root_group, storage_size: 100, dependency_proxy_size: 100) } + let_it_be(:group1_namespace_stat) { create(:namespace_statistics, namespace: group1, storage_size: 200, dependency_proxy_size: 200) } + let_it_be(:group2_namespace_stat) { create(:namespace_statistics, namespace: group2, storage_size: 300, dependency_proxy_size: 300) } + let_it_be(:subgroup1_namespace_stat) { create(:namespace_statistics, namespace: subgroup1, storage_size: 300, dependency_proxy_size: 100) } + + let(:namespace) { root_group } + + it 'aggregates namespace statistics' do + # This group is not a descendant of the root_group so it shouldn't be included in the final stats. + other_group = create(:group) + create(:namespace_statistics, namespace: other_group, storage_size: 500, dependency_proxy_size: 500) + + root_storage_statistics.recalculate! + + total_repository_size = project_stat1.repository_size + project_stat2.repository_size + total_lfs_objects_size = project_stat1.lfs_objects_size + project_stat2.lfs_objects_size + total_build_artifacts_size = project_stat1.build_artifacts_size + project_stat2.build_artifacts_size + total_packages_size = project_stat1.packages_size + project_stat2.packages_size + total_snippets_size = project_stat1.snippets_size + project_stat2.snippets_size + total_pipeline_artifacts_size = project_stat1.pipeline_artifacts_size + project_stat2.pipeline_artifacts_size + total_uploads_size = project_stat1.uploads_size + project_stat2.uploads_size + total_wiki_size = project_stat1.wiki_size + project_stat2.wiki_size + total_dependency_proxy_size = root_namespace_stat.dependency_proxy_size + group1_namespace_stat.dependency_proxy_size + group2_namespace_stat.dependency_proxy_size + subgroup1_namespace_stat.dependency_proxy_size + total_storage_size = project_stat1.storage_size + project_stat2.storage_size + root_namespace_stat.storage_size + group1_namespace_stat.storage_size + group2_namespace_stat.storage_size + subgroup1_namespace_stat.storage_size + + expect(root_storage_statistics.repository_size).to eq(total_repository_size) + expect(root_storage_statistics.lfs_objects_size).to eq(total_lfs_objects_size) + expect(root_storage_statistics.build_artifacts_size).to eq(total_build_artifacts_size) + expect(root_storage_statistics.packages_size).to eq(total_packages_size) + expect(root_storage_statistics.snippets_size).to eq(total_snippets_size) + expect(root_storage_statistics.pipeline_artifacts_size).to eq(total_pipeline_artifacts_size) + expect(root_storage_statistics.uploads_size).to eq(total_uploads_size) + expect(root_storage_statistics.dependency_proxy_size).to eq(total_dependency_proxy_size) + expect(root_storage_statistics.wiki_size).to eq(total_wiki_size) + expect(root_storage_statistics.storage_size).to eq(total_storage_size) + end + + it 'works when there are no namespace statistics' do + NamespaceStatistics.delete_all + + root_storage_statistics.recalculate! + + total_storage_size = project_stat1.storage_size + project_stat2.storage_size + + expect(root_storage_statistics.storage_size).to eq(total_storage_size) + end + end + context 'with a personal namespace' do let_it_be(:user) { create(:user) } let(:namespace) { user.namespace } - it_behaves_like 'data refresh' + it_behaves_like 'project data refresh' + + it 'does not aggregate namespace statistics' do + create(:namespace_statistics, namespace: user.namespace, storage_size: 200, dependency_proxy_size: 200) + + root_storage_statistics.recalculate! + + expect(root_storage_statistics.storage_size).to eq(project_stat1.storage_size + project_stat2.storage_size) + expect(root_storage_statistics.dependency_proxy_size).to eq(0) + end context 'when user has personal snippets' do - let(:total_project_snippets_size) { stat1.snippets_size + stat2.snippets_size } + let(:total_project_snippets_size) { project_stat1.snippets_size + project_stat2.snippets_size } it 'aggregates personal and project snippets size' do # This is just a a snippet authored by other user diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 5da0f7a134c..1728d4fc3f3 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -23,6 +23,7 @@ RSpec.describe Namespace do it { is_expected.to have_one :root_storage_statistics } it { is_expected.to have_one :aggregation_schedule } it { is_expected.to have_one :namespace_settings } + it { is_expected.to have_one(:namespace_statistics) } it { is_expected.to have_many :custom_emoji } it { is_expected.to have_one :package_setting_relation } it { is_expected.to have_one :onboarding_progress } @@ -361,10 +362,70 @@ RSpec.describe Namespace do context 'linear' do it_behaves_like 'namespace traversal scopes' end + + shared_examples 'makes recursive queries' do + specify do + expect { subject }.to make_queries_matching(/WITH RECURSIVE/) + end + end + + shared_examples 'does not make recursive queries' do + specify do + expect { subject }.not_to make_queries_matching(/WITH RECURSIVE/) + end + end + + describe '.self_and_descendants' do + let_it_be(:namespace) { create(:namespace) } + + subject { described_class.where(id: namespace).self_and_descendants.load } + + it_behaves_like 'does not make recursive queries' + + context 'when feature flag :use_traversal_ids is disabled' do + before do + stub_feature_flags(use_traversal_ids: false) + end + + it_behaves_like 'makes recursive queries' + end + + context 'when feature flag :use_traversal_ids_for_descendants_scopes is disabled' do + before do + stub_feature_flags(use_traversal_ids_for_descendants_scopes: false) + end + + it_behaves_like 'makes recursive queries' + end + end + + describe '.self_and_descendant_ids' do + let_it_be(:namespace) { create(:namespace) } + + subject { described_class.where(id: namespace).self_and_descendant_ids.load } + + it_behaves_like 'does not make recursive queries' + + context 'when feature flag :use_traversal_ids is disabled' do + before do + stub_feature_flags(use_traversal_ids: false) + end + + it_behaves_like 'makes recursive queries' + end + + context 'when feature flag :use_traversal_ids_for_descendants_scopes is disabled' do + before do + stub_feature_flags(use_traversal_ids_for_descendants_scopes: false) + end + + it_behaves_like 'makes recursive queries' + end + end end context 'traversal_ids on create' do - context 'default traversal_ids' do + shared_examples 'default traversal_ids' do let(:namespace) { build(:namespace) } before do @@ -374,6 +435,18 @@ RSpec.describe Namespace do it { expect(namespace.traversal_ids).to eq [namespace.id] } end + + context 'with before_commit callback' do + it_behaves_like 'default traversal_ids' + end + + context 'with after_create callback' do + before do + stub_feature_flags(sync_traversal_ids_before_commit: false) + end + + it_behaves_like 'default traversal_ids' + end end describe "after_commit :expire_child_caches" do @@ -2158,4 +2231,13 @@ RSpec.describe Namespace do end end end + + describe 'storage_enforcement_date' do + let_it_be(:namespace) { create(:group) } + + # Date TBD: https://gitlab.com/gitlab-org/gitlab/-/issues/350632 + it 'returns false' do + expect(namespace.storage_enforcement_date).to be(nil) + end + end end diff --git a/spec/models/namespace_statistics_spec.rb b/spec/models/namespace_statistics_spec.rb new file mode 100644 index 00000000000..ac747b70a9f --- /dev/null +++ b/spec/models/namespace_statistics_spec.rb @@ -0,0 +1,207 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe NamespaceStatistics do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + + it { is_expected.to belong_to(:namespace) } + + it { is_expected.to validate_presence_of(:namespace) } + + describe '#refresh!' do + let(:namespace) { group } + let(:statistics) { create(:namespace_statistics, namespace: namespace) } + let(:columns) { [] } + + subject(:refresh!) { statistics.refresh!(only: columns) } + + context 'when database is read_only' do + it 'does not save the object' do + allow(Gitlab::Database).to receive(:read_only?).and_return(true) + + expect(statistics).not_to receive(:save!) + + refresh! + end + end + + context 'when namespace belong to a user' do + let(:namespace) { user.namespace } + + it 'does not save the object' do + expect(statistics).not_to receive(:save!) + + refresh! + end + end + + shared_examples 'creates the namespace statistics' do + specify do + expect(statistics).to receive(:save!) + + refresh! + end + end + + context 'when invalid option is passed' do + let(:columns) { [:foo] } + + it 'does not update any column' do + create(:dependency_proxy_manifest, group: namespace, size: 50) + + expect(statistics).not_to receive(:update_dependency_proxy_size) + expect { refresh! }.not_to change { statistics.reload.storage_size } + end + + it_behaves_like 'creates the namespace statistics' + end + + context 'when no option is passed' do + it 'updates the dependency proxy size' do + expect(statistics).to receive(:update_dependency_proxy_size) + + refresh! + end + + it_behaves_like 'creates the namespace statistics' + end + + context 'when dependency_proxy_size option is passed' do + let(:columns) { [:dependency_proxy_size] } + + it 'updates the dependency proxy size' do + expect(statistics).to receive(:update_dependency_proxy_size) + + refresh! + end + + it_behaves_like 'creates the namespace statistics' + end + end + + describe '#update_storage_size' do + let_it_be(:statistics, reload: true) { create(:namespace_statistics, namespace: group) } + + it 'sets storage_size to the dependency_proxy_size' do + statistics.dependency_proxy_size = 3 + + statistics.update_storage_size + + expect(statistics.storage_size).to eq 3 + end + end + + describe '#update_dependency_proxy_size' do + let_it_be(:statistics, reload: true) { create(:namespace_statistics, namespace: group) } + let_it_be(:dependency_proxy_manifest) { create(:dependency_proxy_manifest, group: group, size: 50) } + let_it_be(:dependency_proxy_blob) { create(:dependency_proxy_blob, group: group, size: 50) } + + subject(:update_dependency_proxy_size) { statistics.update_dependency_proxy_size } + + it 'updates the dependency proxy size' do + update_dependency_proxy_size + + expect(statistics.dependency_proxy_size).to eq 100 + end + + context 'when namespace does not belong to a group' do + let(:statistics) { create(:namespace_statistics, namespace: user.namespace) } + + it 'does not update the dependency proxy size' do + update_dependency_proxy_size + + expect(statistics.dependency_proxy_size).to be_zero + end + end + end + + context 'before saving statistics' do + let(:statistics) { create(:namespace_statistics, namespace: group, dependency_proxy_size: 10) } + + it 'updates storage size' do + expect(statistics).to receive(:update_storage_size).and_call_original + + statistics.save! + + expect(statistics.storage_size).to eq 10 + end + end + + context 'after saving statistics', :aggregate_failures do + let(:statistics) { create(:namespace_statistics, namespace: namespace) } + let(:namespace) { group } + + context 'when storage_size is not updated' do + it 'does not enqueue the job to update root storage statistics' do + expect(statistics).not_to receive(:update_root_storage_statistics) + expect(Namespaces::ScheduleAggregationWorker).not_to receive(:perform_async) + + statistics.save! + end + end + + context 'when storage_size is updated' do + before do + # we have to update this value instead of `storage_size` because the before_save + # hook we have. If we don't do it, storage_size will be set to the dependency_proxy_size value + # which is 0. + statistics.dependency_proxy_size = 10 + end + + it 'enqueues the job to update root storage statistics' do + expect(statistics).to receive(:update_root_storage_statistics).and_call_original + expect(Namespaces::ScheduleAggregationWorker).to receive(:perform_async).with(group.id) + + statistics.save! + end + + context 'when namespace does not belong to a group' do + let(:namespace) { user.namespace } + + it 'does not enqueue the job to update root storage statistics' do + expect(statistics).to receive(:update_root_storage_statistics).and_call_original + expect(Namespaces::ScheduleAggregationWorker).not_to receive(:perform_async) + + statistics.save! + end + end + end + + context 'when other columns are updated' do + it 'does not enqueue the job to update root storage statistics' do + columns_to_update = NamespaceStatistics.columns_hash.reject { |k, _| %w(id namespace_id).include?(k) || k.include?('_size') }.keys + columns_to_update.each { |c| statistics[c] = 10 } + + expect(statistics).not_to receive(:update_root_storage_statistics) + expect(Namespaces::ScheduleAggregationWorker).not_to receive(:perform_async) + + statistics.save! + end + end + end + + context 'after destroy statistics', :aggregate_failures do + let(:statistics) { create(:namespace_statistics, namespace: namespace) } + let(:namespace) { group } + + it 'enqueues the job to update root storage statistics' do + expect(statistics).to receive(:update_root_storage_statistics).and_call_original + expect(Namespaces::ScheduleAggregationWorker).to receive(:perform_async).with(group.id) + + statistics.destroy! + end + + context 'when namespace belongs to a group' do + let(:namespace) { user.namespace } + + it 'does not enqueue the job to update root storage statistics' do + expect(statistics).to receive(:update_root_storage_statistics).and_call_original + expect(Namespaces::ScheduleAggregationWorker).not_to receive(:perform_async) + + statistics.destroy! + end + end + end +end diff --git a/spec/models/namespaces/user_namespace_spec.rb b/spec/models/namespaces/user_namespace_spec.rb index 7c00a597756..fb9e7571666 100644 --- a/spec/models/namespaces/user_namespace_spec.rb +++ b/spec/models/namespaces/user_namespace_spec.rb @@ -9,4 +9,13 @@ RSpec.describe Namespaces::UserNamespace, type: :model do describe 'validations' do it { is_expected.to validate_presence_of(:owner) } end + + describe '#owners' do + let(:owner) { build(:user) } + let(:namespace) { build(:namespace, owner: owner) } + + specify do + expect(namespace.owners).to match_array([owner]) + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 9d9cca0678a..34ce0031bd2 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -445,7 +445,7 @@ RSpec.describe Note do end end - describe "#system_note_with_references_visible_for?" do + describe "#system_note_visible_for?" do let(:project) { create(:project, :public) } let(:user) { create(:user) } let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user } @@ -469,22 +469,25 @@ RSpec.describe Note do end it 'returns visible but not readable for non-member user' do - expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy + expect(note.system_note_visible_for?(non_member)).to be_truthy expect(note.readable_by?(non_member)).to be_falsy end it 'returns visible but not readable for a nil user' do - expect(note.system_note_with_references_visible_for?(nil)).to be_truthy + expect(note.system_note_visible_for?(nil)).to be_truthy expect(note.readable_by?(nil)).to be_falsy end end end describe "#system_note_viewable_by?(user)" do - let_it_be(:note) { create(:note) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:note) { create(:note, project: project) } let_it_be(:user) { create(:user) } - let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") } + let(:action) { "commit" } + let!(:metadata) { create(:system_note_metadata, note: note, action: action) } context "when system_note_metadata is not present" do it "returns true" do @@ -494,32 +497,50 @@ RSpec.describe Note do end end - context "system_note_metadata isn't of type 'branch'" do - before do - metadata.action = "not_a_branch" - end - + context "system_note_metadata isn't of type 'branch' or 'contact'" do it "returns true" do expect(note.send(:system_note_viewable_by?, user)).to be_truthy end end - context "user doesn't have :download_code ability" do - it "returns false" do - expect(note.send(:system_note_viewable_by?, user)).to be_falsey + context "system_note_metadata is of type 'branch'" do + let(:action) { "branch" } + + context "user doesn't have :download_code ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :download_code ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end end end - context "user has the :download_code ability" do - it "returns true" do - expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + context "system_note_metadata is of type 'contact'" do + let(:action) { "contact" } - expect(note.send(:system_note_viewable_by?, user)).to be_truthy + context "user doesn't have :read_crm_contact ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :read_crm_contact ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :read_crm_contact, note.project.group).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end end end end - describe "system_note_with_references_visible_for?" do + describe "system_note_visible_for?" do let_it_be(:private_user) { create(:user) } let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } } let_it_be(:private_issue) { create(:issue, project: private_project) } @@ -529,11 +550,11 @@ RSpec.describe Note do shared_examples "checks references" do it "returns false" do - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end it "returns true" do - expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy + expect(note.system_note_visible_for?(private_user)).to be_truthy end it "returns true if user visible reference count set" do @@ -541,7 +562,7 @@ RSpec.describe Note do note.total_reference_count = 1 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy + expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy end it "returns false if user visible reference count set but does not match total reference count" do @@ -549,14 +570,14 @@ RSpec.describe Note do note.total_reference_count = 2 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end it "returns false if ref count is 0" do note.user_visible_reference_count = 0 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end end @@ -617,16 +638,16 @@ RSpec.describe Note do let(:note) do create :note, noteable: ext_issue, project: ext_proj, - note: "mentioned in #{ext_proj.owner.to_reference}", + note: "mentioned in #{ext_proj.first_owner.to_reference}", system: true end it "returns true for other users" do - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy + expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy end it "returns true for anonymous users" do - expect(note.system_note_with_references_visible_for?(nil)).to be_truthy + expect(note.system_note_visible_for?(nil)).to be_truthy end end end diff --git a/spec/models/packages/package_file_spec.rb b/spec/models/packages/package_file_spec.rb index a86caa074f1..fd453d8e5a9 100644 --- a/spec/models/packages/package_file_spec.rb +++ b/spec/models/packages/package_file_spec.rb @@ -148,16 +148,6 @@ RSpec.describe Packages::PackageFile, type: :model do it 'does not return them' do expect(described_class.for_helm_with_channel(project, channel)).to contain_exactly(helm_file2) end - - context 'with packages_installable_package_files disabled' do - before do - stub_feature_flags(packages_installable_package_files: false) - end - - it 'returns them' do - expect(described_class.for_helm_with_channel(project, channel)).to contain_exactly(helm_file2, package_file_pending_destruction) - end - end end end @@ -232,16 +222,6 @@ RSpec.describe Packages::PackageFile, type: :model do it 'does not return them' do expect(subject).to contain_exactly(helm_package_file2) end - - context 'with packages_installable_package_files disabled' do - before do - stub_feature_flags(packages_installable_package_files: false) - end - - it 'returns them' do - expect(subject).to contain_exactly(package_file_pending_destruction) - end - end end end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 0735bf25690..2ebc9864d9b 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -36,9 +36,12 @@ RSpec.describe PagesDomain do '123.456.789' => true, '0x12345.com' => true, '0123123' => true, - '_foo.com' => false, + 'a-reserved.com' => true, + 'a.b-reserved.com' => true, 'reserved.com' => false, + '_foo.com' => false, 'a.reserved.com' => false, + 'a.b.reserved.com' => false, nil => false }.each do |value, validity| context "domain #{value.inspect} validity" do diff --git a/spec/models/personal_access_token_spec.rb b/spec/models/personal_access_token_spec.rb index 8cd831d2f85..88206fbf48c 100644 --- a/spec/models/personal_access_token_spec.rb +++ b/spec/models/personal_access_token_spec.rb @@ -22,6 +22,16 @@ RSpec.describe PersonalAccessToken do end describe 'scopes' do + describe '.project_access_tokens' do + let_it_be(:user) { create(:user, :project_bot) } + let_it_be(:project_member) { create(:project_member, user: user) } + let_it_be(:project_access_token) { create(:personal_access_token, user: user) } + + subject { described_class.project_access_token } + + it { is_expected.to contain_exactly(project_access_token) } + end + describe '.for_user' do it 'returns personal access tokens of specified user only' do user_1 = create(:user) diff --git a/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb b/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb new file mode 100644 index 00000000000..7ecb6bb9861 --- /dev/null +++ b/spec/models/preloaders/users_max_access_level_in_projects_preloader_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +RSpec.describe Preloaders::UsersMaxAccessLevelInProjectsPreloader do + let_it_be(:user1) { create(:user) } + let_it_be(:user2) { create(:user) } + + let_it_be(:project_1) { create(:project) } + let_it_be(:project_2) { create(:project) } + let_it_be(:project_3) { create(:project) } + + let(:projects) { [project_1, project_2, project_3] } + let(:users) { [user1, user2] } + + before do + project_1.add_developer(user1) + project_1.add_developer(user2) + + project_2.add_developer(user1) + project_2.add_developer(user2) + + project_3.add_developer(user1) + project_3.add_developer(user2) + end + + context 'preload maximum access level to avoid querying project_authorizations', :request_store do + it 'avoids N+1 queries', :request_store do + Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute + + expect(count_queries).to eq(0) + end + + it 'runs N queries without preloading' do + query_count_without_preload = count_queries + + Preloaders::UsersMaxAccessLevelInProjectsPreloader.new(projects: projects, users: users).execute + count_queries_with_preload = count_queries + + expect(count_queries_with_preload).to be < query_count_without_preload + end + end + + def count_queries + ActiveRecord::QueryRecorder.new do + projects.each do |project| + user1.can?(:read_project, project) + user2.can?(:read_project, project) + end + end.count + end +end diff --git a/spec/models/project_import_state_spec.rb b/spec/models/project_import_state_spec.rb index 843beb4ce23..4ad2446f8d0 100644 --- a/spec/models/project_import_state_spec.rb +++ b/spec/models/project_import_state_spec.rb @@ -79,6 +79,29 @@ RSpec.describe ProjectImportState, type: :model do expect(import_state.last_error).to eq(error_message) end + + it 'removes project import data' do + import_data = ProjectImportData.new(data: { 'test' => 'some data' }) + project = create(:project, import_data: import_data) + import_state = create(:import_state, :started, project: project) + + expect do + import_state.mark_as_failed(error_message) + end.to change { project.reload.import_data }.from(import_data).to(nil) + end + + context 'when remove_import_data_on_failure feature flag is disabled' do + it 'removes project import data' do + stub_feature_flags(remove_import_data_on_failure: false) + + project = create(:project, import_data: ProjectImportData.new(data: { 'test' => 'some data' })) + import_state = create(:import_state, :started, project: project) + + expect do + import_state.mark_as_failed(error_message) + end.not_to change { project.reload.import_data } + end + end end describe '#human_status_name' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 30114d36a06..c487c87db1c 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -236,27 +236,42 @@ RSpec.describe Project, factory_default: :keep do end context 'with project namespaces' do - it 'automatically creates a project namespace' do - project = build(:project, path: 'hopefully-valid-path1') - project.save! + shared_examples 'creates project namespace' do + it 'automatically creates a project namespace' do + project = build(:project, path: 'hopefully-valid-path1') + project.save! - expect(project).to be_persisted - expect(project.project_namespace).to be_persisted - expect(project.project_namespace).to be_in_sync_with_project(project) - end + expect(project).to be_persisted + expect(project.project_namespace).to be_persisted + expect(project.project_namespace).to be_in_sync_with_project(project) + expect(project.reload.project_namespace.traversal_ids).to eq([project.namespace.traversal_ids, project.project_namespace.id].flatten.compact) + end - context 'with FF disabled' do - before do - stub_feature_flags(create_project_namespace_on_project_create: false) + context 'with FF disabled' do + before do + stub_feature_flags(create_project_namespace_on_project_create: false) + end + + it 'does not create a project namespace' do + project = build(:project, path: 'hopefully-valid-path2') + project.save! + + expect(project).to be_persisted + expect(project.project_namespace).to be_nil + end end + end - it 'does not create a project namespace' do - project = build(:project, path: 'hopefully-valid-path2') - project.save! + context 'sync-ing traversal_ids in before_commit callback' do + it_behaves_like 'creates project namespace' + end - expect(project).to be_persisted - expect(project.project_namespace).to be_nil + context 'sync-ing traversal_ids in after_create callback' do + before do + stub_feature_flags(sync_traversal_ids_before_commit: false) end + + it_behaves_like 'creates project namespace' end end end @@ -870,7 +885,88 @@ RSpec.describe Project, factory_default: :keep do end end + describe '#merge_commit_template_or_default' do + let_it_be(:project) { create(:project) } + + it 'returns default merge commit template' do + expect(project.merge_commit_template_or_default).to eq(Project::DEFAULT_MERGE_COMMIT_TEMPLATE) + end + + context 'when merge commit template is set and not nil' do + before do + project.merge_commit_template = '%{description}' + end + + it 'returns current value' do + expect(project.merge_commit_template_or_default).to eq('%{description}') + end + end + end + + describe '#merge_commit_template_or_default=' do + let_it_be(:project) { create(:project) } + + it 'sets template to nil when set to default value' do + project.merge_commit_template_or_default = Project::DEFAULT_MERGE_COMMIT_TEMPLATE + expect(project.merge_commit_template).to be_nil + end + + it 'sets template to nil when set to default value but with CRLF line endings' do + project.merge_commit_template_or_default = "Merge branch '%{source_branch}' into '%{target_branch}'\r\n\r\n%{title}\r\n\r\n%{issues}\r\n\r\nSee merge request %{reference}" + expect(project.merge_commit_template).to be_nil + end + + it 'allows changing template' do + project.merge_commit_template_or_default = '%{description}' + expect(project.merge_commit_template).to eq('%{description}') + end + + it 'allows setting template to nil' do + project.merge_commit_template_or_default = nil + expect(project.merge_commit_template).to be_nil + end + end + + describe '#squash_commit_template_or_default' do + let_it_be(:project) { create(:project) } + + it 'returns default squash commit template' do + expect(project.squash_commit_template_or_default).to eq(Project::DEFAULT_SQUASH_COMMIT_TEMPLATE) + end + + context 'when squash commit template is set and not nil' do + before do + project.squash_commit_template = '%{description}' + end + + it 'returns current value' do + expect(project.squash_commit_template_or_default).to eq('%{description}') + end + end + end + + describe '#squash_commit_template_or_default=' do + let_it_be(:project) { create(:project) } + + it 'sets template to nil when set to default value' do + project.squash_commit_template_or_default = Project::DEFAULT_SQUASH_COMMIT_TEMPLATE + expect(project.squash_commit_template).to be_nil + end + + it 'allows changing template' do + project.squash_commit_template_or_default = '%{description}' + expect(project.squash_commit_template).to eq('%{description}') + end + + it 'allows setting template to nil' do + project.squash_commit_template_or_default = nil + expect(project.squash_commit_template).to be_nil + end + end + describe 'reference methods' do + # TODO update when we have multiple owners of a project + # https://gitlab.com/gitlab-org/gitlab/-/issues/350605 let_it_be(:owner) { create(:user, name: 'Gitlab') } let_it_be(:namespace) { create(:namespace, name: 'Sample namespace', path: 'sample-namespace', owner: owner) } let_it_be(:project) { create(:project, name: 'Sample project', path: 'sample-project', namespace: namespace) } @@ -2874,7 +2970,7 @@ RSpec.describe Project, factory_default: :keep do end before do - project.repository.rm_branch(project.owner, branch.name) + project.repository.rm_branch(project.first_owner, branch.name) end subject { project.latest_pipeline(branch.name) } @@ -3869,45 +3965,6 @@ RSpec.describe Project, factory_default: :keep do end end - describe '#ci_instance_variables_for' do - let(:project) { build_stubbed(:project) } - - let!(:instance_variable) do - create(:ci_instance_variable, value: 'secret') - end - - let!(:protected_instance_variable) do - create(:ci_instance_variable, :protected, value: 'protected') - end - - subject { project.ci_instance_variables_for(ref: 'ref') } - - before do - stub_application_setting( - default_branch_protection: Gitlab::Access::PROTECTION_NONE) - end - - context 'when the ref is not protected' do - before do - allow(project).to receive(:protected_for?).with('ref').and_return(false) - end - - it 'contains only the CI variables' do - is_expected.to contain_exactly(instance_variable) - end - end - - context 'when the ref is protected' do - before do - allow(project).to receive(:protected_for?).with('ref').and_return(true) - end - - it 'contains all the variables' do - is_expected.to contain_exactly(instance_variable, protected_instance_variable) - end - end - end - describe '#any_lfs_file_locks?', :request_store do let_it_be(:project) { create(:project) } @@ -6238,6 +6295,21 @@ RSpec.describe Project, factory_default: :keep do end end + describe '.for_group_and_its_ancestor_groups' do + it 'returns projects for group and its ancestors' do + group_1 = create(:group) + project_1 = create(:project, namespace: group_1) + group_2 = create(:group, parent: group_1) + project_2 = create(:project, namespace: group_2) + group_3 = create(:group, parent: group_2) + project_3 = create(:project, namespace: group_2) + group_4 = create(:group, parent: group_3) + create(:project, namespace: group_4) + + expect(described_class.for_group_and_its_ancestor_groups(group_3)).to match_array([project_1, project_2, project_3]) + end + end + describe '.deployments' do subject { project.deployments } @@ -7116,6 +7188,29 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to be true } end + describe '#related_group_ids' do + let_it_be(:group) { create(:group) } + let_it_be(:sub_group) { create(:group, parent: group) } + + context 'when associated with a namespace' do + let(:project) { create(:project, namespace: create(:namespace)) } + let!(:linked_group) { create(:project_group_link, project: project).group } + + it 'only includes linked groups' do + expect(project.related_group_ids).to contain_exactly(linked_group.id) + end + end + + context 'when associated with a group' do + let(:project) { create(:project, group: sub_group) } + let!(:linked_group) { create(:project_group_link, project: project).group } + + it 'includes self, ancestors and linked groups' do + expect(project.related_group_ids).to contain_exactly(group.id, sub_group.id, linked_group.id) + end + end + end + describe '#package_already_taken?' do let_it_be(:namespace) { create(:namespace, path: 'test') } let_it_be(:project) { create(:project, :public, namespace: namespace) } @@ -7410,6 +7505,67 @@ RSpec.describe Project, factory_default: :keep do expect(project.reload.topics.map(&:name)).to eq(%w[topic1 topic2 topic3]) end end + + context 'public topics counter' do + let_it_be(:topic_1) { create(:topic, name: 't1') } + let_it_be(:topic_2) { create(:topic, name: 't2') } + let_it_be(:topic_3) { create(:topic, name: 't3') } + + let(:private) { Gitlab::VisibilityLevel::PRIVATE } + let(:internal) { Gitlab::VisibilityLevel::INTERNAL } + let(:public) { Gitlab::VisibilityLevel::PUBLIC } + + subject do + project_updates = { + visibility_level: new_visibility, + topic_list: new_topic_list + }.compact + + project.update!(project_updates) + end + + using RSpec::Parameterized::TableSyntax + + # rubocop:disable Lint/BinaryOperatorWithIdenticalOperands + where(:initial_visibility, :new_visibility, :new_topic_list, :expected_count_changes) do + ref(:private) | nil | 't2, t3' | [0, 0, 0] + ref(:internal) | nil | 't2, t3' | [-1, 0, 1] + ref(:public) | nil | 't2, t3' | [-1, 0, 1] + ref(:private) | ref(:public) | nil | [1, 1, 0] + ref(:private) | ref(:internal) | nil | [1, 1, 0] + ref(:private) | ref(:private) | nil | [0, 0, 0] + ref(:internal) | ref(:public) | nil | [0, 0, 0] + ref(:internal) | ref(:internal) | nil | [0, 0, 0] + ref(:internal) | ref(:private) | nil | [-1, -1, 0] + ref(:public) | ref(:public) | nil | [0, 0, 0] + ref(:public) | ref(:internal) | nil | [0, 0, 0] + ref(:public) | ref(:private) | nil | [-1, -1, 0] + ref(:private) | ref(:public) | 't2, t3' | [0, 1, 1] + ref(:private) | ref(:internal) | 't2, t3' | [0, 1, 1] + ref(:private) | ref(:private) | 't2, t3' | [0, 0, 0] + ref(:internal) | ref(:public) | 't2, t3' | [-1, 0, 1] + ref(:internal) | ref(:internal) | 't2, t3' | [-1, 0, 1] + ref(:internal) | ref(:private) | 't2, t3' | [-1, -1, 0] + ref(:public) | ref(:public) | 't2, t3' | [-1, 0, 1] + ref(:public) | ref(:internal) | 't2, t3' | [-1, 0, 1] + ref(:public) | ref(:private) | 't2, t3' | [-1, -1, 0] + end + # rubocop:enable Lint/BinaryOperatorWithIdenticalOperands + + with_them do + it 'increments or decrements counters of topics' do + project.reload.update!( + visibility_level: initial_visibility, + topic_list: [topic_1.name, topic_2.name] + ) + + expect { subject } + .to change { topic_1.reload.non_private_projects_count }.by(expected_count_changes[0]) + .and change { topic_2.reload.non_private_projects_count }.by(expected_count_changes[1]) + .and change { topic_3.reload.non_private_projects_count }.by(expected_count_changes[2]) + end + end + end end shared_examples 'all_runners' do @@ -7801,7 +7957,8 @@ RSpec.describe Project, factory_default: :keep do end describe '#context_commits_enabled?' do - let_it_be(:project) { create(:project) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, namespace: group) } subject(:result) { project.context_commits_enabled? } @@ -7821,19 +7978,19 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to be_falsey } end - context 'when context_commits feature flag is enabled on this project' do + context 'when context_commits feature flag is enabled on project group' do before do - stub_feature_flags(context_commits: project) + stub_feature_flags(context_commits: group) end it { is_expected.to be_truthy } end - context 'when context_commits feature flag is enabled on another project' do - let(:another_project) { create(:project) } + context 'when context_commits feature flag is enabled on another group' do + let(:another_group) { create(:group) } before do - stub_feature_flags(context_commits: another_project) + stub_feature_flags(context_commits: another_group) end it { is_expected.to be_falsey } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index c0bad96effc..bfdebbc33df 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -77,15 +77,43 @@ RSpec.describe ProjectTeam do end end + describe 'owner methods' do + context 'personal project' do + let(:project) { create(:project) } + let(:owner) { project.first_owner } + + specify { expect(project.team.owners).to contain_exactly(owner) } + specify { expect(project.team.owner?(owner)).to be_truthy } + end + + context 'group project' do + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + let(:user1) { create(:user) } + let(:user2) { create(:user) } + + before do + group.add_owner(user1) + group.add_owner(user2) + end + + specify { expect(project.team.owners).to contain_exactly(user1, user2) } + specify { expect(project.team.owner?(user1)).to be_truthy } + specify { expect(project.team.owner?(user2)).to be_truthy } + end + end + describe '#fetch_members' do context 'personal project' do let(:project) { create(:project) } it 'returns project members' do + # TODO this can be updated when we have multiple project owners + # See https://gitlab.com/gitlab-org/gitlab/-/issues/350605 user = create(:user) project.add_guest(user) - expect(project.team.members).to contain_exactly(user, project.owner) + expect(project.team.members).to contain_exactly(user, project.first_owner) end it 'returns project members of a specified level' do @@ -103,7 +131,7 @@ RSpec.describe ProjectTeam do group_access: Gitlab::Access::GUEST) expect(project.team.members) - .to contain_exactly(group_member.user, project.owner) + .to contain_exactly(group_member.user, project.first_owner) end it 'returns invited members of a group of a specified level' do diff --git a/spec/models/state_note_spec.rb b/spec/models/state_note_spec.rb index bd07af7ceca..e91150695b0 100644 --- a/spec/models/state_note_spec.rb +++ b/spec/models/state_note_spec.rb @@ -55,7 +55,7 @@ RSpec.describe StateNote do it 'contains the expected values' do expect(subject.author).to eq(author) expect(subject.created_at).to eq(event.created_at) - expect(subject.note).to eq('resolved the corresponding error and closed the issue.') + expect(subject.note).to eq('resolved the corresponding error and closed the issue') end end @@ -65,7 +65,7 @@ RSpec.describe StateNote do it 'contains the expected values' do expect(subject.author).to eq(author) expect(subject.created_at).to eq(event.created_at) - expect(subject.note).to eq('automatically closed this issue because the alert resolved.') + expect(subject.note).to eq('automatically closed this incident because the alert resolved') end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c2535fd3698..e4f25c79e53 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2612,6 +2612,12 @@ RSpec.describe User do it 'returns users with a exact matching name shorter than 3 chars regardless of the casing' do expect(described_class.search(user3.name.upcase)).to eq([user3]) end + + context 'when use_minimum_char_limit is false' do + it 'returns users with a partially matching name' do + expect(described_class.search('u', use_minimum_char_limit: false)).to eq([user3, user, user2]) + end + end end describe 'email matching' do @@ -2671,204 +2677,20 @@ RSpec.describe User do it 'returns users with a exact matching username shorter than 3 chars regardless of the casing' do expect(described_class.search(user3.username.upcase)).to eq([user3]) end - end - - it 'returns no matches for an empty string' do - expect(described_class.search('')).to be_empty - end - - it 'returns no matches for nil' do - expect(described_class.search(nil)).to be_empty - end - end - - describe '.search_without_secondary_emails' do - let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) } - let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) } - let_it_be(:email) { create(:email, user: another_user, email: 'alias@example.com') } - - it 'returns users with a matching name' do - expect(described_class.search_without_secondary_emails(user.name)).to eq([user]) - end - - it 'returns users with a partially matching name' do - expect(described_class.search_without_secondary_emails(user.name[0..2])).to eq([user]) - end - - it 'returns users with a matching name regardless of the casing' do - expect(described_class.search_without_secondary_emails(user.name.upcase)).to eq([user]) - end - - it 'returns users with a matching email' do - expect(described_class.search_without_secondary_emails(user.email)).to eq([user]) - end - - it 'does not return users with a partially matching email' do - expect(described_class.search_without_secondary_emails(user.email[1...-1])).to be_empty - end - - it 'returns users with a matching email regardless of the casing' do - expect(described_class.search_without_secondary_emails(user.email.upcase)).to eq([user]) - end - - it 'returns users with a matching username' do - expect(described_class.search_without_secondary_emails(user.username)).to eq([user]) - end - - it 'returns users with a partially matching username' do - expect(described_class.search_without_secondary_emails(user.username[0..2])).to eq([user]) - end - - it 'returns users with a matching username regardless of the casing' do - expect(described_class.search_without_secondary_emails(user.username.upcase)).to eq([user]) - end - - it 'does not return users with a matching whole secondary email' do - expect(described_class.search_without_secondary_emails(email.email)).not_to include(email.user) - end - it 'does not return users with a matching part of secondary email' do - expect(described_class.search_without_secondary_emails(email.email[1...-1])).to be_empty - end - - it 'returns no matches for an empty string' do - expect(described_class.search_without_secondary_emails('')).to be_empty - end - - it 'returns no matches for nil' do - expect(described_class.search_without_secondary_emails(nil)).to be_empty - end - end - - describe '.search_with_public_emails' do - let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) } - let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) } - let_it_be(:public_email) do - create(:email, :confirmed, user: another_user, email: 'alias@example.com').tap do |email| - another_user.update!(public_email: email.email) + context 'when use_minimum_char_limit is false' do + it 'returns users with a partially matching username' do + expect(described_class.search('se', use_minimum_char_limit: false)).to eq([user3, user, user2]) + end end end - let_it_be(:secondary_email) do - create(:email, :confirmed, user: another_user, email: 'secondary@example.com') - end - - it 'returns users with a matching name' do - expect(described_class.search_with_public_emails(user.name)).to match_array([user]) - end - - it 'returns users with a partially matching name' do - expect(described_class.search_with_public_emails(user.name[0..2])).to match_array([user]) - end - - it 'returns users with a matching name regardless of the casing' do - expect(described_class.search_with_public_emails(user.name.upcase)).to match_array([user]) - end - - it 'returns users with a matching public email' do - expect(described_class.search_with_public_emails(another_user.public_email)).to match_array([another_user]) - end - - it 'does not return users with a partially matching email' do - expect(described_class.search_with_public_emails(another_user.public_email[1...-1])).to be_empty - end - - it 'returns users with a matching email regardless of the casing' do - expect(described_class.search_with_public_emails(another_user.public_email.upcase)).to match_array([another_user]) - end - - it 'returns users with a matching username' do - expect(described_class.search_with_public_emails(user.username)).to match_array([user]) - end - - it 'returns users with a partially matching username' do - expect(described_class.search_with_public_emails(user.username[0..2])).to match_array([user]) - end - - it 'returns users with a matching username regardless of the casing' do - expect(described_class.search_with_public_emails(user.username.upcase)).to match_array([user]) - end - - it 'does not return users with a matching whole private email' do - expect(described_class.search_with_public_emails(user.email)).not_to include(user) - end - - it 'does not return users with a matching whole private email' do - expect(described_class.search_with_public_emails(secondary_email.email)).to be_empty - end - - it 'does not return users with a matching part of secondary email' do - expect(described_class.search_with_public_emails(secondary_email.email[1...-1])).to be_empty - end - - it 'does not return users with a matching part of private email' do - expect(described_class.search_with_public_emails(user.email[1...-1])).to be_empty - end - - it 'returns no matches for an empty string' do - expect(described_class.search_with_public_emails('')).to be_empty - end - - it 'returns no matches for nil' do - expect(described_class.search_with_public_emails(nil)).to be_empty - end - end - - describe '.search_with_secondary_emails' do - let_it_be(:user) { create(:user, name: 'John Doe', username: 'john.doe', email: 'someone.1@example.com' ) } - let_it_be(:another_user) { create(:user, name: 'Albert Smith', username: 'albert.smith', email: 'another.2@example.com' ) } - let_it_be(:email) { create(:email, user: another_user, email: 'alias@example.com') } - - it 'returns users with a matching name' do - expect(described_class.search_with_secondary_emails(user.name)).to eq([user]) - end - - it 'returns users with a partially matching name' do - expect(described_class.search_with_secondary_emails(user.name[0..2])).to eq([user]) - end - - it 'returns users with a matching name regardless of the casing' do - expect(described_class.search_with_secondary_emails(user.name.upcase)).to eq([user]) - end - - it 'returns users with a matching email' do - expect(described_class.search_with_secondary_emails(user.email)).to eq([user]) - end - - it 'does not return users with a partially matching email' do - expect(described_class.search_with_secondary_emails(user.email[1...-1])).to be_empty - end - - it 'returns users with a matching email regardless of the casing' do - expect(described_class.search_with_secondary_emails(user.email.upcase)).to eq([user]) - end - - it 'returns users with a matching username' do - expect(described_class.search_with_secondary_emails(user.username)).to eq([user]) - end - - it 'returns users with a partially matching username' do - expect(described_class.search_with_secondary_emails(user.username[0..2])).to eq([user]) - end - - it 'returns users with a matching username regardless of the casing' do - expect(described_class.search_with_secondary_emails(user.username.upcase)).to eq([user]) - end - - it 'returns users with a matching whole secondary email' do - expect(described_class.search_with_secondary_emails(email.email)).to eq([email.user]) - end - - it 'does not return users with a matching part of secondary email' do - expect(described_class.search_with_secondary_emails(email.email[1...-1])).to be_empty - end - it 'returns no matches for an empty string' do - expect(described_class.search_with_secondary_emails('')).to be_empty + expect(described_class.search('')).to be_empty end it 'returns no matches for nil' do - expect(described_class.search_with_secondary_emails(nil)).to be_empty + expect(described_class.search(nil)).to be_empty end end @@ -3683,13 +3505,16 @@ RSpec.describe User do let!(:project1) { create(:project) } let!(:project2) { fork_project(project3) } let!(:project3) { create(:project) } + let!(:project_aimed_for_deletion) { create(:project, marked_for_deletion_at: 2.days.ago, pending_delete: false) } let!(:merge_request) { create(:merge_request, source_project: project2, target_project: project3, author: subject) } let!(:push_event) { create(:push_event, project: project1, author: subject) } let!(:merge_event) { create(:event, :created, project: project3, target: merge_request, author: subject) } + let!(:merge_event_2) { create(:event, :created, project: project_aimed_for_deletion, target: merge_request, author: subject) } before do project1.add_maintainer(subject) project2.add_maintainer(subject) + project_aimed_for_deletion.add_maintainer(subject) end it 'includes IDs for projects the user has pushed to' do @@ -3703,6 +3528,10 @@ RSpec.describe User do it "doesn't include IDs for unrelated projects" do expect(subject.contributed_projects).not_to include(project2) end + + it "doesn't include projects aimed for deletion" do + expect(subject.contributed_projects).not_to include(project_aimed_for_deletion) + end end describe '#fork_of' do @@ -4365,6 +4194,8 @@ RSpec.describe User do context 'when FF ci_owned_runners_cross_joins_fix is disabled' do before do + skip_if_multiple_databases_are_setup + stub_feature_flags(ci_owned_runners_cross_joins_fix: false) end diff --git a/spec/models/work_item_spec.rb b/spec/models/work_item_spec.rb new file mode 100644 index 00000000000..2fa1abda44a --- /dev/null +++ b/spec/models/work_item_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItem do + describe '#noteable_target_type_name' do + it 'returns `issue` as the target name' do + work_item = build(:work_item) + + expect(work_item.noteable_target_type_name).to eq('issue') + end + end +end |