diff options
Diffstat (limited to 'spec/models')
94 files changed, 3273 insertions, 1577 deletions
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 5bd69ad9fad..422dd9a463b 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -151,6 +151,38 @@ RSpec.describe Ability do end end + describe '.users_that_can_read_internal_note' do + shared_examples 'filtering users that can read internal note' do + let_it_be(:guest) { create(:user) } + let_it_be(:reporter) { create(:user) } + + let(:users) { [reporter, guest] } + + before do + parent.add_guest(guest) + parent.add_reporter(reporter) + end + + it 'returns users that can read internal notes' do + result = described_class.users_that_can_read_internal_notes(users, parent) + + expect(result).to match_array([reporter]) + end + end + + context 'for groups' do + it_behaves_like 'filtering users that can read internal note' do + let(:parent) { create(:group) } + end + end + + context 'for projects' do + it_behaves_like 'filtering users that can read internal note' do + let(:parent) { create(:project) } + end + end + end + describe '.merge_requests_readable_by_user' do context 'with an admin when admin mode is enabled', :enable_admin_mode do it 'returns all merge requests' do diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 61f008416ea..0b3521cdd0c 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -104,6 +104,9 @@ RSpec.describe ApplicationSetting do it { is_expected.to validate_numericality_of(:packages_cleanup_package_file_worker_capacity).only_integer.is_greater_than_or_equal_to(0) } it { is_expected.not_to allow_value(nil).for(:packages_cleanup_package_file_worker_capacity) } + it { is_expected.to validate_numericality_of(:package_registry_cleanup_policies_worker_capacity).only_integer.is_greater_than_or_equal_to(0) } + it { is_expected.not_to allow_value(nil).for(:package_registry_cleanup_policies_worker_capacity) } + it { is_expected.to validate_numericality_of(:snippet_size_limit).only_integer.is_greater_than(0) } it { is_expected.to validate_numericality_of(:wiki_page_max_content_bytes).only_integer.is_greater_than_or_equal_to(1024) } it { is_expected.to validate_presence_of(:max_artifacts_size) } @@ -305,18 +308,20 @@ RSpec.describe ApplicationSetting do end end - context 'when snowplow is enabled' do - before do - setting.snowplow_enabled = true - end + describe 'snowplow settings', :do_not_stub_snowplow_by_default do + context 'when snowplow is enabled' do + before do + setting.snowplow_enabled = true + end - it { is_expected.not_to allow_value(nil).for(:snowplow_collector_hostname) } - it { is_expected.to allow_value("snowplow.gitlab.com").for(:snowplow_collector_hostname) } - it { is_expected.not_to allow_value('/example').for(:snowplow_collector_hostname) } - end + it { is_expected.not_to allow_value(nil).for(:snowplow_collector_hostname) } + it { is_expected.to allow_value("snowplow.gitlab.com").for(:snowplow_collector_hostname) } + it { is_expected.not_to allow_value('/example').for(:snowplow_collector_hostname) } + end - context 'when snowplow is not enabled' do - it { is_expected.to allow_value(nil).for(:snowplow_collector_hostname) } + context 'when snowplow is not enabled' do + it { is_expected.to allow_value(nil).for(:snowplow_collector_hostname) } + end end context 'when mailgun_events_enabled is enabled' do @@ -1065,6 +1070,35 @@ RSpec.describe ApplicationSetting do it { is_expected.to validate_numericality_of(:sidekiq_job_limiter_compression_threshold_bytes).only_integer.is_greater_than_or_equal_to(0) } it { is_expected.to validate_numericality_of(:sidekiq_job_limiter_limit_bytes).only_integer.is_greater_than_or_equal_to(0) } end + + context 'prometheus settings' do + it 'validates metrics_method_call_threshold' do + allow(subject).to receive(:prometheus_metrics_enabled).and_return(true) + + is_expected.to validate_numericality_of(:metrics_method_call_threshold).is_greater_than_or_equal_to(0) + end + end + + context 'error tracking settings' do + context 'with error tracking disabled' do + before do + setting.error_tracking_enabled = false + end + + it { is_expected.to allow_value(nil).for(:error_tracking_api_url) } + end + + context 'with error tracking enabled' do + before do + setting.error_tracking_enabled = true + end + + it { is_expected.to allow_value(http).for(:error_tracking_api_url) } + it { is_expected.to allow_value(https).for(:error_tracking_api_url) } + it { is_expected.not_to allow_value(ftp).for(:error_tracking_api_url) } + it { is_expected.to validate_presence_of(:error_tracking_api_url) } + end + end end context 'restrict creating duplicates' do diff --git a/spec/models/authentication_event_spec.rb b/spec/models/authentication_event_spec.rb index 83598fa6765..23e253c2a28 100644 --- a/spec/models/authentication_event_spec.rb +++ b/spec/models/authentication_event_spec.rb @@ -44,4 +44,31 @@ RSpec.describe AuthenticationEvent do expect(described_class.providers).to match_array %w(ldapmain google_oauth2 standard two-factor two-factor-via-u2f-device two-factor-via-webauthn-device) end end + + describe '.initial_login_or_known_ip_address?' do + let_it_be(:user) { create(:user) } + let_it_be(:ip_address) { '127.0.0.1' } + + subject { described_class.initial_login_or_known_ip_address?(user, ip_address) } + + context 'on first login, when no record exists yet' do + it { is_expected.to eq(true) } + end + + context 'on second login from the same ip address' do + before do + create(:authentication_event, :successful, user: user, ip_address: ip_address) + end + + it { is_expected.to eq(true) } + end + + context 'on second login from another ip address' do + before do + create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') + end + + it { is_expected.to eq(false) } + end + end end diff --git a/spec/models/awareness_session_spec.rb b/spec/models/awareness_session_spec.rb new file mode 100644 index 00000000000..854ce5957f7 --- /dev/null +++ b/spec/models/awareness_session_spec.rb @@ -0,0 +1,163 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AwarenessSession, :clean_gitlab_redis_shared_state do + subject { AwarenessSession.for(session_id) } + + let!(:user) { create(:user) } + let(:session_id) { 1 } + + describe "when initiating a session" do + it "provides a string representation of the model instance" do + expected = "awareness_session=6b86b273ff34fce" + + expect(subject.to_s).to eql(expected) + end + + it "provides a parameterized version of the session identifier" do + expected = "6b86b273ff34fce" + + expect(subject.to_param).to eql(expected) + end + end + + describe "when a user joins a session" do + let(:user2) { create(:user) } + + let(:presence_ttl) { 15.minutes } + + it "changes number of session members" do + expect { subject.join(user) }.to change(subject, :size).by(1) + end + + it "returns user as member of session with last_activity timestamp" do + freeze_time do + subject.join(user) + + session_users = subject.users_with_last_activity + session_user, last_activity = session_users.first + + expect(session_user.id).to be(user.id) + expect(last_activity).to be_eql(Time.now.utc) + end + end + + it "maintains user ID and last_activity pairs" do + now = Time.zone.now + + travel_to now - 1.minute do + subject.join(user2) + end + + travel_to now do + subject.join(user) + end + + session_users = subject.users_with_last_activity + + expect(session_users[0].first.id).to eql(user.id) + expect(session_users[0].last.to_i).to eql(now.to_i) + + expect(session_users[1].first.id).to eql(user2.id) + expect(session_users[1].last.to_i).to eql((now - 1.minute).to_i) + end + + it "reports user as present" do + freeze_time do + subject.join(user) + + expect(subject.present?(user, threshold: presence_ttl)).to be true + end + end + + it "reports user as away after a certain time on inactivity" do + subject.join(user) + + travel_to((presence_ttl + 1.minute).from_now) do + expect(subject.away?(user, threshold: presence_ttl)).to be true + end + end + + it "reports user as present still when there was some activity" do + subject.join(user) + + travel_to((presence_ttl - 1.minute).from_now) do + subject.touch!(user) + end + + travel_to((presence_ttl + 1.minute).from_now) do + expect(subject.present?(user, threshold: presence_ttl)).to be true + end + end + + it "creates user and session awareness keys in store" do + subject.join(user) + + Gitlab::Redis::SharedState.with do |redis| + keys = redis.scan_each(match: "gitlab:awareness:*").to_a + + expect(keys.size).to be(2) + end + end + + it "sets a timeout for user and session key" do + subject.join(user) + subject_id = Digest::SHA256.hexdigest(session_id.to_s)[0, 15] + + Gitlab::Redis::SharedState.with do |redis| + ttl_session = redis.ttl("gitlab:awareness:session:#{subject_id}:users") + ttl_user = redis.ttl("gitlab:awareness:user:#{user.id}:sessions") + + expect(ttl_session).to be > 0 + expect(ttl_user).to be > 0 + end + end + + it "fetches user(s) from database" do + subject.join(user) + + expect(subject.users.first).to eql(user) + end + + it "fetches and filters online user(s) from database" do + subject.join(user) + + travel 2.hours do + subject.join(user2) + + online_users = subject.online_users_with_last_activity + online_user, _ = online_users.first + + expect(online_users.size).to be 1 + expect(online_user).to eql(user2) + end + end + end + + describe "when a user leaves a session" do + it "changes number of session members" do + subject.join(user) + + expect { subject.leave(user) }.to change(subject, :size).by(-1) + end + + it "destroys the session when it was the last user" do + subject.join(user) + + expect { subject.leave(user) }.to change(subject, :id).to(nil) + end + end + + describe "when last user leaves a session" do + it "session and user keys are removed" do + subject.join(user) + + Gitlab::Redis::SharedState.with do |redis| + expect { subject.leave(user) } + .to change { redis.scan_each(match: "gitlab:awareness:*").to_a.size } + .to(0) + end + end + end +end diff --git a/spec/models/ci/build_report_result_spec.rb b/spec/models/ci/build_report_result_spec.rb index 3f53c6c1c0e..09ea19cf077 100644 --- a/spec/models/ci/build_report_result_spec.rb +++ b/spec/models/ci/build_report_result_spec.rb @@ -70,10 +70,4 @@ RSpec.describe Ci::BuildReportResult do expect(build_report_result.tests_skipped).to eq(0) end end - - describe '#tests_total' do - it 'returns the total count' do - expect(build_report_result.tests_total).to eq(2) - end - end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 6ad6bb16eb5..e0166ba64a4 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -43,30 +43,10 @@ RSpec.describe Ci::Build do it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) } shared_examples 'calling proper BuildFinishedWorker' do - context 'when ci_build_finished_worker_namespace_changed feature flag enabled' do - before do - stub_feature_flags(ci_build_finished_worker_namespace_changed: build.project) - end - - it 'calls Ci::BuildFinishedWorker' do - expect(Ci::BuildFinishedWorker).to receive(:perform_async) - expect(::BuildFinishedWorker).not_to receive(:perform_async) - - subject - end - end - - context 'when ci_build_finished_worker_namespace_changed feature flag disabled' do - before do - stub_feature_flags(ci_build_finished_worker_namespace_changed: false) - end - - it 'calls ::BuildFinishedWorker' do - expect(::BuildFinishedWorker).to receive(:perform_async) - expect(Ci::BuildFinishedWorker).not_to receive(:perform_async) + it 'calls Ci::BuildFinishedWorker' do + expect(Ci::BuildFinishedWorker).to receive(:perform_async) - subject - end + subject end end @@ -1364,7 +1344,7 @@ RSpec.describe Ci::Build do before do allow(Deployments::LinkMergeRequestWorker).to receive(:perform_async) - allow(deployment).to receive(:execute_hooks) + allow(Deployments::HooksWorker).to receive(:perform_async) end it 'has deployments record with created status' do @@ -1420,7 +1400,7 @@ RSpec.describe Ci::Build do before do allow(Deployments::UpdateEnvironmentWorker).to receive(:perform_async) - allow(deployment).to receive(:execute_hooks) + allow(Deployments::HooksWorker).to receive(:perform_async) end it_behaves_like 'avoid deadlock' @@ -1506,28 +1486,14 @@ RSpec.describe Ci::Build do it 'transitions to running and calls webhook' do freeze_time do - expect(deployment).to receive(:execute_hooks).with(Time.current) + expect(Deployments::HooksWorker) + .to receive(:perform_async).with(deployment_id: deployment.id, status: 'running', status_changed_at: Time.current) subject end expect(deployment).to be_running end - - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - freeze_time do - expect(Deployments::HooksWorker) - .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current) - - subject - end - end - end end end end @@ -1567,48 +1533,47 @@ RSpec.describe Ci::Build do end end - describe 'deployment' do - describe '#outdated_deployment?' do - subject { build.outdated_deployment? } - - context 'when build succeeded' do - let(:build) { create(:ci_build, :success) } - let!(:deployment) { create(:deployment, :success, deployable: build) } + describe 'environment' do + describe '#has_environment?' do + subject { build.has_environment? } - context 'current deployment is latest' do - it { is_expected.to be_falsey } + context 'when environment is defined' do + before do + build.update!(environment: 'review') end - context 'current deployment is not latest on environment' do - let!(:deployment2) { create(:deployment, :success, environment: deployment.environment) } - - it { is_expected.to be_truthy } - end + it { is_expected.to be_truthy } end - context 'when build failed' do - let(:build) { create(:ci_build, :failed) } + context 'when environment is not defined' do + before do + build.update!(environment: nil) + end it { is_expected.to be_falsey } end end - end - describe 'environment' do - describe '#has_environment?' do - subject { build.has_environment? } + describe '#count_user_verification?' do + subject { build.count_user_verification? } - context 'when environment is defined' do - before do - build.update!(environment: 'review') + context 'when build is the verify action for the environment' do + let(:build) do + create(:ci_build, + ref: 'master', + environment: 'staging', + options: { environment: { action: 'verify' } }) end it { is_expected.to be_truthy } end - context 'when environment is not defined' do - before do - build.update!(environment: nil) + context 'when build is not the verify action for the environment' do + let(:build) do + create(:ci_build, + ref: 'master', + environment: 'staging', + options: { environment: { action: 'start' } }) end it { is_expected.to be_falsey } @@ -1975,16 +1940,6 @@ RSpec.describe Ci::Build do end end - describe '#first_pending' do - let!(:first) { create(:ci_build, pipeline: pipeline, status: 'pending', created_at: Date.yesterday) } - let!(:second) { create(:ci_build, pipeline: pipeline, status: 'pending') } - - subject { described_class.first_pending } - - it { is_expected.to be_a(described_class) } - it('returns with the first pending build') { is_expected.to eq(first) } - end - describe '#failed_but_allowed?' do subject { build.failed_but_allowed? } @@ -2134,6 +2089,34 @@ RSpec.describe Ci::Build do end end + describe '#save_tags' do + let(:build) { create(:ci_build, tag_list: ['tag']) } + + it 'saves tags' do + build.save! + + expect(build.tags.count).to eq(1) + expect(build.tags.first.name).to eq('tag') + end + + it 'strips tags' do + build.tag_list = [' taga', 'tagb ', ' tagc '] + + build.save! + expect(build.tags.map(&:name)).to match_array(%w[taga tagb tagc]) + end + + context 'with BulkInsertableTags.with_bulk_insert_tags' do + it 'does not save_tags' do + Ci::BulkInsertableTags.with_bulk_insert_tags do + build.save! + end + + expect(build.tags).to be_empty + end + end + end + describe '#has_tags?' do context 'when build has tags' do subject { create(:ci_build, tag_list: ['tag']) } @@ -2641,7 +2624,7 @@ RSpec.describe Ci::Build do context 'when token is empty' do before do - build.update_columns(token: nil, token_encrypted: nil) + build.update_columns(token_encrypted: nil) end it { is_expected.to be_nil} @@ -3294,10 +3277,6 @@ RSpec.describe Ci::Build do let(:trigger) { create(:ci_trigger, project: project) } let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } - let(:user_trigger_variable) do - { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false, masked: false } - end - let(:predefined_trigger_variable) do { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true, masked: false } end @@ -3306,26 +3285,7 @@ RSpec.describe Ci::Build do build.trigger_request = trigger_request end - shared_examples 'returns variables for triggers' do - it { is_expected.to include(user_trigger_variable) } - it { is_expected.to include(predefined_trigger_variable) } - end - - context 'when variables are stored in trigger_request' do - before do - trigger_request.update_attribute(:variables, { 'TRIGGER_KEY_1' => 'TRIGGER_VALUE_1' } ) - end - - it_behaves_like 'returns variables for triggers' - end - - context 'when variables are stored in pipeline_variables' do - before do - create(:ci_pipeline_variable, pipeline: pipeline, key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1') - end - - it_behaves_like 'returns variables for triggers' - end + it { is_expected.to include(predefined_trigger_variable) } end context 'when pipeline has a variable' do @@ -3848,7 +3808,7 @@ RSpec.describe Ci::Build do end it 'queues BuildHooksWorker' do - expect(BuildHooksWorker).to receive(:perform_async).with(build.id) + expect(BuildHooksWorker).to receive(:perform_async).with(build) build.enqueue end @@ -4321,7 +4281,7 @@ RSpec.describe Ci::Build do describe '#collect_test_reports!' do subject { build.collect_test_reports!(test_reports) } - let(:test_reports) { Gitlab::Ci::Reports::TestReports.new } + let(:test_reports) { Gitlab::Ci::Reports::TestReport.new } it { expect(test_reports.get_suite(build.name).total_count).to eq(0) } @@ -4372,7 +4332,7 @@ RSpec.describe Ci::Build do context 'when build is part of parallel build' do let(:build_1) { create(:ci_build, name: 'build 1/2') } - let(:test_report) { Gitlab::Ci::Reports::TestReports.new } + let(:test_report) { Gitlab::Ci::Reports::TestReport.new } before do build_1.collect_test_reports!(test_report) @@ -4396,7 +4356,7 @@ RSpec.describe Ci::Build do end context 'when build is part of matrix build' do - let(:test_report) { Gitlab::Ci::Reports::TestReports.new } + let(:test_report) { Gitlab::Ci::Reports::TestReport.new } let(:matrix_build_1) { create(:ci_build, :matrix) } before do diff --git a/spec/models/ci/group_spec.rb b/spec/models/ci/group_spec.rb index 6c96e659a34..4900bc792af 100644 --- a/spec/models/ci/group_spec.rb +++ b/spec/models/ci/group_spec.rb @@ -70,7 +70,7 @@ RSpec.describe Ci::Group do describe '.fabricate' do let(:pipeline) { create(:ci_empty_pipeline) } - let(:stage) { create(:ci_stage_entity, pipeline: pipeline) } + let(:stage) { create(:ci_stage, pipeline: pipeline) } before do create_build(:ci_build, name: 'rspec 0 2') diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 3a4b836e453..fc5a9c879f6 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -56,4 +56,10 @@ RSpec.describe Ci::GroupVariable do let!(:parent) { model.group } end + + describe '#audit_details' do + it "equals to the group variable's key" do + expect(subject.audit_details).to eq(subject.key) + end + end end diff --git a/spec/models/ci/legacy_stage_spec.rb b/spec/models/ci/legacy_stage_spec.rb deleted file mode 100644 index 2487ad85889..00000000000 --- a/spec/models/ci/legacy_stage_spec.rb +++ /dev/null @@ -1,268 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Ci::LegacyStage do - let(:stage) { build(:ci_stage) } - let(:pipeline) { stage.pipeline } - let(:stage_name) { stage.name } - - describe '#expectations' do - subject { stage } - - it { is_expected.to include_module(StaticModel) } - - it { is_expected.to respond_to(:pipeline) } - it { is_expected.to respond_to(:name) } - - it { is_expected.to delegate_method(:project).to(:pipeline) } - end - - describe '#statuses' do - let!(:stage_build) { create_job(:ci_build) } - let!(:commit_status) { create_job(:commit_status) } - let!(:other_build) { create_job(:ci_build, stage: 'other stage') } - - subject { stage.statuses } - - it "returns only matching statuses" do - is_expected.to contain_exactly(stage_build, commit_status) - end - end - - describe '#groups' do - before do - create_job(:ci_build, name: 'rspec 0 2') - create_job(:ci_build, name: 'rspec 0 1') - create_job(:ci_build, name: 'spinach 0 1') - create_job(:commit_status, name: 'aaaaa') - end - - it 'returns an array of three groups' do - expect(stage.groups).to be_a Array - expect(stage.groups).to all(be_a Ci::Group) - expect(stage.groups.size).to eq 3 - end - - it 'returns groups with correctly ordered statuses' do - expect(stage.groups.first.jobs.map(&:name)) - .to eq ['aaaaa'] - expect(stage.groups.second.jobs.map(&:name)) - .to eq ['rspec 0 1', 'rspec 0 2'] - expect(stage.groups.third.jobs.map(&:name)) - .to eq ['spinach 0 1'] - end - - it 'returns groups with correct names' do - expect(stage.groups.map(&:name)) - .to eq %w[aaaaa rspec spinach] - end - - context 'when a name is nil on legacy pipelines' do - before do - pipeline.builds.first.update_attribute(:name, nil) - end - - it 'returns an array of three groups' do - expect(stage.groups.map(&:name)) - .to eq ['', 'aaaaa', 'rspec', 'spinach'] - end - end - end - - describe '#statuses_count' do - before do - create_job(:ci_build) - create_job(:ci_build, stage: 'other stage') - end - - subject { stage.statuses_count } - - it "counts statuses only from current stage" do - is_expected.to eq(1) - end - end - - describe '#builds' do - let!(:stage_build) { create_job(:ci_build) } - let!(:commit_status) { create_job(:commit_status) } - - subject { stage.builds } - - it "returns only builds" do - is_expected.to contain_exactly(stage_build) - end - end - - describe '#status' do - subject { stage.status } - - context 'if status is already defined' do - let(:stage) { build(:ci_stage, status: 'success') } - - it "returns defined status" do - is_expected.to eq('success') - end - end - - context 'if status has to be calculated' do - let!(:stage_build) { create_job(:ci_build, status: :failed) } - - it "returns status of a build" do - is_expected.to eq('failed') - end - - context 'and builds are retried' do - let!(:new_build) { create_job(:ci_build, status: :success) } - - before do - stage_build.update!(retried: true) - end - - it "returns status of latest build" do - is_expected.to eq('success') - end - end - end - end - - describe '#detailed_status' do - let(:user) { create(:user) } - - subject { stage.detailed_status(user) } - - context 'when build is created' do - let!(:stage_build) { create_job(:ci_build, status: :created) } - - it 'returns detailed status for created stage' do - expect(subject.text).to eq s_('CiStatusText|created') - end - end - - context 'when build is pending' do - let!(:stage_build) { create_job(:ci_build, status: :pending) } - - it 'returns detailed status for pending stage' do - expect(subject.text).to eq s_('CiStatusText|pending') - end - end - - context 'when build is running' do - let!(:stage_build) { create_job(:ci_build, status: :running) } - - it 'returns detailed status for running stage' do - expect(subject.text).to eq s_('CiStatus|running') - end - end - - context 'when build is successful' do - let!(:stage_build) { create_job(:ci_build, status: :success) } - - it 'returns detailed status for successful stage' do - expect(subject.text).to eq s_('CiStatusText|passed') - end - end - - context 'when build is failed' do - let!(:stage_build) { create_job(:ci_build, status: :failed) } - - it 'returns detailed status for failed stage' do - expect(subject.text).to eq s_('CiStatusText|failed') - end - end - - context 'when build is canceled' do - let!(:stage_build) { create_job(:ci_build, status: :canceled) } - - it 'returns detailed status for canceled stage' do - expect(subject.text).to eq s_('CiStatusText|canceled') - end - end - - context 'when build is skipped' do - let!(:stage_build) { create_job(:ci_build, status: :skipped) } - - it 'returns detailed status for skipped stage' do - expect(subject.text).to eq s_('CiStatusText|skipped') - end - end - end - - describe '#success?' do - context 'when stage is successful' do - before do - create_job(:ci_build, status: :success) - create_job(:generic_commit_status, status: :success) - end - - it 'is successful' do - expect(stage).to be_success - end - end - - context 'when stage is not successful' do - before do - create_job(:ci_build, status: :failed) - create_job(:generic_commit_status, status: :success) - end - - it 'is not successful' do - expect(stage).not_to be_success - end - end - end - - describe '#has_warnings?' do - context 'when stage has warnings' do - context 'when using memoized warnings flag' do - context 'when there are warnings' do - let(:stage) { build(:ci_stage, warnings: true) } - - it 'returns true using memoized value' do - expect(stage).not_to receive(:statuses) - expect(stage).to have_warnings - end - end - - context 'when there are no warnings' do - let(:stage) { build(:ci_stage, warnings: false) } - - it 'returns false using memoized value' do - expect(stage).not_to receive(:statuses) - expect(stage).not_to have_warnings - end - end - end - - context 'when calculating warnings from statuses' do - before do - create(:ci_build, :failed, :allowed_to_fail, - stage: stage_name, pipeline: pipeline) - end - - it 'has warnings calculated from statuses' do - expect(stage).to receive(:statuses).and_call_original - expect(stage).to have_warnings - end - end - end - - context 'when stage does not have warnings' do - before do - create(:ci_build, :success, stage: stage_name, - pipeline: pipeline) - end - - it 'does not have warnings calculated from statuses' do - expect(stage).to receive(:statuses).and_call_original - expect(stage).not_to have_warnings - end - end - end - - def create_job(type, status: 'success', stage: stage_name, **opts) - create(type, pipeline: pipeline, stage: stage, status: status, **opts) - end - - it_behaves_like 'manual playable stage', :ci_stage -end diff --git a/spec/models/ci/pending_build_spec.rb b/spec/models/ci/pending_build_spec.rb index 5692444339f..4bb43233dbd 100644 --- a/spec/models/ci/pending_build_spec.rb +++ b/spec/models/ci/pending_build_spec.rb @@ -118,41 +118,27 @@ RSpec.describe Ci::PendingBuild do project.shared_runners_enabled = true end - context 'when ci_pending_builds_maintain_denormalized_data is enabled' do - it 'sets instance_runners_enabled to true' do - described_class.upsert_from_build!(build) - - expect(described_class.last.instance_runners_enabled).to be_truthy - end - - context 'when project is about to be deleted' do - before do - build.project.update!(pending_delete: true) - end + it 'sets instance_runners_enabled to true' do + described_class.upsert_from_build!(build) - it 'sets instance_runners_enabled to false' do - described_class.upsert_from_build!(build) + expect(described_class.last.instance_runners_enabled).to be_truthy + end - expect(described_class.last.instance_runners_enabled).to be_falsey - end + context 'when project is about to be deleted' do + before do + build.project.update!(pending_delete: true) end - context 'when builds are disabled' do - before do - build.project.project_feature.update!(builds_access_level: false) - end - - it 'sets instance_runners_enabled to false' do - described_class.upsert_from_build!(build) + it 'sets instance_runners_enabled to false' do + described_class.upsert_from_build!(build) - expect(described_class.last.instance_runners_enabled).to be_falsey - end + expect(described_class.last.instance_runners_enabled).to be_falsey end end - context 'when ci_pending_builds_maintain_denormalized_data is disabled' do + context 'when builds are disabled' do before do - stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) + build.project.project_feature.update!(builds_access_level: false) end it 'sets instance_runners_enabled to false' do @@ -168,24 +154,10 @@ RSpec.describe Ci::PendingBuild do subject(:ci_pending_build) { described_class.last } - context 'when ci_pending_builds_maintain_denormalized_data is enabled' do - it 'sets tag_ids' do - described_class.upsert_from_build!(build) - - expect(ci_pending_build.tag_ids).to eq(build.tags_ids) - end - end - - context 'when ci_pending_builds_maintain_denormalized_data is disabled' do - before do - stub_feature_flags(ci_pending_builds_maintain_denormalized_data: false) - end - - it 'does not set tag_ids' do - described_class.upsert_from_build!(build) + it 'sets tag_ids' do + described_class.upsert_from_build!(build) - expect(ci_pending_build.tag_ids).to be_empty - end + expect(ci_pending_build.tag_ids).to eq(build.tags_ids) end end diff --git a/spec/models/ci/pipeline_artifact_spec.rb b/spec/models/ci/pipeline_artifact_spec.rb index 801505f0231..b051f646bd4 100644 --- a/spec/models/ci/pipeline_artifact_spec.rb +++ b/spec/models/ci/pipeline_artifact_spec.rb @@ -196,6 +196,80 @@ RSpec.describe Ci::PipelineArtifact, type: :model do end end + describe '.create_or_replace_for_pipeline!' do + let_it_be(:pipeline) { create(:ci_empty_pipeline) } + + let(:file_type) { :code_coverage } + let(:file) { CarrierWaveStringFile.new_file(file_content: 'content', filename: 'file.json', content_type: 'json') } + let(:size) { file['tempfile'].size } + + subject do + Ci::PipelineArtifact.create_or_replace_for_pipeline!( + pipeline: pipeline, + file_type: file_type, + file: file, + size: size + ) + end + + around do |example| + freeze_time { example.run } + end + + context 'when there is no existing record' do + it 'creates a new pipeline artifact for the given parameters' do + expect { subject }.to change { Ci::PipelineArtifact.count }.from(0).to(1) + + expect(subject.code_coverage?).to be(true) + expect(subject.pipeline).to eq(pipeline) + expect(subject.project_id).to eq(pipeline.project_id) + expect(subject.file.filename).to eq(file['filename']) + expect(subject.size).to eq(size) + expect(subject.file_format).to eq(Ci::PipelineArtifact::REPORT_TYPES[file_type].to_s) + expect(subject.expire_at).to eq(Ci::PipelineArtifact::EXPIRATION_DATE.from_now) + end + end + + context 'when there are existing records with different types' do + let!(:existing_artifact) do + create(:ci_pipeline_artifact, pipeline: pipeline, file_type: file_type, expire_at: 1.day.from_now) + end + + let!(:other_artifact) { create(:ci_pipeline_artifact, pipeline: pipeline, file_type: :code_quality_mr_diff) } + + it 'replaces the existing pipeline artifact record with the given file type' do + expect { subject }.not_to change { Ci::PipelineArtifact.count } + + expect(subject.id).not_to eq(existing_artifact.id) + + expect(subject.code_coverage?).to be(true) + expect(subject.pipeline).to eq(pipeline) + expect(subject.project_id).to eq(pipeline.project_id) + expect(subject.file.filename).to eq(file['filename']) + expect(subject.size).to eq(size) + expect(subject.file_format).to eq(Ci::PipelineArtifact::REPORT_TYPES[file_type].to_s) + expect(subject.expire_at).to eq(Ci::PipelineArtifact::EXPIRATION_DATE.from_now) + end + end + + context 'when ActiveRecordError is raised' do + let(:pipeline) { instance_double(Ci::Pipeline, id: 1) } + let(:file_type) { :code_coverage } + let(:error) { ActiveRecord::ActiveRecordError.new('something went wrong') } + + before do + allow(pipeline).to receive(:pipeline_artifacts).and_raise(error) + end + + it 'tracks and raise the exception' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception) + .with(error, { pipeline_id: pipeline.id, file_type: file_type }).and_call_original + + expect { subject }.to raise_error(ActiveRecord::ActiveRecordError, 'something went wrong') + end + end + end + describe '#present' do subject(:presenter) { report.present } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 31752f300f4..081fa6cbbae 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -106,6 +106,50 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end + describe 'state machine transitions' do + context 'from failed to success' do + let_it_be(:pipeline) { create(:ci_empty_pipeline, :failed) } + + it 'schedules CoverageReportWorker' do + expect(Ci::PipelineArtifacts::CoverageReportWorker).to receive(:perform_async).with(pipeline.id) + + pipeline.succeed! + end + end + end + + describe 'pipeline age metric' do + let_it_be(:pipeline) { create(:ci_empty_pipeline, :created) } + + let(:pipeline_age_histogram) do + ::Gitlab::Ci::Pipeline::Metrics.pipeline_age_histogram + end + + context 'when pipeline age histogram is enabled' do + before do + stub_feature_flags(ci_pipeline_age_histogram: true) + end + + it 'observes pipeline age' do + expect(pipeline_age_histogram).to receive(:observe) + + described_class.find(pipeline.id) + end + end + + context 'when pipeline age histogram is disabled' do + before do + stub_feature_flags(ci_pipeline_age_histogram: false) + end + + it 'observes pipeline age' do + expect(pipeline_age_histogram).not_to receive(:observe) + + described_class.find(pipeline.id) + end + end + end + describe '#set_status' do let(:pipeline) { build(:ci_empty_pipeline, :created) } @@ -167,6 +211,28 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end + describe '.created_after' do + let_it_be(:old_pipeline) { create(:ci_pipeline, created_at: 1.week.ago) } + let_it_be(:pipeline) { create(:ci_pipeline) } + + subject { described_class.created_after(1.day.ago) } + + it 'returns the pipeline' do + is_expected.to contain_exactly(pipeline) + end + end + + describe '.created_before_id' do + let_it_be(:pipeline) { create(:ci_pipeline) } + let_it_be(:new_pipeline) { create(:ci_pipeline) } + + subject { described_class.created_before_id(new_pipeline.id) } + + it 'returns the pipeline' do + is_expected.to contain_exactly(pipeline) + end + end + describe '.for_sha' do subject { described_class.for_sha(sha) } @@ -997,6 +1063,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do 'CI_MERGE_REQUEST_PROJECT_PATH' => merge_request.project.full_path, 'CI_MERGE_REQUEST_PROJECT_URL' => merge_request.project.web_url, 'CI_MERGE_REQUEST_TARGET_BRANCH_NAME' => merge_request.target_branch.to_s, + 'CI_MERGE_REQUEST_TARGET_BRANCH_PROTECTED' => ProtectedBranch.protected?(merge_request.target_project, merge_request.target_branch).to_s, 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' => '', 'CI_MERGE_REQUEST_SOURCE_PROJECT_ID' => merge_request.source_project.id.to_s, 'CI_MERGE_REQUEST_SOURCE_PROJECT_PATH' => merge_request.source_project.full_path, @@ -1093,6 +1160,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do 'CI_MERGE_REQUEST_PROJECT_PATH' => merge_request.project.full_path, 'CI_MERGE_REQUEST_PROJECT_URL' => merge_request.project.web_url, 'CI_MERGE_REQUEST_TARGET_BRANCH_NAME' => merge_request.target_branch.to_s, + 'CI_MERGE_REQUEST_TARGET_BRANCH_PROTECTED' => ProtectedBranch.protected?(merge_request.target_project, merge_request.target_branch).to_s, 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA' => merge_request.target_branch_sha, 'CI_MERGE_REQUEST_SOURCE_PROJECT_ID' => merge_request.source_project.id.to_s, 'CI_MERGE_REQUEST_SOURCE_PROJECT_PATH' => merge_request.source_project.full_path, @@ -1289,48 +1357,6 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do status: 'success') end - describe '#legacy_stages' do - using RSpec::Parameterized::TableSyntax - - subject { pipeline.legacy_stages } - - context 'stages list' do - it 'returns ordered list of stages' do - expect(subject.map(&:name)).to eq(%w[build test deploy]) - end - end - - context 'stages with statuses' do - let(:statuses) do - subject.map { |stage| [stage.name, stage.status] } - end - - it 'returns list of stages with correct statuses' do - expect(statuses).to eq([%w(build failed), - %w(test success), - %w(deploy running)]) - end - end - - context 'when there is a stage with warnings' do - before do - create(:commit_status, pipeline: pipeline, - stage: 'deploy', - name: 'prod:2', - stage_idx: 2, - status: 'failed', - allow_failure: true) - end - - it 'populates stage with correct number of warnings' do - deploy_stage = pipeline.legacy_stages.third - - expect(deploy_stage).not_to receive(:statuses) - expect(deploy_stage).to have_warnings - end - end - end - describe '#stages_count' do it 'returns a valid number of stages' do expect(pipeline.stages_count).to eq(3) @@ -1344,37 +1370,11 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end - describe '#legacy_stage' do - subject { pipeline.legacy_stage('test') } - - let(:pipeline) { build(:ci_empty_pipeline, :created) } - - context 'with status in stage' do - before do - create(:commit_status, pipeline: pipeline, stage: 'test') - end - - it { expect(subject).to be_a Ci::LegacyStage } - it { expect(subject.name).to eq 'test' } - it { expect(subject.statuses).not_to be_empty } - end - - context 'without status in stage' do - before do - create(:commit_status, pipeline: pipeline, stage: 'build') - end - - it 'return stage object' do - is_expected.to be_nil - end - end - end - describe '#stages' do let(:pipeline) { build(:ci_empty_pipeline, :created) } before do - create(:ci_stage_entity, project: project, + create(:ci_stage, project: project, pipeline: pipeline, position: 4, name: 'deploy') @@ -1391,12 +1391,12 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do stage_idx: 2, name: 'build') - create(:ci_stage_entity, project: project, + create(:ci_stage, project: project, pipeline: pipeline, position: 1, name: 'sanity') - create(:ci_stage_entity, project: project, + create(:ci_stage, project: project, pipeline: pipeline, position: 5, name: 'cleanup') @@ -1435,7 +1435,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do let(:build_c) { create_build('build3', queued_at: 0) } %w[succeed! drop! cancel! skip! block! delay!].each do |action| - context "when the pipeline recieved #{action} event" do + context "when the pipeline received #{action} event" do it 'deletes a persistent ref' do expect(pipeline.persistent_ref).to receive(:delete).once @@ -1658,7 +1658,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end %w[succeed! drop! cancel! skip!].each do |action| - context "when the pipeline recieved #{action} event" do + context "when the pipeline received #{action} event" do it 'performs AutoMergeProcessWorker' do expect(AutoMergeProcessWorker).to receive(:perform_async).with(merge_request.id) @@ -3074,6 +3074,13 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do let(:pipeline_action) { action } it 'schedules a new PipelineHooksWorker job' do + expect(Gitlab::AppLogger).to receive(:info).with( + message: include("Enqueuing hooks for Pipeline #{pipeline.id}"), + class: described_class.name, + pipeline_id: pipeline.id, + project_id: pipeline.project_id, + pipeline_status: String + ) expect(PipelineHooksWorker).to receive(:perform_async).with(pipeline.id) pipeline.public_send(pipeline_action) @@ -3760,6 +3767,24 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end + describe '#self_and_descendants_complete?' do + let_it_be(:pipeline) { create(:ci_pipeline, :success) } + let_it_be(:child_pipeline) { create(:ci_pipeline, :success, child_of: pipeline) } + let_it_be_with_reload(:grandchild_pipeline) { create(:ci_pipeline, :success, child_of: child_pipeline) } + + context 'when all pipelines in the hierarchy is complete' do + it { expect(pipeline.self_and_descendants_complete?).to be(true) } + end + + context 'when a pipeline in the hierarchy is not complete' do + before do + grandchild_pipeline.update!(status: :running) + end + + it { expect(pipeline.self_and_descendants_complete?).to be(false) } + end + end + describe '#builds_in_self_and_descendants' do subject(:builds) { pipeline.builds_in_self_and_descendants } @@ -3928,7 +3953,21 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do context 'when pipeline status is running' do let(:pipeline) { create(:ci_pipeline, :running) } - it { is_expected.to be_falsey } + context 'with mr_show_reports_immediately flag enabled' do + before do + stub_feature_flags(mr_show_reports_immediately: project) + end + + it { expect(subject).to be_truthy } + end + + context 'with mr_show_reports_immediately flag disabled' do + before do + stub_feature_flags(mr_show_reports_immediately: false) + end + + it { expect(subject).to be_falsey } + end end context 'when pipeline status is success' do @@ -4002,7 +4041,21 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do context 'when pipeline status is running' do let(:pipeline) { create(:ci_pipeline, :running) } - it { expect(subject).to be_falsey } + context 'with mr_show_reports_immediately flag enabled' do + before do + stub_feature_flags(mr_show_reports_immediately: project) + end + + it { expect(subject).to be_truthy } + end + + context 'with mr_show_reports_immediately flag disabled' do + before do + stub_feature_flags(mr_show_reports_immediately: false) + end + + it { expect(subject).to be_falsey } + end end context 'when pipeline status is success' do @@ -4251,13 +4304,13 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end - describe '#find_stage_by_name' do + describe 'fetching a stage by name' do let_it_be(:pipeline) { create(:ci_pipeline) } let(:stage_name) { 'test' } let(:stage) do - create(:ci_stage_entity, + create(:ci_stage, pipeline: pipeline, project: pipeline.project, name: 'test') @@ -4267,19 +4320,37 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do create_list(:ci_build, 2, pipeline: pipeline, stage: stage.name) end - subject { pipeline.find_stage_by_name!(stage_name) } + describe '#stage' do + subject { pipeline.stage(stage_name) } - context 'when stage exists' do - it { is_expected.to eq(stage) } + context 'when stage exists' do + it { is_expected.to eq(stage) } + end + + context 'when stage does not exist' do + let(:stage_name) { 'build' } + + it 'returns nil' do + is_expected.to be_nil + end + end end - context 'when stage does not exist' do - let(:stage_name) { 'build' } + describe '#find_stage_by_name' do + subject { pipeline.find_stage_by_name!(stage_name) } - it 'raises an ActiveRecord exception' do - expect do - subject - end.to raise_exception(ActiveRecord::RecordNotFound) + context 'when stage exists' do + it { is_expected.to eq(stage) } + end + + context 'when stage does not exist' do + let(:stage_name) { 'build' } + + it 'raises an ActiveRecord exception' do + expect do + subject + end.to raise_exception(ActiveRecord::RecordNotFound) + end end end end @@ -4832,13 +4903,13 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end - describe '#has_expired_test_reports?' do - subject { pipeline.has_expired_test_reports? } + describe '#has_test_reports?' do + subject { pipeline.has_test_reports? } let(:pipeline) { create(:ci_pipeline, :success, :with_test_reports) } context 'when artifacts are not expired' do - it { is_expected.to be_falsey } + it { is_expected.to be_truthy } end context 'when artifacts are expired' do @@ -4849,6 +4920,14 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do it { is_expected.to be_truthy } end + context 'when artifacts are removed' do + before do + pipeline.job_artifacts.each(&:destroy) + end + + it { is_expected.to be_falsey } + end + context 'when the pipeline is still running' do let(:pipeline) { create(:ci_pipeline, :running) } @@ -4942,4 +5021,34 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do end end end + + describe '#age_in_minutes' do + let(:pipeline) { build(:ci_pipeline) } + + context 'when pipeline has not been persisted' do + it 'returns zero' do + expect(pipeline.age_in_minutes).to eq 0 + end + end + + context 'when pipeline has been saved' do + it 'returns pipeline age in minutes' do + pipeline.save! + + travel_to(pipeline.created_at + 2.hours) do + expect(pipeline.age_in_minutes).to eq 120 + end + end + end + + context 'when pipeline has been loaded without all attributes' do + it 'raises an exception' do + pipeline.save! + + pipeline_id = Ci::Pipeline.where(id: pipeline.id).select(:id).first + + expect { pipeline_id.age_in_minutes }.to raise_error(ArgumentError) + end + end + end end diff --git a/spec/models/ci/processable_spec.rb b/spec/models/ci/processable_spec.rb index cdd96d45561..789ae3a2ccc 100644 --- a/spec/models/ci/processable_spec.rb +++ b/spec/models/ci/processable_spec.rb @@ -24,7 +24,7 @@ RSpec.describe Ci::Processable do new_proc end - let_it_be(:stage) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: 'test') } + let_it_be(:stage) { create(:ci_stage, project: project, pipeline: pipeline, name: 'test') } shared_context 'processable bridge' do let_it_be(:downstream_project) { create(:project, :repository) } @@ -57,7 +57,7 @@ RSpec.describe Ci::Processable do let(:clone_accessors) { ::Ci::Build.clone_accessors.without(::Ci::Build.extra_accessors) } let(:reject_accessors) do - %i[id status user token token_encrypted coverage trace runner + %i[id status user token_encrypted coverage trace runner artifacts_expire_at created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 74d8b012b29..2fbfbbaf830 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Ci::Runner do end describe 'groups association' do - # Due to other assoctions such as projects this whole spec is allowed to + # Due to other associations such as projects this whole spec is allowed to # generate cross-database queries. So we have this temporary spec to # validate that at least groups association does not generate cross-DB # queries. @@ -35,6 +35,46 @@ RSpec.describe Ci::Runner do end end + describe 'acts_as_taggable' do + let(:tag_name) { 'tag123' } + + context 'on save' do + let_it_be_with_reload(:runner) { create(:ci_runner) } + + before do + runner.tag_list = [tag_name] + end + + context 'tag does not exist' do + it 'creates a tag' do + expect { runner.save! }.to change(ActsAsTaggableOn::Tag, :count).by(1) + end + + it 'creates an association to the tag' do + runner.save! + + expect(described_class.tagged_with(tag_name)).to include(runner) + end + end + + context 'tag already exists' do + before do + ActsAsTaggableOn::Tag.create!(name: tag_name) + end + + it 'does not create a tag' do + expect { runner.save! }.not_to change(ActsAsTaggableOn::Tag, :count) + end + + it 'creates an association to the tag' do + runner.save! + + expect(described_class.tagged_with(tag_name)).to include(runner) + end + end + end + end + describe 'validation' do it { is_expected.to validate_presence_of(:access_level) } it { is_expected.to validate_presence_of(:runner_type) } @@ -1062,18 +1102,6 @@ RSpec.describe Ci::Runner do end end end - - context 'with updated version' do - before do - runner.version = '1.2.3' - end - - it 'updates version components with new version' do - heartbeat - - expect(runner.reload.read_attribute(:semver)).to eq '15.0.1' - end - end end def expect_redis_update @@ -1088,7 +1116,6 @@ RSpec.describe Ci::Runner do .and change { runner.reload.read_attribute(:architecture) } .and change { runner.reload.read_attribute(:config) } .and change { runner.reload.read_attribute(:executor_type) } - .and change { runner.reload.read_attribute(:semver) } end end @@ -1193,6 +1220,47 @@ RSpec.describe Ci::Runner do end end + describe '#save_tags' do + let(:runner) { build(:ci_runner, tag_list: ['tag']) } + + it 'saves tags' do + runner.save! + + expect(runner.tags.count).to eq(1) + expect(runner.tags.first.name).to eq('tag') + end + + it 'strips tags' do + runner.tag_list = [' taga', 'tagb ', ' tagc '] + + runner.save! + expect(runner.tags.map(&:name)).to match_array(%w[taga tagb tagc]) + end + + context 'with BulkInsertableTags.with_bulk_insert_tags' do + it 'does not save_tags' do + Ci::BulkInsertableTags.with_bulk_insert_tags do + runner.save! + end + + expect(runner.tags).to be_empty + end + + context 'over TAG_LIST_MAX_LENGTH' do + let(:tag_list) { (1..described_class::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } } + let(:runner) { build(:ci_runner, tag_list: tag_list) } + + it 'fails validation if over tag limit' do + Ci::BulkInsertableTags.with_bulk_insert_tags do + expect { runner.save! }.to raise_error(ActiveRecord::RecordInvalid) + end + + expect(runner.tags).to be_empty + end + end + end + end + describe '#has_tags?' do context 'when runner has tags' do subject { create(:ci_runner, tag_list: ['tag']) } @@ -1700,40 +1768,37 @@ RSpec.describe Ci::Runner do end end - describe '.save' do - context 'with initial value' do - let(:runner) { create(:ci_runner, version: 'v1.2.3') } - - it 'updates semver column' do - expect(runner.semver).to eq '1.2.3' - end - end + describe '#with_upgrade_status' do + subject { described_class.with_upgrade_status(upgrade_status) } - context 'with no initial version value' do - let(:runner) { build(:ci_runner) } + let_it_be(:runner_14_0_0) { create(:ci_runner, version: '14.0.0') } + let_it_be(:runner_14_1_0) { create(:ci_runner, version: '14.1.0') } + let_it_be(:runner_14_1_1) { create(:ci_runner, version: '14.1.1') } + let_it_be(:runner_version_14_0_0) { create(:ci_runner_version, version: '14.0.0', status: :available) } + let_it_be(:runner_version_14_1_0) { create(:ci_runner_version, version: '14.1.0', status: :recommended) } + let_it_be(:runner_version_14_1_1) { create(:ci_runner_version, version: '14.1.1', status: :not_available) } - context 'with version change' do - subject(:update_version) { runner.update!(version: new_version) } + context ':not_available' do + let(:upgrade_status) { :not_available } - context 'to invalid version' do - let(:new_version) { 'invalid version' } - - it 'updates semver column to nil' do - update_version + it 'returns runners whose version is assigned :not_available' do + is_expected.to contain_exactly(runner_14_1_1) + end + end - expect(runner.reload.semver).to be_nil - end - end + context ':available' do + let(:upgrade_status) { :available } - context 'to v14.10.1' do - let(:new_version) { 'v14.10.1' } + it 'returns runners whose version is assigned :available' do + is_expected.to contain_exactly(runner_14_0_0) + end + end - it 'updates semver column' do - update_version + context ':recommended' do + let(:upgrade_status) { :recommended} - expect(runner.reload.semver).to eq '14.10.1' - end - end + it 'returns runners whose version is assigned :recommended' do + is_expected.to contain_exactly(runner_14_1_0) end end end diff --git a/spec/models/ci/runner_version_spec.rb b/spec/models/ci/runner_version_spec.rb new file mode 100644 index 00000000000..d3395942a39 --- /dev/null +++ b/spec/models/ci/runner_version_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::RunnerVersion do + it_behaves_like 'having unique enum values' + + let_it_be(:runner_version_not_available) do + create(:ci_runner_version, version: 'abc123', status: :not_available) + end + + let_it_be(:runner_version_recommended) do + create(:ci_runner_version, version: 'abc234', status: :recommended) + end + + describe '.not_available' do + subject { described_class.not_available } + + it { is_expected.to match_array([runner_version_not_available]) } + end + + describe '.potentially_outdated' do + subject { described_class.potentially_outdated } + + let_it_be(:runner_version_nil) { create(:ci_runner_version, version: 'abc345', status: nil) } + let_it_be(:runner_version_available) do + create(:ci_runner_version, version: 'abc456', status: :available) + end + + let_it_be(:runner_version_unknown) do + create(:ci_runner_version, version: 'abc567', status: :unknown) + end + + it 'contains any runner version that is not already recommended' do + is_expected.to match_array([ + runner_version_nil, + runner_version_not_available, + runner_version_available, + runner_version_unknown + ]) + end + end + + describe 'validation' do + context 'when runner version is too long' do + let(:runner_version) { build(:ci_runner_version, version: 'a' * 2049) } + + it 'is not valid' do + expect(runner_version).to be_invalid + end + end + end +end diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb index b91348eb408..d55a8509a98 100644 --- a/spec/models/ci/stage_spec.rb +++ b/spec/models/ci/stage_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Ci::Stage, :models do let_it_be(:pipeline) { create(:ci_empty_pipeline) } - let(:stage) { create(:ci_stage_entity, pipeline: pipeline, project: pipeline.project) } + let(:stage) { create(:ci_stage, pipeline: pipeline, project: pipeline.project) } it_behaves_like 'having unique enum values' @@ -30,9 +30,9 @@ RSpec.describe Ci::Stage, :models do describe '.by_position' do it 'finds stages by position' do - a = create(:ci_stage_entity, position: 1) - b = create(:ci_stage_entity, position: 2) - c = create(:ci_stage_entity, position: 3) + a = create(:ci_stage, position: 1) + b = create(:ci_stage, position: 2) + c = create(:ci_stage, position: 3) expect(described_class.by_position(1)).to contain_exactly(a) expect(described_class.by_position(2)).to contain_exactly(b) @@ -42,9 +42,9 @@ RSpec.describe Ci::Stage, :models do describe '.by_name' do it 'finds stages by name' do - a = create(:ci_stage_entity, name: 'a') - b = create(:ci_stage_entity, name: 'b') - c = create(:ci_stage_entity, name: 'c') + a = create(:ci_stage, name: 'a') + b = create(:ci_stage, name: 'b') + c = create(:ci_stage, name: 'c') expect(described_class.by_name('a')).to contain_exactly(a) expect(described_class.by_name('b')).to contain_exactly(b) @@ -54,7 +54,7 @@ RSpec.describe Ci::Stage, :models do describe '#status' do context 'when stage is pending' do - let(:stage) { create(:ci_stage_entity, status: 'pending') } + let(:stage) { create(:ci_stage, status: 'pending') } it 'has a correct status value' do expect(stage.status).to eq 'pending' @@ -62,7 +62,7 @@ RSpec.describe Ci::Stage, :models do end context 'when stage is success' do - let(:stage) { create(:ci_stage_entity, status: 'success') } + let(:stage) { create(:ci_stage, status: 'success') } it 'has a correct status value' do expect(stage.status).to eq 'success' @@ -119,7 +119,7 @@ RSpec.describe Ci::Stage, :models do end context 'when stage has only created builds' do - let(:stage) { create(:ci_stage_entity, status: :created) } + let(:stage) { create(:ci_stage, status: :created) } before do create(:ci_build, :created, stage_id: stage.id) @@ -206,7 +206,7 @@ RSpec.describe Ci::Stage, :models do using RSpec::Parameterized::TableSyntax let(:user) { create(:user) } - let(:stage) { create(:ci_stage_entity, status: :created) } + let(:stage) { create(:ci_stage, status: :created) } subject { stage.detailed_status(user) } @@ -269,7 +269,7 @@ RSpec.describe Ci::Stage, :models do describe '#delay' do subject { stage.delay } - let(:stage) { create(:ci_stage_entity, status: :created) } + let(:stage) { create(:ci_stage, status: :created) } it 'updates stage status' do subject @@ -361,12 +361,12 @@ RSpec.describe Ci::Stage, :models do end end - it_behaves_like 'manual playable stage', :ci_stage_entity + it_behaves_like 'manual playable stage', :ci_stage 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) } + let!(:model) { create(:ci_stage, project: parent) } end end end diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 29ca088ee04..f0af229ff2c 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -51,4 +51,10 @@ RSpec.describe Ci::Variable do let!(:model) { create(:ci_variable, project: parent) } end end + + describe '#audit_details' do + it "equals to the variable's key" do + expect(subject.audit_details).to eq(subject.key) + end + end end diff --git a/spec/models/clusters/applications/elastic_stack_spec.rb b/spec/models/clusters/applications/elastic_stack_spec.rb deleted file mode 100644 index af2802d5e47..00000000000 --- a/spec/models/clusters/applications/elastic_stack_spec.rb +++ /dev/null @@ -1,177 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Applications::ElasticStack do - include KubernetesHelpers - - include_examples 'cluster application core specs', :clusters_applications_elastic_stack - include_examples 'cluster application status specs', :clusters_applications_elastic_stack - include_examples 'cluster application version specs', :clusters_applications_elastic_stack - include_examples 'cluster application helm specs', :clusters_applications_elastic_stack - - describe 'cluster.integration_elastic_stack state synchronization' do - let!(:application) { create(:clusters_applications_elastic_stack) } - let(:cluster) { application.cluster } - let(:integration) { cluster.integration_elastic_stack } - - describe 'after_destroy' do - it 'disables the corresponding integration' do - application.destroy! - - expect(integration).not_to be_enabled - end - end - - describe 'on install' do - it 'enables the corresponding integration' do - application.make_scheduled! - application.make_installing! - application.make_installed! - - expect(integration).to be_enabled - end - end - - describe 'on uninstall' do - it 'disables the corresponding integration' do - application.make_scheduled! - application.make_installing! - application.make_installed! - application.make_externally_uninstalled! - - expect(integration).not_to be_enabled - end - end - end - - describe '#install_command' do - let!(:elastic_stack) { create(:clusters_applications_elastic_stack) } - - subject { elastic_stack.install_command } - - it { is_expected.to be_an_instance_of(Gitlab::Kubernetes::Helm::V3::InstallCommand) } - - it 'is initialized with elastic stack arguments' do - expect(subject.name).to eq('elastic-stack') - expect(subject.chart).to eq('elastic-stack/elastic-stack') - expect(subject.version).to eq('3.0.0') - expect(subject.repository).to eq('https://charts.gitlab.io') - expect(subject).to be_rbac - expect(subject.files).to eq(elastic_stack.files) - expect(subject.preinstall).to be_empty - end - - context 'within values.yaml' do - let(:values_yaml_content) {subject.files[:"values.yaml"]} - - it 'contains the disabled index lifecycle management' do - expect(values_yaml_content).to include "setup.ilm.enabled: false" - end - - it 'contains daily indices with respective template' do - expect(values_yaml_content).to include "index: \"filebeat-%{[agent.version]}-%{+yyyy.MM.dd}\"" - expect(values_yaml_content).to include "setup.template.name: 'filebeat'" - expect(values_yaml_content).to include "setup.template.pattern: 'filebeat-*'" - end - end - - context 'on a non rbac enabled cluster' do - before do - elastic_stack.cluster.platform_kubernetes.abac! - end - - it { is_expected.not_to be_rbac } - end - - context 'on versions older than 2' do - before do - elastic_stack.status = elastic_stack.status_states[:updating] - elastic_stack.version = "1.9.0" - end - - it 'includes a preinstall script' do - expect(subject.preinstall).not_to be_empty - expect(subject.preinstall.first).to include("helm uninstall") - end - end - - context 'on versions older than 3' do - before do - elastic_stack.status = elastic_stack.status_states[:updating] - elastic_stack.version = "2.9.0" - end - - it 'includes a preinstall script' do - expect(subject.preinstall).not_to be_empty - expect(subject.preinstall.first).to include("helm uninstall") - end - end - - context 'application failed to install previously' do - let(:elastic_stack) { create(:clusters_applications_elastic_stack, :errored, version: '0.0.1') } - - it 'is initialized with the locked version' do - expect(subject.version).to eq('3.0.0') - end - end - end - - describe '#chart_above_v2?' do - let(:elastic_stack) { create(:clusters_applications_elastic_stack, version: version) } - - subject { elastic_stack.chart_above_v2? } - - context 'on v1.9.0' do - let(:version) { '1.9.0' } - - it { is_expected.to be_falsy } - end - - context 'on v2.0.0' do - let(:version) { '2.0.0' } - - it { is_expected.to be_truthy } - end - end - - describe '#chart_above_v3?' do - let(:elastic_stack) { create(:clusters_applications_elastic_stack, version: version) } - - subject { elastic_stack.chart_above_v3? } - - context 'on v1.9.0' do - let(:version) { '1.9.0' } - - it { is_expected.to be_falsy } - end - - context 'on v3.0.0' do - let(:version) { '3.0.0' } - - it { is_expected.to be_truthy } - end - end - - describe '#uninstall_command' do - let!(:elastic_stack) { create(:clusters_applications_elastic_stack) } - - subject { elastic_stack.uninstall_command } - - it { is_expected.to be_an_instance_of(Gitlab::Kubernetes::Helm::V3::DeleteCommand) } - - it 'is initialized with elastic stack arguments' do - expect(subject.name).to eq('elastic-stack') - expect(subject).to be_rbac - expect(subject.files).to eq(elastic_stack.files) - end - - it 'specifies a post delete command to remove custom resource definitions' do - expect(subject.postdelete).to eq([ - 'kubectl delete pvc --selector app\\=elastic-stack-elasticsearch-master --namespace gitlab-managed-apps' - ]) - end - end - - it_behaves_like 'cluster-based #elasticsearch_client', :clusters_applications_elastic_stack -end diff --git a/spec/models/clusters/cluster_spec.rb b/spec/models/clusters/cluster_spec.rb index 30591a3ff5d..65ead01a2bd 100644 --- a/spec/models/clusters/cluster_spec.rb +++ b/spec/models/clusters/cluster_spec.rb @@ -42,7 +42,6 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do it { is_expected.to delegate_method(:available?).to(:application_helm).with_prefix } it { is_expected.to delegate_method(:available?).to(:application_ingress).with_prefix } it { is_expected.to delegate_method(:available?).to(:application_knative).with_prefix } - it { is_expected.to delegate_method(:available?).to(:integration_elastic_stack).with_prefix } it { is_expected.to delegate_method(:available?).to(:integration_prometheus).with_prefix } it { is_expected.to delegate_method(:external_ip).to(:application_ingress).with_prefix } it { is_expected.to delegate_method(:external_hostname).to(:application_ingress).with_prefix } @@ -200,22 +199,6 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do end end - describe '.with_available_elasticstack' do - subject { described_class.with_available_elasticstack } - - let_it_be(:cluster) { create(:cluster) } - - context 'cluster has ElasticStack application' do - let!(:application) { create(:clusters_applications_elastic_stack, :installed, cluster: cluster) } - - it { is_expected.to include(cluster) } - end - - context 'cluster does not have ElasticStack application' do - it { is_expected.not_to include(cluster) } - end - end - describe '.distinct_with_deployed_environments' do subject { described_class.distinct_with_deployed_environments } diff --git a/spec/models/clusters/integrations/elastic_stack_spec.rb b/spec/models/clusters/integrations/elastic_stack_spec.rb deleted file mode 100644 index be4d59b52a2..00000000000 --- a/spec/models/clusters/integrations/elastic_stack_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Clusters::Integrations::ElasticStack do - include KubernetesHelpers - include StubRequests - - describe 'associations' do - it { is_expected.to belong_to(:cluster).class_name('Clusters::Cluster') } - end - - describe 'validations' do - it { is_expected.to validate_presence_of(:cluster) } - it { is_expected.not_to allow_value(nil).for(:enabled) } - end - - it_behaves_like 'cluster-based #elasticsearch_client', :clusters_integrations_elastic_stack -end diff --git a/spec/models/clusters/integrations/prometheus_spec.rb b/spec/models/clusters/integrations/prometheus_spec.rb index d1e40fffee0..90e99aefdce 100644 --- a/spec/models/clusters/integrations/prometheus_spec.rb +++ b/spec/models/clusters/integrations/prometheus_spec.rb @@ -26,19 +26,6 @@ RSpec.describe Clusters::Integrations::Prometheus do integration.destroy! end - - context 'when the FF :rename_integrations_workers is disabled' do - before do - stub_feature_flags(rename_integrations_workers: false) - end - - it 'uses the old worker' do - expect(Clusters::Applications::DeactivateServiceWorker) - .to receive(:perform_async).with(cluster.id, 'prometheus') - - integration.destroy! - end - end end describe 'after_save' do @@ -70,19 +57,6 @@ RSpec.describe Clusters::Integrations::Prometheus do integration.update!(enabled: true) end - - context 'when the FF :rename_integrations_workers is disabled' do - before do - stub_feature_flags(rename_integrations_workers: false) - end - - it 'uses the old worker' do - expect(Clusters::Applications::ActivateServiceWorker) - .to receive(:perform_async).with(cluster.id, 'prometheus') - - integration.update!(enabled: true) - end - end end context 'when disabling' do diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index dbb15fad246..3cccc41a892 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -803,7 +803,7 @@ RSpec.describe CommitStatus do describe 'ensure stage assignment' do context 'when commit status has a stage_id assigned' do let!(:stage) do - create(:ci_stage_entity, project: project, pipeline: pipeline) + create(:ci_stage, project: project, pipeline: pipeline) end let(:commit_status) do @@ -836,7 +836,7 @@ RSpec.describe CommitStatus do context 'when commit status does not have stage but it exists' do let!(:stage) do - create(:ci_stage_entity, project: project, + create(:ci_stage, project: project, pipeline: pipeline, name: 'test') end @@ -984,22 +984,6 @@ RSpec.describe CommitStatus do end end - describe '.bulk_insert_tags!' do - let(:statuses) { double('statuses') } - let(:inserter) { double('inserter') } - - it 'delegates to bulk insert class' do - expect(Gitlab::Ci::Tags::BulkInsert) - .to receive(:new) - .with(statuses) - .and_return(inserter) - - expect(inserter).to receive(:insert!) - - described_class.bulk_insert_tags!(statuses) - end - end - describe '#expire_etag_cache!' do it 'expires the etag cache' do expect_next_instance_of(Gitlab::EtagCaching::Store) do |etag_store| diff --git a/spec/models/concerns/awareness_spec.rb b/spec/models/concerns/awareness_spec.rb new file mode 100644 index 00000000000..67acacc7bb1 --- /dev/null +++ b/spec/models/concerns/awareness_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Awareness, :clean_gitlab_redis_shared_state do + subject { create(:user) } + + let(:session) { AwarenessSession.for(1) } + + describe "when joining a session" do + it "increases the number of sessions" do + expect { subject.join(session) } + .to change { subject.session_ids.size } + .by(1) + end + end + + describe "when leaving session" do + it "decreases the number of sessions" do + subject.join(session) + + expect { subject.leave(session) } + .to change { subject.session_ids.size } + .by(-1) + end + end + + describe "when joining multiple sessions" do + let(:session2) { AwarenessSession.for(2) } + + it "increases number of active sessions for user" do + expect do + subject.join(session) + subject.join(session2) + end.to change { subject.session_ids.size } + .by(2) + end + end +end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index a00129b3fdf..19b9a1519eb 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -9,6 +9,8 @@ RSpec.describe CacheMarkdownField, :clean_gitlab_redis_cache do include CacheMarkdownField cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description + + before_validation -> { self.work_item_type_id = ::WorkItems::Type.default_issue_type.id } end end diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb index dc80e30216a..0629debda15 100644 --- a/spec/models/concerns/cacheable_attributes_spec.rb +++ b/spec/models/concerns/cacheable_attributes_spec.rb @@ -205,7 +205,12 @@ RSpec.describe CacheableAttributes do end end - it 'uses RequestStore in addition to process memory cache', :request_store, :do_not_mock_admin_mode_setting do + it( + 'uses RequestStore in addition to process memory cache', + :request_store, + :do_not_mock_admin_mode_setting, + :do_not_stub_snowplow_by_default + ) do # Warm up the cache create(:application_setting).cache! diff --git a/spec/models/concerns/ci/artifactable_spec.rb b/spec/models/concerns/ci/artifactable_spec.rb index b27a4d0dcc1..6af244a5a0f 100644 --- a/spec/models/concerns/ci/artifactable_spec.rb +++ b/spec/models/concerns/ci/artifactable_spec.rb @@ -53,6 +53,15 @@ RSpec.describe Ci::Artifactable do expect { |b| artifact.each_blob(&b) }.to raise_error(described_class::NotSupportedAdapterError) end end + + context 'pushes artifact_size to application context' do + let(:artifact) { create(:ci_job_artifact, :junit) } + + it 'logs artifact size', :aggregate_failures do + expect { |b| artifact.each_blob(&b) }.to yield_control.once + expect(Gitlab::ApplicationContext.current).to include("meta.artifact_size" => artifact.size) + end + end end context 'ActiveRecord scopes' do diff --git a/spec/models/concerns/ci/bulk_insertable_tags_spec.rb b/spec/models/concerns/ci/bulk_insertable_tags_spec.rb new file mode 100644 index 00000000000..23f0831403d --- /dev/null +++ b/spec/models/concerns/ci/bulk_insertable_tags_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::BulkInsertableTags do + let(:taggable_class) do + Class.new do + prepend Ci::BulkInsertableTags + + attr_reader :tags_saved + + def save_tags + @tags_saved = true + end + end + end + + let(:record) { taggable_class.new } + + describe '.with_bulk_insert_tags' do + it 'changes the thread key to true' do + expect(Thread.current['ci_bulk_insert_tags']).to be_nil + + described_class.with_bulk_insert_tags do + expect(Thread.current['ci_bulk_insert_tags']).to eq(true) + end + + expect(Thread.current['ci_bulk_insert_tags']).to be_nil + end + end + + describe '#save_tags' do + it 'calls super' do + record.save_tags + + expect(record.tags_saved).to eq(true) + end + + it 'does not call super with BulkInsertableTags.with_bulk_insert_tags' do + described_class.with_bulk_insert_tags do + record.save_tags + end + + expect(record.tags_saved).to be_nil + end + + it 'isolates bulk insert behavior between threads' do + record2 = taggable_class.new + + t1 = Thread.new do + described_class.with_bulk_insert_tags do + record.save_tags + end + end + + t2 = Thread.new do + record2.save_tags + end + + [t1, t2].each(&:join) + + expect(record.tags_saved).to be_nil + expect(record2.tags_saved).to eq(true) + end + end +end diff --git a/spec/models/concerns/each_batch_spec.rb b/spec/models/concerns/each_batch_spec.rb index f1fb4fcbd03..2c75d4d5c41 100644 --- a/spec/models/concerns/each_batch_spec.rb +++ b/spec/models/concerns/each_batch_spec.rb @@ -3,17 +3,17 @@ require 'spec_helper' RSpec.describe EachBatch do - describe '.each_batch' do - let(:model) do - Class.new(ActiveRecord::Base) do - include EachBatch + let(:model) do + Class.new(ActiveRecord::Base) do + include EachBatch - self.table_name = 'users' + self.table_name = 'users' - scope :never_signed_in, -> { where(sign_in_count: 0) } - end + scope :never_signed_in, -> { where(sign_in_count: 0) } end + end + describe '.each_batch' do before do create_list(:user, 5, updated_at: 1.day.ago) end @@ -86,4 +86,89 @@ RSpec.describe EachBatch do end end end + + describe '.distinct_each_batch' do + let_it_be(:users) { create_list(:user, 5, sign_in_count: 0) } + + let(:params) { {} } + + subject(:values) do + values = [] + + model.distinct_each_batch(**params) { |rel| values.concat(rel.pluck(params[:column])) } + values + end + + context 'when iterating over a unique column' do + context 'when using ascending order' do + let(:expected_values) { users.pluck(:id).sort } + let(:params) { { column: :id, of: 1, order: :asc } } + + it { is_expected.to eq(expected_values) } + + context 'when using larger batch size' do + before do + params[:of] = 3 + end + + it { is_expected.to eq(expected_values) } + end + + context 'when using larger batch size than the result size' do + before do + params[:of] = 100 + end + + it { is_expected.to eq(expected_values) } + end + end + + context 'when using descending order' do + let(:expected_values) { users.pluck(:id).sort.reverse } + let(:params) { { column: :id, of: 1, order: :desc } } + + it { is_expected.to eq(expected_values) } + + context 'when using larger batch size' do + before do + params[:of] = 3 + end + + it { is_expected.to eq(expected_values) } + end + end + end + + context 'when iterating over a non-unique column' do + let(:params) { { column: :sign_in_count, of: 2, order: :asc } } + + context 'when only one value is present' do + it { is_expected.to eq([0]) } + end + + context 'when duplicated values present' do + let(:expected_values) { [2, 5] } + + before do + users[0].reload.update!(sign_in_count: 5) + users[1].reload.update!(sign_in_count: 2) + users[2].reload.update!(sign_in_count: 5) + users[3].reload.update!(sign_in_count: 2) + users[4].reload.update!(sign_in_count: 5) + end + + it { is_expected.to eq(expected_values) } + + context 'when using descending order' do + let(:expected_values) { [5, 2] } + + before do + params[:order] = :desc + end + + it { is_expected.to eq(expected_values) } + end + end + end + end end diff --git a/spec/models/concerns/loose_index_scan_spec.rb b/spec/models/concerns/loose_index_scan_spec.rb new file mode 100644 index 00000000000..685819bfb86 --- /dev/null +++ b/spec/models/concerns/loose_index_scan_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true +# frozen_string_literal + +require 'spec_helper' + +RSpec.describe LooseIndexScan, type: :model do + let(:issue_model) do + Class.new(ApplicationRecord) do + include LooseIndexScan + + self.table_name = 'issues' + end + end + + let_it_be(:user_1) { create(:user) } + let_it_be(:user_2) { create(:user) } + let_it_be(:user_3) { create(:user) } + + let_it_be(:issue_1) { create(:issue, author: user_2) } + let_it_be(:issue_2) { create(:issue, author: user_1) } + let_it_be(:issue_3) { create(:issue, author: user_1) } + let_it_be(:issue_4) { create(:issue, author: user_2) } + let_it_be(:issue_5) { create(:issue, author: user_3) } + + context 'loading distinct author_ids' do + subject(:author_ids) { issue_model.loose_index_scan(column: :author_id, order: order).pluck(:author_id) } + + shared_examples 'assert distinct values example' do + it 'loads the distinct values in the correct order' do + expect(author_ids).to eq(expected_order) + end + end + + context 'when using ascending order' do + let(:order) { :asc } + let(:expected_order) { [user_1.id, user_2.id, user_3.id] } + + it_behaves_like 'assert distinct values example' + + context 'when null values are present' do + before do + issue_1.author_id = nil + issue_1.save!(validate: false) + end + + it_behaves_like 'assert distinct values example' + end + + context 'when using descending order' do + let(:order) { :desc } + let(:expected_order) { [user_3.id, user_2.id, user_1.id] } + + it_behaves_like 'assert distinct values example' + end + end + end +end diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb index 99a3a0fb79a..b92c7c52f0b 100644 --- a/spec/models/concerns/participable_spec.rb +++ b/spec/models/concerns/participable_spec.rb @@ -31,7 +31,7 @@ RSpec.describe Participable do expect(instance).to receive(:foo).and_return(user2) expect(instance).to receive(:bar).and_return(user3) - expect(instance).to receive(:project).twice.and_return(project) + expect(instance).to receive(:project).thrice.and_return(project) participants = instance.participants(user1) @@ -66,7 +66,7 @@ RSpec.describe Participable do expect(instance).to receive(:foo).and_return(other) expect(other).to receive(:bar).and_return(user2) - expect(instance).to receive(:project).twice.and_return(project) + expect(instance).to receive(:project).thrice.and_return(project) expect(instance.participants(user1)).to eq([user2]) end @@ -86,7 +86,7 @@ RSpec.describe Participable do instance = model.new - expect(instance).to receive(:project).twice.and_return(project) + expect(instance).to receive(:project).thrice.and_return(project) instance.participants(user1) @@ -138,7 +138,7 @@ RSpec.describe Participable do allow(instance).to receive_message_chain(:model_name, :element) { 'class' } expect(instance).to receive(:foo).and_return(user2) expect(instance).to receive(:bar).and_return(user3) - expect(instance).to receive(:project).twice.and_return(project) + expect(instance).to receive(:project).thrice.and_return(project) participants = instance.visible_participants(user1) @@ -159,7 +159,7 @@ RSpec.describe Participable do allow(instance).to receive_message_chain(:model_name, :element) { 'class' } allow(instance).to receive(:bar).and_return(user2) - expect(instance).to receive(:project).twice.and_return(project) + expect(instance).to receive(:project).thrice.and_return(project) expect(instance.visible_participants(user1)).to be_empty end diff --git a/spec/models/concerns/pg_full_text_searchable_spec.rb b/spec/models/concerns/pg_full_text_searchable_spec.rb index 84209999ab2..55e3caf3c4c 100644 --- a/spec/models/concerns/pg_full_text_searchable_spec.rb +++ b/spec/models/concerns/pg_full_text_searchable_spec.rb @@ -14,6 +14,8 @@ RSpec.describe PgFullTextSearchable do belongs_to :project has_one :search_data, class_name: 'Issues::SearchData' + before_validation -> { self.work_item_type_id = ::WorkItems::Type.default_issue_type.id } + def persist_pg_full_text_search_vector(search_vector) Issues::SearchData.upsert({ project_id: project_id, issue_id: id, search_vector: search_vector }, unique_by: %i(project_id issue_id)) end @@ -185,6 +187,8 @@ RSpec.describe PgFullTextSearchable do belongs_to :project has_one :search_data, class_name: 'Issues::SearchData' + before_validation -> { self.work_item_type_id = ::WorkItems::Type.default_issue_type.id } + def self.name 'Issue' end diff --git a/spec/models/concerns/require_email_verification_spec.rb b/spec/models/concerns/require_email_verification_spec.rb new file mode 100644 index 00000000000..d087b2864f8 --- /dev/null +++ b/spec/models/concerns/require_email_verification_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RequireEmailVerification do + let_it_be(:model) do + Class.new(ApplicationRecord) do + self.table_name = 'users' + + devise :lockable + + include RequireEmailVerification + end + end + + using RSpec::Parameterized::TableSyntax + + where(:feature_flag_enabled, :two_factor_enabled, :overridden) do + false | false | false + false | true | false + true | false | true + true | true | false + end + + with_them do + let(:instance) { model.new(id: 1) } + let(:another_instance) { model.new(id: 2) } + + before do + stub_feature_flags(require_email_verification: feature_flag_enabled ? instance : another_instance) + allow(instance).to receive(:two_factor_enabled?).and_return(two_factor_enabled) + end + + describe '#lock_access!' do + subject { instance.lock_access! } + + before do + allow(instance).to receive(:save) + end + + it 'sends Devise unlock instructions unless overridden and always sets locked_at' do + expect(instance).to receive(:send_unlock_instructions).exactly(overridden ? 0 : 1).times + + expect { subject }.to change { instance.locked_at }.from(nil) + end + end + + describe '#attempts_exceeded?' do + subject { instance.send(:attempts_exceeded?) } + + context 'when failed_attempts is LT overridden amount' do + before do + instance.failed_attempts = 5 + end + + it { is_expected.to eq(false) } + end + + context 'when failed_attempts is GTE overridden amount but LT Devise default amount' do + before do + instance.failed_attempts = 6 + end + + it { is_expected.to eq(overridden) } + end + + context 'when failed_attempts is GTE Devise default amount' do + before do + instance.failed_attempts = 10 + end + + it { is_expected.to eq(true) } + end + end + + describe '#lock_expired?' do + subject { instance.send(:lock_expired?) } + + context 'when locked shorter ago than Devise default time' do + before do + instance.locked_at = 9.minutes.ago + end + + it { is_expected.to eq(false) } + end + + context 'when locked longer ago than Devise default time but shorter ago than overriden time' do + before do + instance.locked_at = 11.minutes.ago + end + + it { is_expected.to eq(!overridden) } + end + + context 'when locked longer ago than overriden time' do + before do + instance.locked_at = (24.hours + 1.minute).ago + end + + it { is_expected.to eq(true) } + end + end + end +end diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index d7bfcc3f579..a2ce02f4661 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -94,6 +94,7 @@ RSpec.describe ApplicationSetting, 'TokenAuthenticatable' do end it { is_expected.to respond_to(:ensure_runners_registration_token) } + it { is_expected.to respond_to(:ensure_error_tracking_access_token) } it { is_expected.to respond_to(:ensure_yet_another_token) } end diff --git a/spec/models/container_registry/event_spec.rb b/spec/models/container_registry/event_spec.rb index e0194a07f46..799d9d4fd1c 100644 --- a/spec/models/container_registry/event_spec.rb +++ b/spec/models/container_registry/event_spec.rb @@ -40,16 +40,18 @@ RSpec.describe ContainerRegistry::Event do subject(:handle!) { described_class.new(raw_event).handle! } - it 'enqueues a project statistics update' do - expect(ProjectCacheWorker).to receive(:perform_async).with(project.id, [], [:container_registry_size]) + shared_examples 'event with project statistics update' do + it 'enqueues a project statistics update' do + expect(ProjectCacheWorker).to receive(:perform_async).with(project.id, [], [:container_registry_size]) - handle! - end + handle! + end - it 'clears the cache for the namespace container repositories size' do - expect(Rails.cache).to receive(:delete).with(group.container_repositories_size_cache_key) + it 'clears the cache for the namespace container repositories size' do + expect(Rails.cache).to receive(:delete).with(group.container_repositories_size_cache_key) - handle! + handle! + end end shared_examples 'event without project statistics update' do @@ -60,10 +62,32 @@ RSpec.describe ContainerRegistry::Event do end end + it_behaves_like 'event with project statistics update' + context 'with no target tag' do let(:target) { super().without('tag') } it_behaves_like 'event without project statistics update' + + context 'with a target digest' do + let(:target) { super().merge('digest' => 'abc123') } + + it_behaves_like 'event without project statistics update' + end + + context 'with a delete action' do + let(:action) { 'delete' } + + context 'without a target digest' do + it_behaves_like 'event without project statistics update' + end + + context 'with a target digest' do + let(:target) { super().merge('digest' => 'abc123') } + + it_behaves_like 'event with project statistics update' + end + end end context 'with an unsupported action' do diff --git a/spec/models/container_repository_spec.rb b/spec/models/container_repository_spec.rb index 7d0dfad91b2..e35788b1848 100644 --- a/spec/models/container_repository_spec.rb +++ b/spec/models/container_repository_spec.rb @@ -689,10 +689,29 @@ RSpec.describe ContainerRepository, :aggregate_failures do it { is_expected.to eq(nil) } end - context 'with an old repository' do + context 'supports gitlab api on .com with an old repository' do + let(:on_com) { true } let(:created_at) { described_class::MIGRATION_PHASE_1_STARTED_AT - 3.months } - it { is_expected.to eq(nil) } + before do + allow(repository.gitlab_api_client).to receive(:supports_gitlab_api?).and_return(true) + allow(repository.gitlab_api_client).to receive(:repository_details).with(repository.path, sizing: :self).and_return(response) + expect(repository).to receive(:migration_state).and_return(migration_state) + end + + context 'with migration_state import_done' do + let(:response) { { 'size_bytes' => 12345 } } + let(:migration_state) { 'import_done' } + + it { is_expected.to eq(12345) } + end + + context 'with migration_state not import_done' do + let(:response) { { 'size_bytes' => 12345 } } + let(:migration_state) { 'default' } + + it { is_expected.to eq(nil) } + end end end diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index a58d32dfe5d..0a4ee73f3d3 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -139,29 +139,16 @@ RSpec.describe Deployment do end end - it 'executes deployment hooks' do + it 'executes Deployments::HooksWorker asynchronously' do freeze_time do - expect(deployment).to receive(:execute_hooks).with(Time.current) + expect(Deployments::HooksWorker) + .to receive(:perform_async).with(deployment_id: deployment.id, status: 'running', + status_changed_at: Time.current) deployment.run! end end - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - freeze_time do - expect(Deployments::HooksWorker) - .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current) - - deployment.run! - end - end - end - it 'executes Deployments::DropOlderDeploymentsWorker asynchronously' do expect(Deployments::DropOlderDeploymentsWorker) .to receive(:perform_async).once.with(deployment.id) @@ -189,28 +176,15 @@ RSpec.describe Deployment do deployment.succeed! end - it 'executes deployment hooks' do + it 'executes Deployments::HooksWorker asynchronously' do freeze_time do - expect(deployment).to receive(:execute_hooks).with(Time.current) + expect(Deployments::HooksWorker) + .to receive(:perform_async).with(deployment_id: deployment.id, status: 'success', + status_changed_at: Time.current) deployment.succeed! end end - - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - freeze_time do - expect(Deployments::HooksWorker) - .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current) - - deployment.succeed! - end - end - end end context 'when deployment failed' do @@ -232,28 +206,15 @@ RSpec.describe Deployment do deployment.drop! end - it 'executes deployment hooks' do + it 'executes Deployments::HooksWorker asynchronously' do freeze_time do - expect(deployment).to receive(:execute_hooks).with(Time.current) + expect(Deployments::HooksWorker) + .to receive(:perform_async).with(deployment_id: deployment.id, status: 'failed', + status_changed_at: Time.current) deployment.drop! end end - - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - freeze_time do - expect(Deployments::HooksWorker) - .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current) - - deployment.drop! - end - end - end end context 'when deployment was canceled' do @@ -275,28 +236,15 @@ RSpec.describe Deployment do deployment.cancel! end - it 'executes deployment hooks' do + it 'executes Deployments::HooksWorker asynchronously' do freeze_time do - expect(deployment).to receive(:execute_hooks).with(Time.current) + expect(Deployments::HooksWorker) + .to receive(:perform_async).with(deployment_id: deployment.id, status: 'canceled', + status_changed_at: Time.current) deployment.cancel! end end - - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'executes Deployments::HooksWorker asynchronously' do - freeze_time do - expect(Deployments::HooksWorker) - .to receive(:perform_async).with(deployment_id: deployment.id, status_changed_at: Time.current) - - deployment.cancel! - end - end - end end context 'when deployment was skipped' do @@ -324,12 +272,6 @@ RSpec.describe Deployment do deployment.skip! end end - - it 'does not execute deployment hooks' do - expect(deployment).not_to receive(:execute_hooks) - - deployment.skip! - end end context 'when deployment is blocked' do @@ -353,12 +295,6 @@ RSpec.describe Deployment do deployment.block! end - - it 'does not execute deployment hooks' do - expect(deployment).not_to receive(:execute_hooks) - - deployment.block! - end end describe 'synching status to Jira' do @@ -756,6 +692,37 @@ RSpec.describe Deployment do .to contain_exactly(stop_env_b) end end + + context 'When last deployment for environment is a retried build' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:environment_b) { create(:environment, project: project) } + + let(:build_a) do + create(:ci_build, :success, project: project, pipeline: pipeline, environment: environment.name) + end + + let(:build_b) do + create(:ci_build, :success, project: project, pipeline: pipeline, environment: environment_b.name) + end + + let!(:deployment_a) do + create(:deployment, :success, project: project, environment: environment, deployable: build_a) + end + + let!(:deployment_b) do + create(:deployment, :success, project: project, environment: environment_b, deployable: build_b) + end + + before do + # Retry build_b + build_b.update!(retried: true) + + # New successful build after retry. + create(:ci_build, :success, project: project, pipeline: pipeline, environment: environment_b.name) + end + + it { expect(subject_method(environment_b)).not_to be_nil } + end end end @@ -1052,30 +1019,11 @@ RSpec.describe Deployment do expect(Deployments::UpdateEnvironmentWorker).to receive(:perform_async) expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async) expect(Deployments::ArchiveInProjectWorker).to receive(:perform_async) + expect(Deployments::HooksWorker).to receive(:perform_async) expect(deploy.update_status('success')).to eq(true) end - context 'when `deployment_hooks_skip_worker` flag is disabled' do - before do - stub_feature_flags(deployment_hooks_skip_worker: false) - end - - it 'schedules `Deployments::HooksWorker` when finishing a deploy' do - expect(Deployments::HooksWorker).to receive(:perform_async) - - deploy.update_status('success') - end - end - - it 'executes deployment hooks when finishing a deploy' do - freeze_time do - expect(deploy).to receive(:execute_hooks).with(Time.current) - - deploy.update_status('success') - end - end - it 'updates finished_at when transitioning to a finished status' do freeze_time do deploy.update_status('success') diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index f377b34679c..d379ffeee02 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -260,32 +260,12 @@ RSpec.describe DiffNote do end context 'when the discussion was created in the diff' do - context 'when file_identifier_hash is disabled' do - before do - stub_feature_flags(file_identifier_hash: false) - end - - it 'returns correct diff file' do - diff_file = subject.diff_file - - expect(diff_file.old_path).to eq(position.old_path) - expect(diff_file.new_path).to eq(position.new_path) - expect(diff_file.diff_refs).to eq(position.diff_refs) - end - end - - context 'when file_identifier_hash is enabled' do - before do - stub_feature_flags(file_identifier_hash: true) - end - - it 'returns correct diff file' do - diff_file = subject.diff_file + it 'returns correct diff file' do + diff_file = subject.diff_file - expect(diff_file.old_path).to eq(position.old_path) - expect(diff_file.new_path).to eq(position.new_path) - expect(diff_file.diff_refs).to eq(position.diff_refs) - end + expect(diff_file.old_path).to eq(position.old_path) + expect(diff_file.new_path).to eq(position.new_path) + expect(diff_file.diff_refs).to eq(position.diff_refs) end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index fd89a3a2e22..e3207636bdc 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -34,6 +34,14 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do it { is_expected.to validate_length_of(:external_url).is_at_most(255) } + describe 'validation' do + it 'does not become invalid record when external_url is empty' do + environment = build(:environment, external_url: nil) + + expect(environment).to be_valid + end + end + describe '.before_save' do it 'ensures environment tier when a new object is created' do environment = build(:environment, name: 'gprd', tier: nil) @@ -1672,6 +1680,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do 'abcdef' | ChronicDuration::DurationParseError '' | nil nil | nil + 'never' | nil end with_them do it 'sets correct auto_stop_in' do @@ -1711,25 +1720,6 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end end - describe '#elastic_stack_available?' do - let!(:cluster) { create(:cluster, :project, :provided_by_user, projects: [project]) } - let!(:deployment) { create(:deployment, :success, environment: environment, project: project, cluster: cluster) } - - context 'when integration does not exist' do - it 'returns false' do - expect(environment.elastic_stack_available?).to be(false) - end - end - - context 'when integration is enabled' do - let!(:integration) { create(:clusters_integrations_elastic_stack, cluster: cluster) } - - it 'returns true' do - expect(environment.elastic_stack_available?).to be(true) - end - end - end - describe '#destroy' do it 'remove the deployment refs from gitaly' do deployment = create(:deployment, :success, environment: environment, project: project) diff --git a/spec/models/factories_spec.rb b/spec/models/factories_spec.rb index 45c3f93e6cf..2993b2aee58 100644 --- a/spec/models/factories_spec.rb +++ b/spec/models/factories_spec.rb @@ -104,6 +104,7 @@ RSpec.describe 'factories' do factories_based_on_view = %i[ postgres_index postgres_index_bloat_estimate + postgres_autovacuum_activity ].to_set.freeze without_fd, with_fd = FactoryBot.factories diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index d47f43a630d..e8e805b2678 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -42,6 +42,7 @@ RSpec.describe Group do it { is_expected.to have_many(:organizations).class_name('CustomerRelations::Organization') } it { is_expected.to have_one(:crm_settings) } it { is_expected.to have_one(:group_feature) } + it { is_expected.to have_one(:harbor_integration) } describe '#members & #requesters' do let(:requester) { create(:user) } @@ -726,7 +727,7 @@ RSpec.describe Group do context 'when user is a member of private group' do before do - private_group.add_user(user, Gitlab::Access::DEVELOPER) + private_group.add_member(user, Gitlab::Access::DEVELOPER) end it { is_expected.to match_array([private_group, internal_group, group]) } @@ -736,7 +737,7 @@ RSpec.describe Group do let!(:private_subgroup) { create(:group, :private, parent: private_group) } before do - private_subgroup.add_user(user, Gitlab::Access::DEVELOPER) + private_subgroup.add_member(user, Gitlab::Access::DEVELOPER) end it { is_expected.to match_array([private_subgroup, internal_group, group]) } @@ -848,7 +849,7 @@ RSpec.describe Group do expect(member).to receive(:refresh_member_authorized_projects).with(blocking: true) end - group.add_user(user, GroupMember::MAINTAINER) + group.add_member(user, GroupMember::MAINTAINER) expect(group.group_members.maintainers.map(&:user)).to include(user) end @@ -858,7 +859,7 @@ RSpec.describe Group do expect(member).to receive(:refresh_member_authorized_projects).with(blocking: false) end - group.add_user(user, GroupMember::MAINTAINER, blocking_refresh: false) + group.add_member(user, GroupMember::MAINTAINER, blocking_refresh: false) end end @@ -866,12 +867,12 @@ RSpec.describe Group do let(:user) { create(:user) } before do - group.add_users([user.id], GroupMember::GUEST) + group.add_members([user.id], GroupMember::GUEST) end it "updates the group permission" do expect(group.group_members.guests.map(&:user)).to include(user) - group.add_users([user.id], GroupMember::DEVELOPER) + group.add_members([user.id], GroupMember::DEVELOPER) expect(group.group_members.developers.map(&:user)).to include(user) expect(group.group_members.guests.map(&:user)).not_to include(user) end @@ -880,7 +881,7 @@ RSpec.describe Group do let!(:project) { create(:project, group: group) } before do - group.add_users([create(:user)], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: project.id) + group.add_members([create(:user)], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: project.id) end it 'creates a member_task with the correct attributes', :aggregate_failures do @@ -896,7 +897,7 @@ RSpec.describe Group do let(:user) { create(:user) } before do - group.add_user(user, GroupMember::MAINTAINER) + group.add_member(user, GroupMember::MAINTAINER) end it "is true if avatar is image" do @@ -993,7 +994,7 @@ RSpec.describe Group do context 'there is also a project_bot owner' do before do - group.add_user(create(:user, :project_bot), GroupMember::OWNER) + group.add_member(create(:user, :project_bot), GroupMember::OWNER) end it { expect(group.last_owner?(@members[:owner])).to be_truthy } @@ -1024,7 +1025,7 @@ RSpec.describe Group do let(:member) { blocked_user.group_members.last } before do - group.add_user(blocked_user, GroupMember::OWNER) + group.add_member(blocked_user, GroupMember::OWNER) end context 'when last_blocked_owner is set' do @@ -1050,7 +1051,7 @@ RSpec.describe Group do context 'with another active owner' do before do - group.add_user(create(:user), GroupMember::OWNER) + group.add_member(create(:user), GroupMember::OWNER) end it { expect(group.member_last_blocked_owner?(member)).to be(false) } @@ -1058,7 +1059,7 @@ RSpec.describe Group do context 'with 2 blocked owners' do before do - group.add_user(create(:user, :blocked), GroupMember::OWNER) + group.add_member(create(:user, :blocked), GroupMember::OWNER) end it { expect(group.member_last_blocked_owner?(member)).to be(false) } @@ -1082,7 +1083,7 @@ RSpec.describe Group do describe '#single_blocked_owner?' do context 'when there is only one blocked owner' do before do - group.add_user(blocked_user, GroupMember::OWNER) + group.add_member(blocked_user, GroupMember::OWNER) end it 'returns true' do @@ -1094,8 +1095,8 @@ RSpec.describe Group do let_it_be(:blocked_user_2) { create(:user, :blocked) } before do - group.add_user(blocked_user, GroupMember::OWNER) - group.add_user(blocked_user_2, GroupMember::OWNER) + group.add_member(blocked_user, GroupMember::OWNER) + group.add_member(blocked_user_2, GroupMember::OWNER) end it 'returns true' do @@ -1114,8 +1115,8 @@ RSpec.describe Group do let_it_be(:user) { create(:user) } before do - group.add_user(blocked_user, GroupMember::OWNER) - group.add_user(user, GroupMember::OWNER) + group.add_member(blocked_user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) end it 'has only blocked owners' do @@ -1129,7 +1130,7 @@ RSpec.describe Group do context 'when there is only one owner' do let!(:owner) do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) end it 'returns the owner' do @@ -1138,7 +1139,7 @@ RSpec.describe Group do context 'and there is also a project_bot owner' do before do - group.add_user(create(:user, :project_bot), GroupMember::OWNER) + group.add_member(create(:user, :project_bot), GroupMember::OWNER) end it 'returns only the human owner' do @@ -1151,11 +1152,11 @@ RSpec.describe Group do let_it_be(:user_2) { create(:user) } let!(:owner) do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) end let!(:owner2) do - group.add_user(user_2, GroupMember::OWNER) + group.add_member(user_2, GroupMember::OWNER) end it 'returns both owners' do @@ -1164,7 +1165,7 @@ RSpec.describe Group do context 'and there is also a project_bot owner' do before do - group.add_user(create(:user, :project_bot), GroupMember::OWNER) + group.add_member(create(:user, :project_bot), GroupMember::OWNER) end it 'returns only the human owners' do @@ -1186,7 +1187,7 @@ RSpec.describe Group do let(:member) { group.members.last } before do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) end context 'when last_owner is set' do @@ -1284,11 +1285,11 @@ RSpec.describe Group do requester: create(:user) } - group.add_user(members[:owner], GroupMember::OWNER) - group.add_user(members[:maintainer], GroupMember::MAINTAINER) - group.add_user(members[:developer], GroupMember::DEVELOPER) - group.add_user(members[:reporter], GroupMember::REPORTER) - group.add_user(members[:guest], GroupMember::GUEST) + group.add_member(members[:owner], GroupMember::OWNER) + group.add_member(members[:maintainer], GroupMember::MAINTAINER) + group.add_member(members[:developer], GroupMember::DEVELOPER) + group.add_member(members[:reporter], GroupMember::REPORTER) + group.add_member(members[:guest], GroupMember::GUEST) group.request_access(members[:requester]) members @@ -1464,8 +1465,8 @@ RSpec.describe Group do describe '#direct_members' do let_it_be(:group) { create(:group, :nested) } - let_it_be(:maintainer) { group.parent.add_user(create(:user), GroupMember::MAINTAINER) } - let_it_be(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) } + let_it_be(:maintainer) { group.parent.add_member(create(:user), GroupMember::MAINTAINER) } + let_it_be(:developer) { group.add_member(create(:user), GroupMember::DEVELOPER) } it 'does not return members of the parent' do expect(group.direct_members).not_to include(maintainer) @@ -1491,8 +1492,8 @@ RSpec.describe Group do shared_examples_for 'members_with_parents' do let!(:group) { create(:group, :nested) } - let!(:maintainer) { group.parent.add_user(create(:user), GroupMember::MAINTAINER) } - let!(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) } + let!(:maintainer) { group.parent.add_member(create(:user), GroupMember::MAINTAINER) } + let!(:developer) { group.add_member(create(:user), GroupMember::DEVELOPER) } let!(:pending_maintainer) { create(:group_member, :awaiting, :maintainer, group: group.parent) } let!(:pending_developer) { create(:group_member, :awaiting, :developer, group: group) } @@ -1603,9 +1604,9 @@ RSpec.describe Group do context 'members-related methods' do let!(:group) { create(:group, :nested) } let!(:sub_group) { create(:group, parent: group) } - let!(:maintainer) { group.parent.add_user(create(:user), GroupMember::MAINTAINER) } - let!(:developer) { group.add_user(create(:user), GroupMember::DEVELOPER) } - let!(:other_developer) { group.add_user(create(:user), GroupMember::DEVELOPER) } + let!(:maintainer) { group.parent.add_member(create(:user), GroupMember::MAINTAINER) } + let!(:developer) { group.add_member(create(:user), GroupMember::DEVELOPER) } + let!(:other_developer) { group.add_member(create(:user), GroupMember::DEVELOPER) } describe '#direct_and_indirect_members' do it 'returns parents members' do @@ -1619,7 +1620,7 @@ RSpec.describe Group do end describe '#direct_and_indirect_members_with_inactive' do - let!(:maintainer_blocked) { group.parent.add_user(create(:user, :blocked), GroupMember::MAINTAINER) } + let!(:maintainer_blocked) { group.parent.add_member(create(:user, :blocked), GroupMember::MAINTAINER) } it 'returns parents members' do expect(group.direct_and_indirect_members_with_inactive).to include(developer) @@ -1795,8 +1796,8 @@ RSpec.describe Group do maintainer = create(:user) developer = create(:user) - group.add_user(maintainer, GroupMember::MAINTAINER) - group.add_user(developer, GroupMember::DEVELOPER) + group.add_member(maintainer, GroupMember::MAINTAINER) + group.add_member(developer, GroupMember::DEVELOPER) expect(group.user_ids_for_project_authorizations) .to include(maintainer.id, developer.id) @@ -1847,7 +1848,7 @@ RSpec.describe Group do context 'group membership' do before do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) end it 'is called when require_two_factor_authentication is changed' do @@ -1870,7 +1871,7 @@ RSpec.describe Group do it 'calls #update_two_factor_requirement on each group member' do other_user = create(:user) - group.add_user(other_user, GroupMember::OWNER) + group.add_member(other_user, GroupMember::OWNER) calls = 0 allow_any_instance_of(User).to receive(:update_two_factor_requirement) do @@ -1885,7 +1886,7 @@ RSpec.describe Group do context 'sub groups and projects' do it 'enables two_factor_requirement for group member' do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) group.update!(require_two_factor_authentication: true) @@ -1899,7 +1900,7 @@ RSpec.describe Group do context 'two_factor_requirement is also enabled for ancestor group' do it 'enables two_factor_requirement for subgroup member' do subgroup = create(:group, :nested, parent: group) - subgroup.add_user(indirect_user, GroupMember::OWNER) + subgroup.add_member(indirect_user, GroupMember::OWNER) group.update!(require_two_factor_authentication: true) @@ -1910,7 +1911,7 @@ RSpec.describe Group do context 'two_factor_requirement is disabled for ancestor group' do it 'enables two_factor_requirement for subgroup member' do subgroup = create(:group, :nested, parent: group, require_two_factor_authentication: true) - subgroup.add_user(indirect_user, GroupMember::OWNER) + subgroup.add_member(indirect_user, GroupMember::OWNER) group.update!(require_two_factor_authentication: false) @@ -1919,7 +1920,7 @@ RSpec.describe Group do it 'enable two_factor_requirement for ancestor group member' do ancestor_group = create(:group) - ancestor_group.add_user(indirect_user, GroupMember::OWNER) + ancestor_group.add_member(indirect_user, GroupMember::OWNER) group.update!(parent: ancestor_group) group.update!(require_two_factor_authentication: true) @@ -1933,7 +1934,7 @@ RSpec.describe Group do context 'two_factor_requirement is enabled for ancestor group' do it 'enables two_factor_requirement for subgroup member' do subgroup = create(:group, :nested, parent: group) - subgroup.add_user(indirect_user, GroupMember::OWNER) + subgroup.add_member(indirect_user, GroupMember::OWNER) group.update!(require_two_factor_authentication: true) @@ -1944,7 +1945,7 @@ RSpec.describe Group do context 'two_factor_requirement is also disabled for ancestor group' do it 'disables two_factor_requirement for subgroup member' do subgroup = create(:group, :nested, parent: group) - subgroup.add_user(indirect_user, GroupMember::OWNER) + subgroup.add_member(indirect_user, GroupMember::OWNER) group.update!(require_two_factor_authentication: false) @@ -1954,7 +1955,7 @@ RSpec.describe Group do it 'disables two_factor_requirement for ancestor group member' do ancestor_group = create(:group, require_two_factor_authentication: false) indirect_user.update!(require_two_factor_authentication_from_group: true) - ancestor_group.add_user(indirect_user, GroupMember::OWNER) + ancestor_group.add_member(indirect_user, GroupMember::OWNER) group.update!(require_two_factor_authentication: false) diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 4253686b843..923a6f92424 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -15,6 +15,19 @@ RSpec.describe ProjectHook do subject { build(:project_hook, project: create(:project)) } end + describe '.for_projects' do + it 'finds related project hooks' do + hook_a = create(:project_hook) + hook_b = create(:project_hook) + hook_c = create(:project_hook) + + expect(described_class.for_projects([hook_a.project, hook_b.project])) + .to contain_exactly(hook_a, hook_b) + expect(described_class.for_projects(hook_c.project)) + .to contain_exactly(hook_c) + end + end + describe '.push_hooks' do it 'returns hooks for push events only' do hook = create(:project_hook, push_events: true) @@ -50,4 +63,62 @@ RSpec.describe ProjectHook do ) end end + + describe '#update_last_failure', :clean_gitlab_redis_shared_state do + let_it_be(:hook) { create(:project_hook) } + + it 'is a method of this class' do + expect { hook.update_last_failure }.not_to raise_error + end + + context 'when the hook is executable' do + it 'does not update the state' do + expect(Gitlab::Redis::SharedState).not_to receive(:with) + + hook.update_last_failure + end + end + + context 'when the hook is failed' do + before do + allow(hook).to receive(:executable?).and_return(false) + end + + def last_failure + Gitlab::Redis::SharedState.with do |redis| + redis.get("web_hooks:last_failure:project-#{hook.project.id}") + end + end + + context 'there is no prior value', :freeze_time do + it 'updates the state' do + expect { hook.update_last_failure }.to change { last_failure }.to(Time.current) + end + end + + context 'there is a prior value, from before now' do + it 'updates the state' do + the_future = 1.minute.from_now + + hook.update_last_failure + + travel_to(the_future) do + expect { hook.update_last_failure }.to change { last_failure }.to(the_future.iso8601) + end + end + end + + context 'there is a prior value, from after now' do + it 'does not update the state' do + the_past = 1.minute.ago + + hook.update_last_failure + + travel_to(the_past) do + expect { hook.update_last_failure }.not_to change { last_failure } + end + end + end + end + end end diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index fb4d1cee606..9faa5e1567c 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -78,6 +78,32 @@ RSpec.describe WebHook do expect(hook.url).to eq('https://example.com') end + + context 'when there are URL variables' do + subject { hook } + + before do + hook.url_variables = { 'one' => 'a', 'two' => 'b' } + end + + it { is_expected.to allow_value('http://example.com').for(:url) } + it { is_expected.to allow_value('http://example.com/{one}/{two}').for(:url) } + it { is_expected.to allow_value('http://example.com/{one}').for(:url) } + it { is_expected.to allow_value('http://example.com/{two}').for(:url) } + it { is_expected.to allow_value('http://user:s3cret@example.com/{two}').for(:url) } + it { is_expected.to allow_value('http://{one}:{two}@example.com').for(:url) } + + it { is_expected.not_to allow_value('http://example.com/{one}/{two}/{three}').for(:url) } + it { is_expected.not_to allow_value('http://example.com/{foo}').for(:url) } + it { is_expected.not_to allow_value('http:{user}:{pwd}//example.com/{foo}').for(:url) } + + it 'mentions all missing variable names' do + hook.url = 'http://example.com/{one}/{foo}/{two}/{three}' + + expect(hook).to be_invalid + expect(hook.errors[:url].to_sentence).to eq "Invalid URL template. Missing keys: [\"foo\", \"three\"]" + end + end end describe 'token' do @@ -161,8 +187,8 @@ RSpec.describe WebHook do end end - describe '.executable' do - let(:not_executable) do + describe '.executable/.disabled' do + let!(:not_executable) do [ [0, Time.current], [0, 1.minute.from_now], @@ -176,7 +202,7 @@ RSpec.describe WebHook do end end - let(:executables) do + let!(:executables) do [ [0, nil], [0, 1.day.ago], @@ -191,6 +217,7 @@ RSpec.describe WebHook do it 'finds the correct set of project hooks' do expect(described_class.where(project_id: project.id).executable).to match_array executables + expect(described_class.where(project_id: project.id).disabled).to match_array not_executable end context 'when the feature flag is not enabled' do @@ -198,7 +225,7 @@ RSpec.describe WebHook do stub_feature_flags(web_hooks_disable_failed: false) end - it 'is the same as all' do + specify 'enabled is the same as all' do expect(described_class.where(project_id: project.id).executable).to match_array(executables + not_executable) end end @@ -559,4 +586,60 @@ RSpec.describe WebHook do expect(hook.to_json(unsafe_serialization_hash: true)).not_to include('encrypted_url_variables') end end + + describe '#interpolated_url' do + subject(:hook) { build(:project_hook, project: project) } + + context 'when the hook URL does not contain variables' do + before do + hook.url = 'http://example.com' + end + + it { is_expected.to have_attributes(interpolated_url: hook.url) } + end + + it 'is not vulnerable to malicious input' do + hook.url = 'something%{%<foo>2147483628G}' + hook.url_variables = { 'foo' => '1234567890.12345678' } + + expect(hook).to have_attributes(interpolated_url: hook.url) + end + + context 'when the hook URL contains variables' do + before do + hook.url = 'http://example.com/{path}/resource?token={token}' + hook.url_variables = { 'path' => 'abc', 'token' => 'xyz' } + end + + it { is_expected.to have_attributes(interpolated_url: 'http://example.com/abc/resource?token=xyz') } + + context 'when a variable is missing' do + before do + hook.url_variables = { 'path' => 'present' } + end + + it 'raises an error' do + # We expect validations to prevent this entirely - this is not user-error + expect { hook.interpolated_url } + .to raise_error(described_class::InterpolationError, include('Missing key token')) + end + end + + context 'when the URL appears to include percent formatting' do + before do + hook.url = 'http://example.com/%{path}/resource?token=%{token}' + end + + it 'succeeds, interpolates correctly' do + expect(hook.interpolated_url).to eq 'http://example.com/%abc/resource?token=%xyz' + end + end + end + end + + describe '#update_last_failure' do + it 'is a method of this class' do + expect { described_class.new.update_last_failure }.not_to raise_error + end + end end diff --git a/spec/models/incident_management/issuable_escalation_status_spec.rb b/spec/models/incident_management/issuable_escalation_status_spec.rb index f956be3a04e..39d1fb325f5 100644 --- a/spec/models/incident_management/issuable_escalation_status_spec.rb +++ b/spec/models/incident_management/issuable_escalation_status_spec.rb @@ -11,7 +11,9 @@ RSpec.describe IncidentManagement::IssuableEscalationStatus do describe 'associations' do it { is_expected.to belong_to(:issue) } - it { is_expected.to have_one(:project).through(:issue) } + it do + is_expected.to have_one(:project).through(:issue).inverse_of(:incident_management_issuable_escalation_statuses) + end end describe 'validatons' do diff --git a/spec/models/integration_spec.rb b/spec/models/integration_spec.rb index 038018fbd0c..86074765c7b 100644 --- a/spec/models/integration_spec.rb +++ b/spec/models/integration_spec.rb @@ -799,7 +799,7 @@ RSpec.describe Integration do shared_examples '#api_field_names' do it 'filters out secret fields' do - safe_fields = %w[some_safe_field safe_field url trojan_gift] + safe_fields = %w[some_safe_field safe_field url trojan_gift api_only_field] expect(fake_integration.new).to have_attributes( api_field_names: match_array(safe_fields) @@ -807,6 +807,12 @@ RSpec.describe Integration do end end + shared_examples '#form_fields' do + it 'filters out API only fields' do + expect(fake_integration.new.form_fields.pluck(:name)).not_to include('api_only_field') + end + end + context 'when the class overrides #fields' do let(:fake_integration) do Class.new(Integration) do @@ -824,7 +830,8 @@ RSpec.describe Integration do { name: 'safe_field' }, { name: 'url' }, { name: 'trojan_horse', type: 'password' }, - { name: 'trojan_gift', type: 'gift' } + { name: 'trojan_gift', type: 'text' }, + { name: 'api_only_field', api_only: true } ].shuffle end end @@ -832,6 +839,7 @@ RSpec.describe Integration do it_behaves_like '#fields' it_behaves_like '#api_field_names' + it_behaves_like '#form_fields' end context 'when the class uses the field DSL' do @@ -849,12 +857,14 @@ RSpec.describe Integration do field :safe_field field :url field :trojan_horse, type: 'password' - field :trojan_gift, type: 'gift' + field :trojan_gift, type: 'text' + field :api_only_field, api_only: true end end it_behaves_like '#fields' it_behaves_like '#api_field_names' + it_behaves_like '#form_fields' end end @@ -1051,11 +1061,9 @@ RSpec.describe Integration do field :bar, type: 'password' field :password - field :with_help, - help: -> { 'help' } - - field :a_number, - type: 'number' + field :with_help, help: -> { 'help' } + field :select, type: 'select' + field :boolean, type: 'checkbox' end end @@ -1084,6 +1092,16 @@ RSpec.describe Integration do expect(integration).to be_foo_p_changed end + it 'provides boolean accessors for checkbox fields' do + expect(integration).to respond_to(:boolean) + expect(integration).to respond_to(:boolean?) + + expect(integration).not_to respond_to(:foo?) + expect(integration).not_to respond_to(:bar?) + expect(integration).not_to respond_to(:password?) + expect(integration).not_to respond_to(:select?) + end + it 'provides data fields' do integration.foo_dt = 3 expect(integration.foo_dt).to eq 3 @@ -1093,21 +1111,24 @@ RSpec.describe Integration do it 'registers fields in the fields list' do expect(integration.fields.pluck(:name)).to match_array %w[ - foo foo_p foo_dt bar password with_help a_number + foo foo_p foo_dt bar password with_help select boolean ] expect(integration.api_field_names).to match_array %w[ - foo foo_p foo_dt with_help a_number + foo foo_p foo_dt with_help select boolean ] end specify 'fields have expected attributes' do expect(integration.fields).to include( have_attributes(name: 'foo', type: 'text'), + have_attributes(name: 'foo_p', type: 'text'), + have_attributes(name: 'foo_dt', type: 'text'), have_attributes(name: 'bar', type: 'password'), have_attributes(name: 'password', type: 'password'), - have_attributes(name: 'a_number', type: 'number'), - have_attributes(name: 'with_help', help: 'help') + have_attributes(name: 'with_help', help: 'help'), + have_attributes(name: 'select', type: 'select'), + have_attributes(name: 'boolean', type: 'checkbox') ) end end @@ -1115,11 +1136,12 @@ RSpec.describe Integration do describe 'boolean_accessor' do let(:klass) do Class.new(Integration) do + prop_accessor :test_value boolean_accessor :test_value end end - let(:integration) { klass.new(properties: { test_value: input }) } + let(:integration) { klass.new(test_value: input) } where(:input, :method_result, :predicate_method_result) do true | true | true @@ -1149,6 +1171,35 @@ RSpec.describe Integration do test_value: be(method_result), test_value?: be(predicate_method_result) ) + + # Make sure the original value is stored correctly + expect(integration.send(:test_value_before_type_cast)).to eq(input) + expect(integration.properties).to include('test_value' => input) + end + + context 'when using data fields' do + let(:klass) do + Class.new(Integration) do + field :project_url, storage: :data_fields, type: 'checkbox' + + def data_fields + issue_tracker_data || self.build_issue_tracker_data + end + end + end + + let(:integration) { klass.new(project_url: input) } + + it 'has the correct value' do + expect(integration).to have_attributes( + project_url: be(method_result), + project_url?: be(predicate_method_result) + ) + + # Make sure the original value is stored correctly + expect(integration.send(:project_url_before_type_cast)).to eq(input == false ? 'false' : input) + expect(integration.properties).not_to include('project_url') + end end end @@ -1160,6 +1211,24 @@ RSpec.describe Integration do test_value?: be(false) ) end + + context 'when getter is not defined' do + let(:input) { true } + let(:klass) do + Class.new(Integration) do + boolean_accessor :test_value + end + end + + it 'defines a prop_accessor' do + expect(integration).to have_attributes( + test_value: true, + test_value?: true + ) + + expect(integration.properties['test_value']).to be(true) + end + end end describe '#attributes' do @@ -1218,7 +1287,6 @@ RSpec.describe Integration do it 'queues a Integrations::ExecuteWorker' do expect(Integrations::ExecuteWorker).to receive(:perform_async).with(integration.id, data) - expect(ProjectServiceWorker).not_to receive(:perform_async) async_execute end @@ -1232,18 +1300,5 @@ RSpec.describe Integration do async_execute end end - - context 'when the FF :rename_integration_workers is disabled' do - before do - stub_feature_flags(rename_integrations_workers: false) - end - - it 'queues a ProjectServiceWorker' do - expect(ProjectServiceWorker).to receive(:perform_async).with(integration.id, data) - expect(Integrations::ExecuteWorker).not_to receive(:perform_async) - - async_execute - end - end end end diff --git a/spec/models/integrations/base_chat_notification_spec.rb b/spec/models/integrations/base_chat_notification_spec.rb index 672d8de1e14..eb503e501d6 100644 --- a/spec/models/integrations/base_chat_notification_spec.rb +++ b/spec/models/integrations/base_chat_notification_spec.rb @@ -285,4 +285,22 @@ RSpec.describe Integrations::BaseChatNotification do expect { subject.webhook_placeholder }.to raise_error(NotImplementedError) end end + + describe '#event_channel_name' do + it 'returns the channel field name for the given event' do + expect(subject.event_channel_name(:event)).to eq('event_channel') + end + end + + describe '#event_channel_value' do + it 'returns the channel field value for the given event' do + subject.push_channel = '#pushes' + + expect(subject.event_channel_value(:push)).to eq('#pushes') + end + + it 'raises an error for unsupported events' do + expect { subject.event_channel_value(:foo) }.to raise_error(NoMethodError) + end + end end diff --git a/spec/models/integrations/chat_message/deployment_message_spec.rb b/spec/models/integrations/chat_message/deployment_message_spec.rb index 6bcd29c0a00..8da27ef5aa0 100644 --- a/spec/models/integrations/chat_message/deployment_message_spec.rb +++ b/spec/models/integrations/chat_message/deployment_message_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Integrations::ChatMessage::DeploymentMessage do let_it_be(:deployment) { create(:deployment, status: :success, deployable: ci_build, environment: environment, project: project, user: user, sha: commit.sha) } let(:args) do - Gitlab::DataBuilder::Deployment.build(deployment, Time.current) + Gitlab::DataBuilder::Deployment.build(deployment, 'success', Time.current) end it_behaves_like Integrations::ChatMessage diff --git a/spec/models/integrations/datadog_spec.rb b/spec/models/integrations/datadog_spec.rb index cfc44b22a84..47f916e8457 100644 --- a/spec/models/integrations/datadog_spec.rb +++ b/spec/models/integrations/datadog_spec.rb @@ -240,4 +240,20 @@ RSpec.describe Integrations::Datadog do end end end + + describe '#fields' do + it 'includes the archive_trace_events field' do + expect(instance.fields).to include(have_attributes(name: 'archive_trace_events')) + end + + context 'when the FF :datadog_integration_logs_collection is disabled' do + before do + stub_feature_flags(datadog_integration_logs_collection: false) + end + + it 'does not include the archive_trace_events field' do + expect(instance.fields).not_to include(have_attributes(name: 'archive_trace_events')) + end + end + end end diff --git a/spec/models/integrations/field_spec.rb b/spec/models/integrations/field_spec.rb index 6b1ce7fcbde..642fb1fbf7f 100644 --- a/spec/models/integrations/field_spec.rb +++ b/spec/models/integrations/field_spec.rb @@ -14,6 +14,37 @@ RSpec.describe ::Integrations::Field do end end + describe '#initialize' do + it 'sets type password for secret names' do + attrs[:name] = 'token' + attrs[:type] = 'text' + + expect(field[:type]).to eq('password') + end + + it 'uses the given type for other names' do + attrs[:name] = 'field' + attrs[:type] = 'select' + + expect(field[:type]).to eq('select') + end + + it 'raises an error if an invalid attribute is given' do + attrs[:foo] = 'foo' + attrs[:bar] = 'bar' + attrs[:name] = 'name' + attrs[:type] = 'text' + + expect { field }.to raise_error(ArgumentError, "Invalid attributes [:foo, :bar]") + end + + it 'raises an error if an invalid type is given' do + attrs[:type] = 'other' + + expect { field }.to raise_error(ArgumentError, 'Invalid type "other"') + end + end + describe '#name' do before do attrs[:name] = :foo @@ -59,7 +90,7 @@ RSpec.describe ::Integrations::Field do it 'has the correct default' do expect(field[name]).to have_correct_default - expect(field.send(name)).to have_correct_default + expect(field.public_send(name)).to have_correct_default end end @@ -69,32 +100,66 @@ RSpec.describe ::Integrations::Field do end it 'is known' do + next if name == :type + expect(field[name]).to eq(:known) - expect(field.send(name)).to eq(:known) + expect(field.public_send(name)).to eq(:known) end end context 'when set to a dynamic value' do it 'is computed' do + next if name == :type + attrs[name] = -> { Time.current } start = Time.current travel_to(start + 1.minute) do expect(field[name]).to be_after(start) - expect(field.send(name)).to be_after(start) + expect(field.public_send(name)).to be_after(start) end end it 'is executed in the class scope' do + next if name == :type + attrs[name] = -> { default_placeholder } expect(field[name]).to eq('my placeholder') - expect(field.send(name)).to eq('my placeholder') + expect(field.public_send(name)).to eq('my placeholder') end end end end + described_class::BOOLEAN_ATTRIBUTES.each do |name| + describe "##{name}?" do + it 'returns true if the value is truthy' do + attrs[name] = '' + expect(field.public_send("#{name}?")).to be(true) + end + + it 'returns false if the value is falsey' do + attrs[name] = nil + expect(field.public_send("#{name}?")).to be(false) + end + end + end + + described_class::TYPES.each do |type| + describe "##{type}?" do + it 'returns true if the type matches' do + attrs[:type] = type + expect(field.public_send("#{type}?")).to be(true) + end + + it 'returns false if the type does not match' do + attrs[:type] = (described_class::TYPES - [type]).first + expect(field.public_send("#{type}?")).to be(false) + end + end + end + describe '#secret?' do context 'when empty' do it { is_expected.not_to be_secret } diff --git a/spec/models/integrations/harbor_spec.rb b/spec/models/integrations/harbor_spec.rb index 9e3d4b524a6..5d8597969a1 100644 --- a/spec/models/integrations/harbor_spec.rb +++ b/spec/models/integrations/harbor_spec.rb @@ -19,6 +19,14 @@ RSpec.describe Integrations::Harbor do it { is_expected.to allow_value('helloworld').for(:password) } end + describe 'url' do + subject { build(:harbor_integration) } + + it { is_expected.not_to allow_value('https://192.168.1.1').for(:url) } + it { is_expected.not_to allow_value('https://127.0.0.1').for(:url) } + it { is_expected.to allow_value('https://demo.goharbor.io').for(:url)} + end + describe '#fields' do it 'returns custom fields' do expect(harbor_integration.fields.pluck(:name)).to eq(%w[url project_name username password]) diff --git a/spec/models/integrations/irker_spec.rb b/spec/models/integrations/irker_spec.rb index 16487aa36e7..e98b8b54e03 100644 --- a/spec/models/integrations/irker_spec.rb +++ b/spec/models/integrations/irker_spec.rb @@ -76,19 +76,5 @@ RSpec.describe Integrations::Irker do ensure conn.close if conn end - - context 'when the FF :rename_integrations_workers is disabled' do - before do - stub_feature_flags(rename_integrations_workers: false) - end - - it 'queues a IrkerWorker' do - expect(::IrkerWorker).to receive(:perform_async) - .with(project.id, irker.channels, colorize_messages, sample_data, irker.settings) - expect(Integrations::IrkerWorker).not_to receive(:perform_async) - - irker.execute(sample_data) - end - end end end diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb index 28d97b74adb..2a994540bd3 100644 --- a/spec/models/integrations/jira_spec.rb +++ b/spec/models/integrations/jira_spec.rb @@ -164,7 +164,7 @@ RSpec.describe Integrations::Jira do subject(:fields) { integration.fields } it 'returns custom fields' do - expect(fields.pluck(:name)).to eq(%w[url api_url username password]) + expect(fields.pluck(:name)).to eq(%w[url api_url username password jira_issue_transition_id]) end end diff --git a/spec/models/integrations/prometheus_spec.rb b/spec/models/integrations/prometheus_spec.rb index fbeaebfd807..ae965ed78d1 100644 --- a/spec/models/integrations/prometheus_spec.rb +++ b/spec/models/integrations/prometheus_spec.rb @@ -475,47 +475,4 @@ RSpec.describe Integrations::Prometheus, :use_clean_rails_memory_store_caching, end end end - - describe '#fields' do - let(:expected_fields) do - [ - { - type: 'checkbox', - name: 'manual_configuration', - title: s_('PrometheusService|Active'), - help: s_('PrometheusService|Select this checkbox to override the auto configuration settings with your own settings.'), - required: true - }, - { - type: 'text', - name: 'api_url', - title: 'API URL', - placeholder: s_('PrometheusService|https://prometheus.example.com/'), - help: s_('PrometheusService|The Prometheus API base URL.'), - required: true - }, - { - type: 'text', - name: 'google_iap_audience_client_id', - title: 'Google IAP Audience Client ID', - placeholder: s_('PrometheusService|IAP_CLIENT_ID.apps.googleusercontent.com'), - help: s_('PrometheusService|The ID of the IAP-secured resource.'), - autocomplete: 'off', - required: false - }, - { - type: 'textarea', - name: 'google_iap_service_account_json', - title: 'Google IAP Service Account JSON', - placeholder: s_('PrometheusService|{ "type": "service_account", "project_id": ... }'), - help: s_('PrometheusService|The contents of the credentials.json file of your service account.'), - required: false - } - ] - end - - it 'returns fields' do - expect(integration.fields).to eq(expected_fields) - end - end end diff --git a/spec/models/integrations/slack_spec.rb b/spec/models/integrations/slack_spec.rb index 3997d69f947..5801a4c3749 100644 --- a/spec/models/integrations/slack_spec.rb +++ b/spec/models/integrations/slack_spec.rb @@ -59,7 +59,7 @@ RSpec.describe Integrations::Slack do context 'deployment notification' do let_it_be(:deployment) { create(:deployment, user: user) } - let(:data) { Gitlab::DataBuilder::Deployment.build(deployment, Time.current) } + let(:data) { Gitlab::DataBuilder::Deployment.build(deployment, deployment.status, Time.current) } it_behaves_like 'increases the usage data counter', 'i_ecosystem_slack_service_deployment_notification' end diff --git a/spec/models/integrations/youtrack_spec.rb b/spec/models/integrations/youtrack_spec.rb index f6a9dd8ef37..618ebcbb76a 100644 --- a/spec/models/integrations/youtrack_spec.rb +++ b/spec/models/integrations/youtrack_spec.rb @@ -37,4 +37,10 @@ RSpec.describe Integrations::Youtrack do expect(described_class.reference_pattern.match('yt-123')[:issue]).to eq('yt-123') end end + + describe '#fields' do + it 'only returns the project_url and issues_url fields' do + expect(subject.fields.pluck(:name)).to eq(%w[project_url issues_url]) + end + end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index d45a23a7ef8..89c440dc49c 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -14,7 +14,6 @@ RSpec.describe Issue do it { is_expected.to belong_to(:milestone) } it { is_expected.to belong_to(:iteration) } it { is_expected.to belong_to(:project) } - it { is_expected.to have_one(:namespace).through(:project) } it { is_expected.to belong_to(:work_item_type).class_name('WorkItems::Type') } it { is_expected.to belong_to(:moved_to).class_name('Issue') } it { is_expected.to have_one(:moved_from).class_name('Issue') } @@ -132,6 +131,37 @@ RSpec.describe Issue do create(:issue) end end + + context 'issue namespace' do + let(:issue) { build(:issue, project: reusable_project) } + + it 'sets the namespace_id' do + expect(issue).to be_valid + expect(issue.namespace).to eq(reusable_project.project_namespace) + end + + context 'when issue is created' do + it 'sets the namespace_id' do + issue.save! + + expect(issue.reload.namespace).to eq(reusable_project.project_namespace) + end + end + + context 'when existing issue is saved' do + let(:issue) { create(:issue) } + + before do + issue.update!(namespace_id: nil) + end + + it 'sets the namespace id' do + issue.update!(title: "#{issue.title} and something extra") + + expect(issue.namespace).to eq(issue.project.project_namespace) + end + end + end end context 'order by upvotes' do @@ -651,28 +681,6 @@ RSpec.describe Issue do end end - describe '#has_related_branch?' do - let(:issue) { create(:issue, project: reusable_project, title: "Blue Bell Knoll") } - - subject { issue.has_related_branch? } - - context 'branch found' do - before do - allow(issue.project.repository).to receive(:branch_names).and_return(["iceblink-luck", issue.to_branch_name]) - end - - it { is_expected.to eq true } - end - - context 'branch not found' do - before do - allow(issue.project.repository).to receive(:branch_names).and_return(["lazy-calm"]) - end - - it { is_expected.to eq false } - end - end - it_behaves_like 'an editable mentionable' do subject { create(:issue, project: create(:project, :repository)) } @@ -744,25 +752,11 @@ RSpec.describe Issue do end describe '#participants' do - context 'using a public project' do - let_it_be(:public_project) { create(:project, :public) } - let_it_be(:issue) { create(:issue, project: public_project) } + it_behaves_like 'issuable participants' do + let_it_be(:issuable_parent) { create(:project, :public) } + let_it_be_with_refind(:issuable) { create(:issue, project: issuable_parent) } - let!(:note1) do - create(:note_on_issue, noteable: issue, project: public_project, note: 'a') - end - - let!(:note2) do - create(:note_on_issue, noteable: issue, project: public_project, note: 'b') - end - - it 'includes the issue author' do - expect(issue.participants).to include(issue.author) - end - - it 'includes the authors of the notes' do - expect(issue.participants).to include(note1.author, note2.author) - end + let(:params) { { noteable: issuable, project: issuable_parent } } end context 'using a private project' do diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index a9d1a8a5ef2..b98c0e8eae0 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -47,10 +47,9 @@ RSpec.describe Key, :mailer do end describe 'validation of banned keys' do - let_it_be(:user) { create(:user) } - let(:key) { build(:key) } - let(:banned_keys) do + + where(:key_content) do [ 'ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAIEAwRIdDlHaIqZXND/l1vFT7ue3rc/DvXh2y' \ 'x5EFtuxGQRHVxGMazDhV4vj5ANGXDQwUYI0iZh6aOVrDy8I/y9/y+YDGCvsnqrDbuPDjW' \ @@ -131,68 +130,13 @@ RSpec.describe Key, :mailer do ] end - context 'when ssh_banned_key feature flag is enabled with a user' do - before do - stub_feature_flags(ssh_banned_key: user) - end - - where(:key_content) { banned_keys } - - with_them do - it 'does not allow banned keys' do - key.key = key_content - key.user = user - - expect(key).to be_invalid - expect(key.errors[:key]).to include( - _('cannot be used because it belongs to a compromised private key. Stop using this key and generate a new one.')) - end - - it 'allows when the user is a ghost user' do - key.key = key_content - key.user = User.ghost - - expect(key).to be_valid - end - - it 'allows when the user is nil' do - key.key = key_content - key.user = nil - - expect(key).to be_valid - end - end - - it 'allows other keys' do - key.user = user - - expect(key).to be_valid - end - - it 'allows other users' do - key.user = User.ghost - - expect(key).to be_valid - end - end - - context 'when ssh_banned_key feature flag is disabled' do - before do - stub_feature_flags(ssh_banned_key: false) - end - - where(:key_content) { banned_keys } + with_them do + it 'does not allow banned keys' do + key.key = key_content - with_them do - it 'allows banned keys' do - key.key = key_content - - expect(key).to be_valid - end - end - - it 'allows other keys' do - expect(key).to be_valid + expect(key).to be_invalid + expect(key.errors[:key]).to include( + _('cannot be used because it belongs to a compromised private key. Stop using this key and generate a new one.')) end end end @@ -296,6 +240,39 @@ RSpec.describe Key, :mailer do end end + describe '#ensure_sha256_fingerprint!' do + let_it_be_with_reload(:user_key) { create(:personal_key) } + + context 'with a valid SHA256 fingerprint' do + it 'does nothing' do + expect(user_key).not_to receive(:generate_fingerprint) + + user_key.ensure_sha256_fingerprint! + end + end + + context 'with a missing SHA256 fingerprint' do + before do + user_key.update_column(:fingerprint_sha256, nil) + user_key.ensure_sha256_fingerprint! + end + + it 'fingerprints are present' do + expect(user_key.reload.fingerprint_sha256).to be_present + end + end + + context 'with an invalid public key' do + before do + user_key.update_column(:key, 'a') + end + + it 'does not throw an exception' do + expect { user_key.ensure_sha256_fingerprint! }.not_to raise_error + end + end + end + context 'fingerprint generation' do it 'generates both md5 and sha256 fingerprints' do key = build(:rsa_key_4096) diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index f93c2d36966..94032146f51 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -219,7 +219,7 @@ RSpec.describe GroupMember do end context 'on create' do - let(:action) { group.add_user(user, Gitlab::Access::GUEST) } + let(:action) { group.add_member(user, Gitlab::Access::GUEST) } let(:blocking) { true } it 'changes access level', :sidekiq_inline do @@ -241,7 +241,7 @@ RSpec.describe GroupMember do context 'on update' do before do - group.add_user(user, Gitlab::Access::GUEST) + group.add_member(user, Gitlab::Access::GUEST) end let(:action) { group.members.find_by(user: user).update!(access_level: Gitlab::Access::DEVELOPER) } @@ -266,7 +266,7 @@ RSpec.describe GroupMember do context 'on destroy' do before do - group.add_user(user, Gitlab::Access::GUEST) + group.add_member(user, Gitlab::Access::GUEST) end let(:action) { group.members.find_by(user: user).destroy! } diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 8c989f5aaca..39d9d25a98c 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -111,12 +111,12 @@ RSpec.describe ProjectMember do end end - describe '.add_users_to_projects' do + describe '.add_members_to_projects' do it 'adds the given users to the given projects' do projects = create_list(:project, 2) users = create_list(:user, 2) - described_class.add_users_to_projects( + described_class.add_members_to_projects( [projects.first.id, projects.second.id], [users.first.id, users.second], described_class::MAINTAINER) @@ -174,8 +174,8 @@ RSpec.describe ProjectMember do expect { project.destroy! }.to change { user.can?(:guest_access, project) }.from(true).to(false) end - it 'refreshes the authorization without calling AuthorizedProjectUpdate::ProjectRecalculatePerUserService' do - expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserService).not_to receive(:new) + it 'refreshes the authorization without calling AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker' do + expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker).not_to receive(:bulk_perform_and_wait) project.destroy! end @@ -199,7 +199,7 @@ RSpec.describe ProjectMember do context 'when importing' do it 'does not refresh' do - expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserService).not_to receive(:new) + expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker).not_to receive(:bulk_perform_and_wait) member = build(:project_member) member.importing = true @@ -212,11 +212,11 @@ RSpec.describe ProjectMember do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - shared_examples_for 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations' do - it 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService' do - expect_next_instance_of(AuthorizedProjectUpdate::ProjectRecalculatePerUserService, project, user) do |service| - expect(service).to receive(:execute) - end + shared_examples_for 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker inline to recalculate authorizations' do + it 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker' do + expect(AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker).to receive(:bulk_perform_and_wait).with( + [[project.id, user.id]] + ) action end @@ -236,13 +236,13 @@ RSpec.describe ProjectMember do end context 'on create' do - let(:action) { project.add_user(user, Gitlab::Access::GUEST) } + let(:action) { project.add_member(user, Gitlab::Access::GUEST) } it 'changes access level' do expect { action }.to change { user.can?(:guest_access, project) }.from(false).to(true) end - it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations' + it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker inline to recalculate authorizations' it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' end @@ -250,14 +250,14 @@ RSpec.describe ProjectMember do let(:action) { project.members.find_by(user: user).update!(access_level: Gitlab::Access::DEVELOPER) } before do - project.add_user(user, Gitlab::Access::GUEST) + project.add_member(user, Gitlab::Access::GUEST) end it 'changes access level' do expect { action }.to change { user.can?(:developer_access, project) }.from(false).to(true) end - it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserService to recalculate authorizations' + it_behaves_like 'calls AuthorizedProjectUpdate::ProjectRecalculatePerUserWorker inline to recalculate authorizations' it_behaves_like 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' end @@ -265,7 +265,7 @@ RSpec.describe ProjectMember do let(:action) { project.members.find_by(user: user).destroy! } before do - project.add_user(user, Gitlab::Access::GUEST) + project.add_member(user, Gitlab::Access::GUEST) end it 'changes access level', :sidekiq_inline do diff --git a/spec/models/merge_request_diff_file_spec.rb b/spec/models/merge_request_diff_file_spec.rb index c9bcb900eca..7dc550a6c93 100644 --- a/spec/models/merge_request_diff_file_spec.rb +++ b/spec/models/merge_request_diff_file_spec.rb @@ -13,50 +13,53 @@ RSpec.describe MergeRequestDiffFile do let(:invalid_items_for_bulk_insertion) { [] } # class does not have any validations defined end + let(:unpacked) { 'unpacked' } + let(:packed) { [unpacked].pack('m0') } + let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first } + describe '#diff' do + let(:file) { build(:merge_request_diff_file) } + context 'when diff is not stored' do let(:unpacked) { 'unpacked' } let(:packed) { [unpacked].pack('m0') } before do - subject.diff = packed + file.diff = packed end context 'when the diff is marked as binary' do before do - subject.binary = true + file.binary = true end it 'unpacks from base 64' do - expect(subject.diff).to eq(unpacked) + expect(file.diff).to eq(unpacked) end context 'invalid base64' do let(:packed) { '---/dev/null' } it 'returns the raw diff' do - expect(subject.diff).to eq(packed) + expect(file.diff).to eq(packed) end end end context 'when the diff is not marked as binary' do it 'returns the raw diff' do - expect(subject.diff).to eq(packed) + expect(file.diff).to eq(packed) end end end context 'when diff is stored in DB' do - let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first } - it 'returns UTF-8 string' do expect(file.diff.encoding).to eq Encoding::UTF_8 end end context 'when diff is stored in external storage' do - let(:file) { create(:merge_request).merge_request_diff.merge_request_diff_files.first } let(:test_dir) { 'tmp/tests/external-diffs' } around do |example| @@ -81,17 +84,141 @@ RSpec.describe MergeRequestDiffFile do describe '#utf8_diff' do it 'does not raise error when the diff is binary' do - subject.diff = "\x05\x00\x68\x65\x6c\x6c\x6f" + file = build(:merge_request_diff_file) + file.diff = "\x05\x00\x68\x65\x6c\x6c\x6f" - expect { subject.utf8_diff }.not_to raise_error + expect { file.utf8_diff }.not_to raise_error end it 'calls #diff once' do - allow(subject).to receive(:diff).and_return('test') + allow(file).to receive(:diff).and_return('test') + + expect(file).to receive(:diff).once + + file.utf8_diff + end + + context 'externally stored diff caching' do + let(:test_dir) { 'tmp/tests/external-diffs' } + + around do |example| + FileUtils.mkdir_p(test_dir) + + begin + example.run + ensure + FileUtils.rm_rf(test_dir) + end + end + + before do + stub_external_diffs_setting(enabled: true, storage_path: test_dir) + end + + context 'when external diff is not cached' do + it 'caches external diffs' do + expect(file.merge_request_diff).to receive(:cache_external_diff).and_call_original + + expect(file.utf8_diff).to eq(file.diff) + end + end + + context 'when external diff is already cached' do + it 'reads diff from cached external diff' do + file_stub = double + + allow(file.merge_request_diff).to receive(:cached_external_diff).and_yield(file_stub) + expect(file_stub).to receive(:seek).with(file.external_diff_offset) + expect(file_stub).to receive(:read).with(file.external_diff_size) + + file.utf8_diff + end + end + + context 'when the diff is marked as binary' do + let(:file) { build(:merge_request_diff_file) } - expect(subject).to receive(:diff).once + before do + allow(file.merge_request_diff).to receive(:stored_externally?).and_return(true) + allow(file.merge_request_diff).to receive(:cached_external_diff).and_return(packed) + end - subject.utf8_diff + context 'when the diff is marked as binary' do + before do + file.binary = true + end + + it 'unpacks from base 64' do + expect(file.utf8_diff).to eq(unpacked) + end + + context 'invalid base64' do + let(:packed) { '---/dev/null' } + + it 'returns the raw diff' do + expect(file.utf8_diff).to eq(packed) + end + end + end + + context 'when the diff is not marked as binary' do + it 'returns the raw diff' do + expect(file.utf8_diff).to eq(packed) + end + end + end + + context 'when content responds to #encoding' do + it 'encodes content to utf8 encoding' do + expect(file.utf8_diff.encoding).to eq(Encoding::UTF_8) + end + end + + context 'when content is blank' do + it 'returns an empty string' do + allow(file.merge_request_diff).to receive(:cached_external_diff).and_return(nil) + + expect(file.utf8_diff).to eq('') + end + end + + context 'when exception is raised' do + it 'falls back to #diff' do + allow(file).to receive(:binary?).and_raise(StandardError, 'Error!') + expect(file).to receive(:diff) + expect(Gitlab::AppLogger) + .to receive(:warn) + .with( + a_hash_including( + :message => 'Cached external diff export failed', + :merge_request_diff_file_id => file.id, + :merge_request_diff_id => file.merge_request_diff.id, + 'exception.class' => 'StandardError', + 'exception.message' => 'Error!' + ) + ) + + file.utf8_diff + end + end + end + + context 'when externally_stored_diffs_caching_export feature flag is disabled' do + it 'calls #diff' do + stub_feature_flags(externally_stored_diffs_caching_export: false) + + expect(file).to receive(:diff) + + file.utf8_diff + end + end + + context 'when diff is not stored externally' do + it 'calls #diff' do + expect(file).to receive(:diff) + + file.utf8_diff + end end end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index afe7251f59a..007e84164a8 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -1120,4 +1120,101 @@ RSpec.describe MergeRequestDiff do expect(described_class.latest_diff_for_merge_requests(nil)).to be_empty end end + + context 'external diff caching' do + let(:test_dir) { 'tmp/tests/external-diffs' } + let(:cache_dir) { File.join(Dir.tmpdir, "project-#{diff.project.id}-external-mr-#{diff.merge_request_id}-diff-#{diff.id}-cache") } + let(:cache_filepath) { File.join(cache_dir, "diff-#{diff.id}") } + let(:external_diff_content) { diff.opening_external_diff { |diff| diff.read } } + + around do |example| + FileUtils.mkdir_p(test_dir) + + begin + example.run + ensure + FileUtils.rm_rf(test_dir) + end + end + + before do + stub_external_diffs_setting(enabled: true, storage_path: test_dir) + end + + subject(:diff) { diff_with_commits } + + describe '#cached_external_diff' do + context 'when diff is externally stored' do + context 'when diff is already cached' do + it 'yields cached file' do + Dir.mkdir(cache_dir) + File.open(cache_filepath, 'wb') { |f| f.write(external_diff_content) } + + expect(diff).not_to receive(:cache_external_diff) + + expect { |b| diff.cached_external_diff(&b) }.to yield_with_args(File) + end + end + + context 'when diff is not cached' do + it 'caches external diff in tmp storage' do + expect(diff).to receive(:cache_external_diff).and_call_original + expect(File.exist?(cache_filepath)).to eq(false) + expect { |b| diff.cached_external_diff(&b) }.to yield_with_args(File) + expect(File.exist?(cache_filepath)).to eq(true) + expect(File.read(cache_filepath)).to eq(external_diff_content) + end + end + end + + context 'when diff is not externally stored' do + it 'yields nil' do + stub_external_diffs_setting(enabled: false) + + expect { |b| diff.cached_external_diff(&b) }.to yield_with_args(nil) + end + end + end + + describe '#remove_cached_external_diff' do + before do + diff.cached_external_diff { |diff| diff } + end + + it 'removes external diff cache diff' do + expect(Dir.exist?(cache_dir)).to eq(true) + + diff.remove_cached_external_diff + + expect(Dir.exist?(cache_dir)).to eq(false) + end + + context 'when path is traversed' do + it 'raises' do + allow(diff).to receive(:external_diff_cache_dir).and_return(File.join(cache_dir, '..')) + + expect { diff.remove_cached_external_diff }.to raise_error(Gitlab::Utils::PathTraversalAttackError, 'Invalid path') + end + end + + context 'when path is not allowed' do + it 'raises' do + allow(diff).to receive(:external_diff_cache_dir).and_return('/') + + expect { diff.remove_cached_external_diff }.to raise_error(StandardError, 'path / is not allowed') + end + end + + context 'when dir does not exist' do + it 'returns' do + FileUtils.rm_rf(cache_dir) + + expect(Dir.exist?(cache_dir)).to eq(false) + expect(FileUtils).not_to receive(:rm_rf).with(cache_dir) + + diff.remove_cached_external_diff + end + end + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 381eccf2376..c3e325c4e6c 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -658,7 +658,7 @@ RSpec.describe MergeRequest, factory_default: :keep do end before do - project.add_user(user, :developer) + project.add_member(user, :developer) end describe '.total_time_to_merge' do @@ -4286,6 +4286,18 @@ RSpec.describe MergeRequest, factory_default: :keep do end end + describe 'transition to closed' do + context 'with merge error' do + subject { create(:merge_request, merge_error: 'merge error') } + + it 'clears merge error' do + subject.close! + + expect(subject.reload.merge_error).to eq(nil) + end + end + end + describe 'transition to cannot_be_merged' do let(:notification_service) { double(:notification_service) } let(:todo_service) { double(:todo_service) } @@ -4903,7 +4915,7 @@ RSpec.describe MergeRequest, factory_default: :keep do .to delegate_method(:builds_with_coverage) .to(:head_pipeline) .with_prefix - .with_arguments(allow_nil: true) + .allow_nil end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 96e06e617d5..664cdb27290 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -32,6 +32,7 @@ RSpec.describe Namespace do it { is_expected.to have_one :namespace_route } it { is_expected.to have_many :namespace_members } it { is_expected.to have_one :cluster_enabled_grant } + it { is_expected.to have_many(:work_items) } it do is_expected.to have_one(:ci_cd_settings).class_name('NamespaceCiCdSetting').inverse_of(:namespace).autosave(true) @@ -337,16 +338,13 @@ RSpec.describe Namespace do end describe 'delegate' do - it { is_expected.to delegate_method(:name).to(:owner).with_prefix.with_arguments(allow_nil: true) } - it { is_expected.to delegate_method(:avatar_url).to(:owner).with_arguments(allow_nil: true) } - it do - is_expected.to delegate_method(:prevent_sharing_groups_outside_hierarchy) - .to(:namespace_settings).with_arguments(allow_nil: true) - end + it { is_expected.to delegate_method(:name).to(:owner).with_prefix.allow_nil } + it { is_expected.to delegate_method(:avatar_url).to(:owner).allow_nil } + it { is_expected.to delegate_method(:prevent_sharing_groups_outside_hierarchy).to(:namespace_settings).allow_nil } it do - is_expected.to delegate_method(:prevent_sharing_groups_outside_hierarchy=) - .to(:namespace_settings).with_arguments(allow_nil: true) + is_expected.to delegate_method(:prevent_sharing_groups_outside_hierarchy=).to(:namespace_settings) + .with_arguments(:args).allow_nil end end @@ -1886,17 +1884,39 @@ RSpec.describe Namespace do end end + describe '#emails_enabled?' do + it "is the opposite of emails_disabled" do + group = create(:group, emails_disabled: false) + + expect(group.emails_enabled?).to be_truthy + end + end + describe '#pages_virtual_domain' do let(:project) { create(:project, namespace: namespace) } + let(:virtual_domain) { namespace.pages_virtual_domain } - it 'returns the virual domain' do + before do project.mark_pages_as_deployed project.update_pages_deployment!(create(:pages_deployment, project: project)) + end - virtual_domain = namespace.pages_virtual_domain - + it 'returns the virual domain' do expect(virtual_domain).to be_an_instance_of(Pages::VirtualDomain) expect(virtual_domain.lookup_paths).not_to be_empty + expect(virtual_domain.cache_key).to eq("pages_domain_for_namespace_#{namespace.root_ancestor.id}") + end + + context 'when :cache_pages_domain_api is disabled' do + before do + stub_feature_flags(cache_pages_domain_api: false) + end + + it 'returns the virual domain' do + expect(virtual_domain).to be_an_instance_of(Pages::VirtualDomain) + expect(virtual_domain.lookup_paths).not_to be_empty + expect(virtual_domain.cache_key).to be_nil + end end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 4b262c1f3a9..fc6f7832c2c 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -106,6 +106,22 @@ RSpec.describe Note do end end + describe 'created_at in the past' do + let_it_be(:noteable) { create(:issue) } + + context 'when creating a note not too much in the past' do + subject { build(:note, project: noteable.project, noteable: noteable, created_at: '1990-05-06') } + + it { is_expected.to be_valid } + end + + context 'when creating a note too much in the past' do + subject { build(:note, project: noteable.project, noteable: noteable, created_at: '1600-05-06') } + + it { is_expected.not_to be_valid } + end + end + describe 'confidentiality' do context 'for existing public note' do let_it_be(:existing_note) { create(:note) } diff --git a/spec/models/oauth_access_token_spec.rb b/spec/models/oauth_access_token_spec.rb index 65a7f6410cf..2b47da1ebe1 100644 --- a/spec/models/oauth_access_token_spec.rb +++ b/spec/models/oauth_access_token_spec.rb @@ -7,22 +7,40 @@ RSpec.describe OauthAccessToken do let(:app_one) { create(:oauth_application) } let(:app_two) { create(:oauth_application) } let(:app_three) { create(:oauth_application) } - let(:tokens) { described_class.all } + let(:token) { create(:oauth_access_token, application_id: app_one.id) } - before do - create(:oauth_access_token, application_id: app_one.id) - create_list(:oauth_access_token, 2, resource_owner: user, application_id: app_two.id) - end + describe 'scopes' do + describe '.distinct_resource_owner_counts' do + let(:tokens) { described_class.all } + + before do + token + create_list(:oauth_access_token, 2, resource_owner: user, application_id: app_two.id) + end + + it 'returns unique owners' do + expect(tokens.count).to eq(3) + expect(tokens.distinct_resource_owner_counts([app_one])).to eq({ app_one.id => 1 }) + expect(tokens.distinct_resource_owner_counts([app_two])).to eq({ app_two.id => 1 }) + expect(tokens.distinct_resource_owner_counts([app_three])).to eq({}) + expect(tokens.distinct_resource_owner_counts([app_one, app_two])) + .to eq({ + app_one.id => 1, + app_two.id => 1 + }) + end + end + + describe '.latest_per_application' do + let!(:app_two_token1) { create(:oauth_access_token, application: app_two) } + let!(:app_two_token2) { create(:oauth_access_token, application: app_two) } + let!(:app_three_token1) { create(:oauth_access_token, application: app_three) } + let!(:app_three_token2) { create(:oauth_access_token, application: app_three) } - it 'returns unique owners' do - expect(tokens.count).to eq(3) - expect(tokens.distinct_resource_owner_counts([app_one])).to eq({ app_one.id => 1 }) - expect(tokens.distinct_resource_owner_counts([app_two])).to eq({ app_two.id => 1 }) - expect(tokens.distinct_resource_owner_counts([app_three])).to eq({}) - expect(tokens.distinct_resource_owner_counts([app_one, app_two])) - .to eq({ - app_one.id => 1, - app_two.id => 1 - }) + it 'returns only the latest token for each application' do + expect(described_class.latest_per_application.map(&:id)) + .to match_array([app_two_token2.id, app_three_token2.id]) + end + end end end diff --git a/spec/models/operations/feature_flags_client_spec.rb b/spec/models/operations/feature_flags_client_spec.rb index 05988d676f3..2ed3222c65c 100644 --- a/spec/models/operations/feature_flags_client_spec.rb +++ b/spec/models/operations/feature_flags_client_spec.rb @@ -3,7 +3,15 @@ require 'spec_helper' RSpec.describe Operations::FeatureFlagsClient do - subject { create(:operations_feature_flags_client) } + let_it_be(:project) { create(:project) } + + let!(:client) { create(:operations_feature_flags_client, project: project) } + + subject { client } + + before do + client.unleash_app_name = 'production' + end describe 'associations' do it { is_expected.to belong_to(:project) } @@ -18,4 +26,64 @@ RSpec.describe Operations::FeatureFlagsClient do expect(subject.token).not_to be_empty end end + + describe '.update_last_feature_flag_updated_at!' do + subject { described_class.update_last_feature_flag_updated_at!(project) } + + it 'updates the last_feature_flag_updated_at of the project client' do + freeze_time do + expect { subject }.to change { client.reload.last_feature_flag_updated_at }.from(nil).to(Time.current) + end + end + end + + describe '#unleash_api_version' do + subject { client.unleash_api_version } + + it { is_expected.to eq(described_class::DEFAULT_UNLEASH_API_VERSION) } + end + + describe '#unleash_api_features' do + subject { client.unleash_api_features } + + it 'fetches' do + expect(Operations::FeatureFlag).to receive(:for_unleash_client).with(project, 'production').once + + subject + end + + context 'when unleash app name is not set' do + before do + client.unleash_app_name = nil + end + + it 'does not fetch' do + expect(Operations::FeatureFlag).not_to receive(:for_unleash_client) + + subject + end + end + end + + describe '#unleash_api_cache_key' do + subject { client.unleash_api_cache_key } + + it 'constructs the cache key' do + is_expected.to eq("api_version:#{client.unleash_api_version}"\ + ":app_name:#{client.unleash_app_name}"\ + ":updated_at:#{client.last_feature_flag_updated_at.to_i}") + end + + context 'when unleash app name is not set' do + before do + client.unleash_app_name = nil + end + + it 'constructs the cache key without unleash app name' do + is_expected.to eq("api_version:#{client.unleash_api_version}"\ + ":app_name:"\ + ":updated_at:#{client.last_feature_flag_updated_at.to_i}") + end + end + end end diff --git a/spec/models/packages/cleanup/policy_spec.rb b/spec/models/packages/cleanup/policy_spec.rb index c08ae4aa7e7..a37042520e7 100644 --- a/spec/models/packages/cleanup/policy_spec.rb +++ b/spec/models/packages/cleanup/policy_spec.rb @@ -25,4 +25,40 @@ RSpec.describe Packages::Cleanup::Policy, type: :model do it { is_expected.to contain_exactly(active_policy) } end + + describe '.with_packages' do + let_it_be(:policy_with_packages) { create(:packages_cleanup_policy) } + let_it_be(:policy_without_packages) { create(:packages_cleanup_policy) } + let_it_be(:package) { create(:package, project: policy_with_packages.project) } + + subject { described_class.with_packages } + + it { is_expected.to contain_exactly(policy_with_packages) } + end + + describe '.runnable' do + let_it_be(:runnable_policy_with_packages) { create(:packages_cleanup_policy, :runnable) } + let_it_be(:runnable_policy_without_packages) { create(:packages_cleanup_policy, :runnable) } + let_it_be(:non_runnable_policy_with_packages) { create(:packages_cleanup_policy) } + let_it_be(:non_runnable_policy_without_packages) { create(:packages_cleanup_policy) } + + let_it_be(:package1) { create(:package, project: runnable_policy_with_packages.project) } + let_it_be(:package2) { create(:package, project: non_runnable_policy_with_packages.project) } + + subject { described_class.runnable } + + it { is_expected.to contain_exactly(runnable_policy_with_packages) } + end + + describe '#keep_n_duplicated_package_files_disabled?' do + subject { policy.keep_n_duplicated_package_files_disabled? } + + %w[all 1].each do |value| + context "with value set to #{value}" do + let(:policy) { build(:packages_cleanup_policy, keep_n_duplicated_package_files: value) } + + it { is_expected.to eq(value == 'all') } + end + end + end end diff --git a/spec/models/packages/debian/file_entry_spec.rb b/spec/models/packages/debian/file_entry_spec.rb index e981adf69bc..ed6372f2873 100644 --- a/spec/models/packages/debian/file_entry_spec.rb +++ b/spec/models/packages/debian/file_entry_spec.rb @@ -31,6 +31,13 @@ RSpec.describe Packages::Debian::FileEntry, type: :model do describe 'validations' do it { is_expected.to be_valid } + context 'with FIPS mode', :fips_mode do + it 'raises an error' do + expect { subject.validate! } + .to raise_error(::Packages::FIPS::DisabledError, 'Debian registry is not FIPS compliant') + end + end + describe '#filename' do it { is_expected.to validate_presence_of(:filename) } it { is_expected.not_to allow_value('Hé').for(:filename) } diff --git a/spec/models/pages/virtual_domain_spec.rb b/spec/models/pages/virtual_domain_spec.rb index 29c14cbeb3e..b5a421295b2 100644 --- a/spec/models/pages/virtual_domain_spec.rb +++ b/spec/models/pages/virtual_domain_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Pages::VirtualDomain do let(:domain) { nil } let(:project) { instance_double(Project) } - subject(:virtual_domain) { described_class.new([project], domain: domain) } + subject(:virtual_domain) { described_class.new(projects: [project], domain: domain) } it 'returns nil if there is no domain provided' do expect(virtual_domain.certificate).to be_nil @@ -35,7 +35,7 @@ RSpec.describe Pages::VirtualDomain do context 'when there is pages domain provided' do let(:domain) { instance_double(PagesDomain) } - subject(:virtual_domain) { described_class.new([project_a, project_b, project_c], domain: domain) } + subject(:virtual_domain) { described_class.new(projects: [project_a, project_b, project_c], domain: domain) } it 'returns collection of projects pages lookup paths sorted by prefix in reverse' do expect(project_a).to receive(:pages_lookup_path).with(domain: domain, trim_prefix: nil).and_return(pages_lookup_path_a) @@ -47,7 +47,7 @@ RSpec.describe Pages::VirtualDomain do end context 'when there is trim_prefix provided' do - subject(:virtual_domain) { described_class.new([project_a, project_b], trim_prefix: 'group/') } + subject(:virtual_domain) { described_class.new(projects: [project_a, project_b], trim_prefix: 'group/') } it 'returns collection of projects pages lookup paths sorted by prefix in reverse' do expect(project_a).to receive(:pages_lookup_path).with(trim_prefix: 'group/', domain: nil).and_return(pages_lookup_path_a) @@ -57,4 +57,19 @@ RSpec.describe Pages::VirtualDomain do end end end + + describe '#cache_key' do + it 'returns the cache key based in the given cache_control' do + cache_control = instance_double(::Gitlab::Pages::CacheControl, cache_key: 'cache_key') + virtual_domain = described_class.new(projects: [instance_double(Project)], cache: cache_control) + + expect(virtual_domain.cache_key).to eq('cache_key') + end + + it 'returns nil when no cache_control is given' do + virtual_domain = described_class.new(projects: [instance_double(Project)]) + + expect(virtual_domain.cache_key).to be_nil + end + end end diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 7fde8d63947..4e463b1194c 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -544,16 +544,31 @@ RSpec.describe PagesDomain do end end - it 'returns the virual domain when there are pages deployed for the project' do - project.mark_pages_as_deployed - project.update_pages_deployment!(create(:pages_deployment, project: project)) + context 'when there are pages deployed for the project' do + let(:virtual_domain) { pages_domain.pages_virtual_domain } - expect(Pages::VirtualDomain).to receive(:new).with([project], domain: pages_domain).and_call_original + before do + project.mark_pages_as_deployed + project.update_pages_deployment!(create(:pages_deployment, project: project)) + end + + it 'returns the virual domain when there are pages deployed for the project' do + expect(virtual_domain).to be_an_instance_of(Pages::VirtualDomain) + expect(virtual_domain.lookup_paths).not_to be_empty + expect(virtual_domain.cache_key).to eq("pages_domain_for_project_#{project.id}") + end - virtual_domain = pages_domain.pages_virtual_domain + context 'when :cache_pages_domain_api is disabled' do + before do + stub_feature_flags(cache_pages_domain_api: false) + end - expect(virtual_domain).to be_an_instance_of(Pages::VirtualDomain) - expect(virtual_domain.lookup_paths).not_to be_empty + it 'returns the virual domain when there are pages deployed for the project' do + expect(virtual_domain).to be_an_instance_of(Pages::VirtualDomain) + expect(virtual_domain.lookup_paths).not_to be_empty + expect(virtual_domain.cache_key).to be_nil + end + end end end diff --git a/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb b/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb index 7d4268f74e9..7411bc95147 100644 --- a/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb +++ b/spec/models/preloaders/user_max_access_level_in_projects_preloader_spec.rb @@ -23,7 +23,8 @@ RSpec.describe Preloaders::UserMaxAccessLevelInProjectsPreloader do # we have an existing N+1, one for each project for which user is not a member # in this spec, project_3, project_4, project_5 # https://gitlab.com/gitlab-org/gitlab/-/issues/362890 - expect { query }.to make_queries(projects.size + 3) + ee_only_policy_check_queries = Gitlab.ee? ? 1 : 0 + expect { query }.to make_queries(projects.size + 3 + ee_only_policy_check_queries) end end diff --git a/spec/models/project_export_job_spec.rb b/spec/models/project_export_job_spec.rb index 5a2b1443f8b..653d4d2df27 100644 --- a/spec/models/project_export_job_spec.rb +++ b/spec/models/project_export_job_spec.rb @@ -3,17 +3,14 @@ require 'spec_helper' RSpec.describe ProjectExportJob, type: :model do - let(:project) { create(:project) } - let!(:job1) { create(:project_export_job, project: project, status: 0) } - let!(:job2) { create(:project_export_job, project: project, status: 2) } - describe 'associations' do - it { expect(job1).to belong_to(:project) } + it { is_expected.to belong_to(:project) } + it { is_expected.to have_many(:relation_exports) } end describe 'validations' do - it { expect(job1).to validate_presence_of(:project) } - it { expect(job1).to validate_presence_of(:jid) } - it { expect(job1).to validate_presence_of(:status) } + it { is_expected.to validate_presence_of(:project) } + it { is_expected.to validate_presence_of(:jid) } + it { is_expected.to validate_presence_of(:status) } end end diff --git a/spec/models/project_import_state_spec.rb b/spec/models/project_import_state_spec.rb index f6e398bd23c..db79185d759 100644 --- a/spec/models/project_import_state_spec.rb +++ b/spec/models/project_import_state_spec.rb @@ -156,7 +156,7 @@ RSpec.describe ProjectImportState, type: :model do project.import_state.finish end - it 'does not qneueue housekeeping when project does not have a valid import type' do + it 'does not enqueue housekeeping when project does not have a valid import type' do project = create(:project, :import_started, import_type: nil) expect(Projects::AfterImportWorker).not_to receive(:perform_async) @@ -164,6 +164,43 @@ RSpec.describe ProjectImportState, type: :model do project.import_state.finish end end + + context 'state transition: [:none, :scheduled, :started] => [:canceled]' do + it 'updates the import status' do + import_state = create(:import_state, :none) + expect { import_state.cancel } + .to change { import_state.status } + .from('none').to('canceled') + end + + it 'unsets the JID' do + import_state = create(:import_state, :started, jid: '123') + + expect(Gitlab::SidekiqStatus) + .to receive(:unset) + .with('123') + .and_call_original + + import_state.cancel! + + expect(import_state.jid).to be_nil + end + + it 'removes import data' do + import_data = ProjectImportData.new(data: { 'test' => 'some data' }) + project = create(:project, :import_scheduled, import_data: import_data) + + expect(project) + .to receive(:remove_import_data) + .and_call_original + + expect do + project.import_state.cancel + project.reload + end.to change { project.import_data } + .from(import_data).to(nil) + end + end end describe 'clearing `jid` after finish', :clean_gitlab_redis_cache do @@ -178,7 +215,7 @@ RSpec.describe ProjectImportState, type: :model do end end - context 'with an JID' do + context 'with a JID' do it 'unsets the JID' do import_state = create(:import_state, :started, jid: '123') diff --git a/spec/models/project_setting_spec.rb b/spec/models/project_setting_spec.rb index 867ad843406..fb1601a5f9c 100644 --- a/spec/models/project_setting_spec.rb +++ b/spec/models/project_setting_spec.rb @@ -6,6 +6,17 @@ RSpec.describe ProjectSetting, type: :model do using RSpec::Parameterized::TableSyntax it { is_expected.to belong_to(:project) } + describe 'scopes' do + let_it_be(:project_1) { create(:project) } + let_it_be(:project_2) { create(:project) } + let_it_be(:project_setting_1) { create(:project_setting, project: project_1) } + let_it_be(:project_setting_2) { create(:project_setting, project: project_2) } + + it 'returns project setting for the given projects' do + expect(described_class.for_projects(project_1)).to contain_exactly(project_setting_1) + end + end + describe 'validations' do it { is_expected.not_to allow_value(nil).for(:target_platforms) } it { is_expected.to allow_value([]).for(:target_platforms) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2d84c1b843e..2171ee752fd 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -27,6 +27,7 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to have_many(:merge_requests) } it { is_expected.to have_many(:merge_request_metrics).class_name('MergeRequest::Metrics') } it { is_expected.to have_many(:issues) } + it { is_expected.to have_many(:incident_management_issuable_escalation_statuses).through(:issues).inverse_of(:project).class_name('IncidentManagement::IssuableEscalationStatus') } it { is_expected.to have_many(:milestones) } it { is_expected.to have_many(:iterations) } it { is_expected.to have_many(:project_members).dependent(:delete_all) } @@ -81,7 +82,6 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to have_one(:last_event).class_name('Event') } it { is_expected.to have_one(:forked_from_project).through(:fork_network_member) } it { is_expected.to have_one(:auto_devops).class_name('ProjectAutoDevops') } - it { is_expected.to have_one(:tracing_setting).class_name('ProjectTracingSetting') } it { is_expected.to have_one(:error_tracking_setting).class_name('ErrorTracking::ProjectErrorTrackingSetting') } it { is_expected.to have_one(:project_setting) } it { is_expected.to have_one(:alerting_setting).class_name('Alerting::ProjectAlertingSetting') } @@ -821,31 +821,38 @@ RSpec.describe Project, factory_default: :keep do end describe 'delegation' do - [:add_guest, :add_reporter, :add_developer, :add_maintainer, :add_user, :add_users].each do |method| + [:add_guest, :add_reporter, :add_developer, :add_maintainer, :add_member, :add_members].each do |method| it { is_expected.to delegate_method(method).to(:team) } end it { is_expected.to delegate_method(:members).to(:team).with_prefix(true) } - it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).with_arguments(allow_nil: true) } - it { is_expected.to delegate_method(:root_ancestor).to(:namespace).with_arguments(allow_nil: true) } - it { is_expected.to delegate_method(:certificate_based_clusters_enabled?).to(:namespace).with_arguments(allow_nil: true) } - it { is_expected.to delegate_method(:last_pipeline).to(:commit).with_arguments(allow_nil: true) } + it { is_expected.to delegate_method(:name).to(:owner).with_prefix(true).allow_nil } + it { is_expected.to delegate_method(:root_ancestor).to(:namespace).allow_nil } + it { is_expected.to delegate_method(:certificate_based_clusters_enabled?).to(:namespace).allow_nil } + it { is_expected.to delegate_method(:last_pipeline).to(:commit).allow_nil } it { is_expected.to delegate_method(:container_registry_enabled?).to(:project_feature) } it { is_expected.to delegate_method(:container_registry_access_level).to(:project_feature) } - describe 'project settings' do + describe 'read project settings' do %i( show_default_award_emojis - show_default_award_emojis= show_default_award_emojis? warn_about_potentially_unwanted_characters - warn_about_potentially_unwanted_characters= warn_about_potentially_unwanted_characters? enforce_auth_checks_on_uploads - enforce_auth_checks_on_uploads= enforce_auth_checks_on_uploads? ).each do |method| - it { is_expected.to delegate_method(method).to(:project_setting).with_arguments(allow_nil: true) } + it { is_expected.to delegate_method(method).to(:project_setting).allow_nil } + end + end + + describe 'write project settings' do + %i( + show_default_award_emojis= + warn_about_potentially_unwanted_characters= + enforce_auth_checks_on_uploads= + ).each do |method| + it { is_expected.to delegate_method(method).to(:project_setting).with_arguments(:args).allow_nil } end end @@ -1855,7 +1862,7 @@ RSpec.describe Project, factory_default: :keep do describe 'when a user has access to a project' do before do - project.add_user(user, Gitlab::Access::MAINTAINER) + project.add_member(user, Gitlab::Access::MAINTAINER) end it { is_expected.to eq([project]) } @@ -3588,6 +3595,14 @@ RSpec.describe Project, factory_default: :keep do end end + describe '#emails_enabled?' do + let(:project) { build(:project, emails_disabled: false) } + + it "is the opposite of emails_disabled" do + expect(project.emails_enabled?).to be_truthy + end + end + describe '#lfs_enabled?' do let(:namespace) { create(:namespace) } let(:project) { build(:project, namespace: namespace) } @@ -8383,6 +8398,27 @@ RSpec.describe Project, factory_default: :keep do end end + describe '#group_group_links' do + context 'with group project' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + it 'returns group links of group' do + expect(group).to receive_message_chain(:shared_with_group_links, :of_ancestors_and_self) + + project.group_group_links + end + end + + context 'with personal project' do + let_it_be(:project) { create(:project) } + + it 'returns none' do + expect(project.group_group_links).to eq(GroupGroupLink.none) + end + end + end + describe '#security_training_available?' do subject { build(:project) } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 2ddbab7779e..1fab07c1452 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -251,13 +251,13 @@ RSpec.describe ProjectTeam do end end - describe '#add_users' do + describe '#add_members' do let(:user1) { create(:user) } let(:user2) { create(:user) } let(:project) { create(:project) } it 'add the given users to the team' do - project.team.add_users([user1, user2], :reporter) + project.team.add_members([user1, user2], :reporter) expect(project.team.reporter?(user1)).to be(true) expect(project.team.reporter?(user2)).to be(true) @@ -265,7 +265,7 @@ RSpec.describe ProjectTeam do context 'when `tasks_to_be_done` and `tasks_project_id` are passed' do before do - project.team.add_users([user1], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: project.id) + project.team.add_members([user1], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: project.id) end it 'creates a member_task with the correct attributes', :aggregate_failures do @@ -277,12 +277,12 @@ RSpec.describe ProjectTeam do end end - describe '#add_user' do + describe '#add_member' do let(:user) { create(:user) } let(:project) { create(:project) } it 'add the given user to the team' do - project.team.add_user(user, :reporter) + project.team.add_member(user, :reporter) expect(project.team.reporter?(user)).to be(true) end diff --git a/spec/models/project_tracing_setting_spec.rb b/spec/models/project_tracing_setting_spec.rb deleted file mode 100644 index a7e4e557b25..00000000000 --- a/spec/models/project_tracing_setting_spec.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ProjectTracingSetting do - describe '#external_url' do - let_it_be(:project) { create(:project) } - - let(:tracing_setting) { project.build_tracing_setting } - - describe 'Validations' do - describe 'external_url' do - it 'accepts a valid url' do - tracing_setting.external_url = 'https://gitlab.com' - - expect(tracing_setting).to be_valid - end - - it 'fails with an invalid url' do - tracing_setting.external_url = 'gitlab.com' - - expect(tracing_setting).to be_invalid - end - - it 'fails with a blank string' do - tracing_setting.external_url = nil - - expect(tracing_setting).to be_invalid - end - - it 'sanitizes the url' do - tracing_setting.external_url = %{https://replaceme.com/'><script>alert(document.cookie)</script>} - - expect(tracing_setting).to be_valid - expect(tracing_setting.external_url).to eq(%{https://replaceme.com/'>}) - end - end - end - end -end diff --git a/spec/models/projects/import_export/relation_export_spec.rb b/spec/models/projects/import_export/relation_export_spec.rb new file mode 100644 index 00000000000..c74ca82e161 --- /dev/null +++ b/spec/models/projects/import_export/relation_export_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ImportExport::RelationExport, type: :model do + subject { create(:project_relation_export) } + + describe 'associations' do + it { is_expected.to belong_to(:project_export_job) } + it { is_expected.to have_one(:upload) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:project_export_job) } + it { is_expected.to validate_presence_of(:relation) } + it { is_expected.to validate_uniqueness_of(:relation).scoped_to(:project_export_job_id) } + it { is_expected.to validate_presence_of(:status) } + it { is_expected.to validate_numericality_of(:status).only_integer } + it { is_expected.to validate_length_of(:relation).is_at_most(255) } + it { is_expected.to validate_length_of(:jid).is_at_most(255) } + it { is_expected.to validate_length_of(:export_error).is_at_most(300) } + end +end diff --git a/spec/models/projects/import_export/relation_export_upload_spec.rb b/spec/models/projects/import_export/relation_export_upload_spec.rb new file mode 100644 index 00000000000..c0014c5a14c --- /dev/null +++ b/spec/models/projects/import_export/relation_export_upload_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ImportExport::RelationExportUpload, type: :model do + subject { described_class.new(relation_export: project_relation_export) } + + let_it_be(:project_relation_export) { create(:project_relation_export) } + + describe 'associations' do + it { is_expected.to belong_to(:relation_export) } + end + + it 'stores export file' do + stub_uploads_object_storage(ImportExportUploader, enabled: false) + + filename = 'labels.tar.gz' + subject.export_file = fixture_file_upload("spec/fixtures/gitlab/import_export/#{filename}") + + subject.save! + + url = "/uploads/-/system/projects/import_export/relation_export_upload/export_file/#{subject.id}/#{filename}" + expect(subject.export_file.url).to eq(url) + end +end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 366de809bed..a3fc09b31fb 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -190,6 +190,14 @@ RSpec.describe ProtectedBranch do expect(described_class).not_to receive(:matching) expect(described_class.protected?(project, protected_branch.name)).to eq(true) end + + it 'sets expires_in for a cache key' do + cache_key = described_class.protected_ref_cache_key(project, protected_branch.name) + + expect(Rails.cache).to receive(:fetch).with(cache_key, expires_in: 1.hour) + + described_class.protected?(project, protected_branch.name) + end end end diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index d2d7859e726..51351c9fdd1 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -5,6 +5,10 @@ require 'spec_helper' RSpec.describe RemoteMirror, :mailer do include GitHelpers + before do + stub_feature_flags(remote_mirror_no_delay: false) + end + describe 'URL validation' do context 'with a valid URL' do it 'is valid' do @@ -343,6 +347,20 @@ RSpec.describe RemoteMirror, :mailer do remote_mirror.sync end + + context 'when remote_mirror_no_delay is enabled' do + before do + stub_feature_flags(remote_mirror_no_delay: true) + end + + it 'schedules a RepositoryUpdateRemoteMirrorWorker to run now' do + remote_mirror.last_update_started_at = Time.current - 30.seconds + + expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(remote_mirror.id, Time.current) + + remote_mirror.sync + end + end end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index e1d903a40cf..11323c40d28 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -125,11 +125,11 @@ RSpec.describe Repository do let(:latest_tag) { 'v0.0.0' } before do - rugged_repo(repository).tags.create(latest_tag, repository.commit.id) + repository.add_tag(user, latest_tag, repository.commit.id) end after do - rugged_repo(repository).tags.delete(latest_tag) + repository.rm_tag(user, latest_tag) end context 'desc' do @@ -150,16 +150,13 @@ RSpec.describe Repository do subject { repository.tags_sorted_by('updated_asc').map(&:name) & (tags_to_compare + [annotated_tag_name]) } before do - options = { message: 'test tag message\n', - tagger: { name: 'John Smith', email: 'john@gmail.com' } } - - rugged_repo(repository).tags.create(annotated_tag_name, 'a48e4fc218069f68ef2e769dd8dfea3991362175', **options) + repository.add_tag(user, annotated_tag_name, 'a48e4fc218069f68ef2e769dd8dfea3991362175', 'test tag message\n') end it { is_expected.to eq(['v1.0.0', 'v1.1.0', annotated_tag_name]) } after do - rugged_repo(repository).tags.delete(annotated_tag_name) + repository.rm_tag(user, annotated_tag_name) end end end @@ -258,21 +255,10 @@ RSpec.describe Repository do end context 'with a commit with invalid UTF-8 path' do - def create_commit_with_invalid_utf8_path - rugged = rugged_repo(repository) - blob_id = Rugged::Blob.from_buffer(rugged, "some contents") - tree_builder = Rugged::Tree::Builder.new(rugged) - tree_builder.insert({ oid: blob_id, name: "hello\x80world", filemode: 0100644 }) - tree_id = tree_builder.write - user = { email: "jcai@gitlab.com", time: Time.current.to_time, name: "John Cai" } - - Rugged::Commit.create(rugged, message: 'some commit message', parents: [rugged.head.target.oid], tree: tree_id, committer: user, author: user) - end - it 'does not raise an error' do - commit = create_commit_with_invalid_utf8_path + response = create_file_in_repo(project, 'master', 'master', "hello\x80world", 'some contents') - expect { repository.list_last_commits_for_tree(commit, '.', offset: 0) }.not_to raise_error + expect { repository.list_last_commits_for_tree(response[:result], '.', offset: 0) }.not_to raise_error end end end @@ -2262,20 +2248,12 @@ RSpec.describe Repository do describe '#branch_count' do it 'returns the number of branches' do expect(repository.branch_count).to be_an(Integer) - - rugged_count = rugged_repo(repository).branches.count - - expect(repository.branch_count).to eq(rugged_count) end end describe '#tag_count' do it 'returns the number of tags' do expect(repository.tag_count).to be_an(Integer) - - rugged_count = rugged_repo(repository).tags.count - - expect(repository.tag_count).to eq(rugged_count) end end @@ -2757,6 +2735,33 @@ RSpec.describe Repository do end end + describe '#changelog_config' do + let(:user) { create(:user) } + let(:changelog_config_path) { Gitlab::Changelog::Config::DEFAULT_FILE_PATH } + + before do + repository.create_file( + user, + changelog_config_path, + 'CONTENT', + message: '...', + branch_name: 'master' + ) + end + + context 'when there is a changelog_config_path at the commit' do + it 'returns the content' do + expect(repository.changelog_config(repository.commit.sha, changelog_config_path)).to eq('CONTENT') + end + end + + context 'when there is no changelog_config_path at the commit' do + it 'returns nil' do + expect(repository.changelog_config(repository.commit.parent.sha, changelog_config_path)).to be_nil + end + end + end + describe '#route_map_for' do before do repository.create_file(User.last, '.gitlab/route-map.yml', 'CONTENT', message: 'Add .gitlab/route-map.yml', branch_name: 'master') @@ -2776,8 +2781,7 @@ RSpec.describe Repository do end def create_remote_branch(remote_name, branch_name, target) - rugged = rugged_repo(repository) - rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id) + repository.write_ref("refs/remotes/#{remote_name}/#{branch_name}", target.id) end shared_examples '#ancestor?' do diff --git a/spec/models/ssh_host_key_spec.rb b/spec/models/ssh_host_key_spec.rb index 4b756846598..0348aab9f97 100644 --- a/spec/models/ssh_host_key_spec.rb +++ b/spec/models/ssh_host_key_spec.rb @@ -26,6 +26,9 @@ RSpec.describe SshHostKey do 'Ebi86VjJRi2sOuYoXQU1' end + let(:ssh_key1) { Gitlab::SSHPublicKey.new(key1) } + let(:ssh_key2) { Gitlab::SSHPublicKey.new(key2) } + # Purposefully ordered so that `sort` will make changes let(:known_hosts) do <<~EOF @@ -88,10 +91,17 @@ RSpec.describe SshHostKey do it 'returns an array of indexed fingerprints when the cache is filled' do stub_reactive_cache(ssh_host_key, known_hosts: known_hosts) - expected = [key1, key2] - .map { |data| Gitlab::SSHPublicKey.new(data) } + expected = [ssh_key1, ssh_key2] .each_with_index - .map { |key, i| { bits: key.bits, fingerprint: key.fingerprint, type: key.type, index: i } } + .map do |key, i| + { + bits: key.bits, + fingerprint: key.fingerprint, + fingerprint_sha256: key.fingerprint_sha256, + type: key.type, + index: i + } + end expect(ssh_host_key.fingerprints.as_json).to eq(expected) end @@ -107,8 +117,16 @@ RSpec.describe SshHostKey do expect(ssh_host_key.fingerprints.as_json).to eq( [ - { bits: 2048, fingerprint: Gitlab::SSHPublicKey.new(key1).fingerprint, type: :rsa, index: 0 }, - { bits: 2048, fingerprint: Gitlab::SSHPublicKey.new(key2).fingerprint, type: :rsa, index: 1 } + { bits: 2048, + fingerprint: ssh_key1.fingerprint, + fingerprint_sha256: ssh_key1.fingerprint_sha256, + type: :rsa, + index: 0 }, + { bits: 2048, + fingerprint: ssh_key2.fingerprint, + fingerprint_sha256: ssh_key2.fingerprint_sha256, + type: :rsa, + index: 1 } ] ) end @@ -116,6 +134,19 @@ RSpec.describe SshHostKey do it 'returns an empty array when the cache is empty' do expect(ssh_host_key.fingerprints).to eq([]) end + + context 'when FIPS is enabled', :fips_mode do + it 'only includes SHA256 fingerprint' do + stub_reactive_cache(ssh_host_key, known_hosts: known_hosts) + + expect(ssh_host_key.fingerprints.as_json).to eq( + [ + { bits: 2048, fingerprint_sha256: ssh_key1.fingerprint_sha256, type: :rsa, index: 0 }, + { bits: 2048, fingerprint_sha256: ssh_key2.fingerprint_sha256, type: :rsa, index: 1 } + ] + ) + end + end end describe '#host_keys_changed?' do diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb index 651e2cf273f..7df22078c6d 100644 --- a/spec/models/todo_spec.rb +++ b/spec/models/todo_spec.rb @@ -114,6 +114,26 @@ RSpec.describe Todo do end end + describe '#for_issue_or_work_item?' do + it 'returns true when target is an Issue' do + subject.target_type = 'Issue' + + expect(subject.for_issue_or_work_item?).to be_truthy + end + + it 'returns true when target is a WorkItem' do + subject.target_type = 'WorkItem' + + expect(subject.for_issue_or_work_item?).to be_truthy + end + + it 'returns false when target is not an Issue' do + subject.target_type = 'DesignManagement::Design' + + expect(subject.for_issue_or_work_item?).to be_falsey + end + end + describe '#target' do context 'for commits' do let(:project) { create(:project, :repository) } diff --git a/spec/models/tree_spec.rb b/spec/models/tree_spec.rb index b7a8276ec55..20d786f311f 100644 --- a/spec/models/tree_spec.rb +++ b/spec/models/tree_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe Tree do - let(:repository) { create(:project, :repository).repository } + let_it_be(:repository) { create(:project, :repository).repository } + let(:sha) { repository.root_ref } subject(:tree) { described_class.new(repository, '54fcc214') } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index abc02dd1f55..6d2ba66d5f4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -136,6 +136,7 @@ RSpec.describe User do it { is_expected.to have_many(:timelogs) } it { is_expected.to have_many(:callouts).class_name('Users::Callout') } it { is_expected.to have_many(:group_callouts).class_name('Users::GroupCallout') } + it { is_expected.to have_many(:namespace_callouts).class_name('Users::NamespaceCallout') } describe '#user_detail' do it 'does not persist `user_detail` by default' do @@ -1109,6 +1110,20 @@ RSpec.describe User do .to contain_exactly(user1, user2) end end + + describe '.order_recent_last_activity' do + it 'sorts users by activity and id to make the ordes deterministic' do + expect(described_class.order_recent_last_activity.to_sql).to include( + 'ORDER BY "users"."last_activity_on" DESC NULLS LAST, "users"."id" ASC') + end + end + + describe '.order_oldest_last_activity' do + it 'sorts users by activity and id to make the ordes deterministic' do + expect(described_class.order_oldest_last_activity.to_sql).to include( + 'ORDER BY "users"."last_activity_on" ASC NULLS FIRST, "users"."id" DESC') + end + end end context 'strip attributes' do @@ -2278,7 +2293,7 @@ RSpec.describe User do @group = create :group @group.add_owner(@user) - @group.add_user(@user2, GroupMember::OWNER) + @group.add_member(@user2, GroupMember::OWNER) end it { expect(@user2.several_namespaces?).to be_truthy } @@ -2729,131 +2744,149 @@ RSpec.describe User do end end - describe '.search' do - let_it_be(:user) { create(:user, name: 'user', username: 'usern', email: 'email@example.com') } - let_it_be(:public_email) do - create(:email, :confirmed, user: user, email: 'publicemail@example.com').tap do |email| - user.update!(public_email: email.email) + shared_examples '.search examples' do + describe '.search' do + let_it_be(:user) { create(:user, name: 'user', username: 'usern', email: 'email@example.com') } + let_it_be(:public_email) do + create(:email, :confirmed, user: user, email: 'publicemail@example.com').tap do |email| + user.update!(public_email: email.email) + end end - end - let_it_be(:user2) { create(:user, name: 'user name', username: 'username', email: 'someemail@example.com') } - let_it_be(:user3) { create(:user, name: 'us', username: 'se', email: 'foo@example.com') } - let_it_be(:email) { create(:email, user: user, email: 'alias@example.com') } + let_it_be(:user2) { create(:user, name: 'user name', username: 'username', email: 'someemail@example.com') } + let_it_be(:user3) { create(:user, name: 'us', username: 'se', email: 'foo@example.com') } + let_it_be(:email) { create(:email, user: user, email: 'alias@example.com') } - describe 'name user and email relative ordering' do - let_it_be(:named_alexander) { create(:user, name: 'Alexander Person', username: 'abcd', email: 'abcd@example.com') } - let_it_be(:username_alexand) { create(:user, name: 'Joao Alexander', username: 'Alexand', email: 'joao@example.com') } + describe 'name user and email relative ordering' do + let_it_be(:named_alexander) { create(:user, name: 'Alexander Person', username: 'abcd', email: 'abcd@example.com') } + let_it_be(:username_alexand) { create(:user, name: 'Joao Alexander', username: 'Alexand', email: 'joao@example.com') } - it 'prioritizes exact matches' do - expect(described_class.search('Alexand')).to eq([username_alexand, named_alexander]) - end + it 'prioritizes exact matches' do + expect(described_class.search('Alexand')).to eq([username_alexand, named_alexander]) + end - it 'falls back to ordering by name' do - expect(described_class.search('Alexander')).to eq([named_alexander, username_alexand]) + it 'falls back to ordering by name' do + expect(described_class.search('Alexander')).to eq([named_alexander, username_alexand]) + end end - end - describe 'name matching' do - it 'returns users with a matching name with exact match first' do - expect(described_class.search(user.name)).to eq([user, user2]) - end + describe 'name matching' do + it 'returns users with a matching name with exact match first' do + expect(described_class.search(user.name)).to eq([user, user2]) + end - it 'returns users with a partially matching name' do - expect(described_class.search(user.name[0..2])).to eq([user, user2]) - end + it 'returns users with a partially matching name' do + expect(described_class.search(user.name[0..2])).to eq([user, user2]) + end - it 'returns users with a matching name regardless of the casing' do - expect(described_class.search(user2.name.upcase)).to eq([user2]) - end + it 'returns users with a matching name regardless of the casing' do + expect(described_class.search(user2.name.upcase)).to eq([user2]) + end - it 'returns users with a exact matching name shorter than 3 chars' do - expect(described_class.search(user3.name)).to eq([user3]) - end + it 'returns users with a exact matching name shorter than 3 chars' do + expect(described_class.search(user3.name)).to eq([user3]) + end - 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 + 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]) + 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 - end - describe 'email matching' do - it 'returns users with a matching public email' do - expect(described_class.search(user.public_email)).to match_array([user]) - end + describe 'email matching' do + it 'returns users with a matching public email' do + expect(described_class.search(user.public_email)).to match_array([user]) + end - it 'does not return users with a partially matching public email' do - expect(described_class.search(user.public_email[1...-1])).to be_empty - end + it 'does not return users with a partially matching public email' do + expect(described_class.search(user.public_email[1...-1])).to be_empty + end - it 'returns users with a matching public email regardless of the casing' do - expect(described_class.search(user.public_email.upcase)).to match_array([user]) - end + it 'returns users with a matching public email regardless of the casing' do + expect(described_class.search(user.public_email.upcase)).to match_array([user]) + end + + it 'does not return users with a matching private email' do + expect(described_class.search(user.email)).to be_empty + expect(described_class.search(email.email)).to be_empty + end + + context 'with private emails search' do + it 'returns users with matching private email' do + expect(described_class.search(user.email, with_private_emails: true)).to match_array([user]) + end - it 'does not return users with a matching private email' do - expect(described_class.search(user.email)).to be_empty - expect(described_class.search(email.email)).to be_empty + it 'returns users with matching private secondary email' do + expect(described_class.search(email.email, with_private_emails: true)).to match_array([user]) + end + end end - context 'with private emails search' do - it 'returns users with matching private email' do - expect(described_class.search(user.email, with_private_emails: true)).to match_array([user]) + describe 'username matching' do + it 'returns users with a matching username' do + expect(described_class.search(user.username)).to eq([user, user2]) end - it 'returns users with matching private secondary email' do - expect(described_class.search(email.email, with_private_emails: true)).to match_array([user]) + it 'returns users with a matching username starting with a @' do + expect(described_class.search("@#{user.username}")).to eq([user, user2]) end - end - end - describe 'username matching' do - it 'returns users with a matching username' do - expect(described_class.search(user.username)).to eq([user, user2]) - end + it 'returns users with a partially matching username' do + expect(described_class.search(user.username[0..2])).to eq([user, user2]) + end - it 'returns users with a matching username starting with a @' do - expect(described_class.search("@#{user.username}")).to eq([user, user2]) - end + it 'returns users with a partially matching username starting with @' do + expect(described_class.search("@#{user.username[0..2]}")).to eq([user, user2]) + end - it 'returns users with a partially matching username' do - expect(described_class.search(user.username[0..2])).to eq([user, user2]) - end + it 'returns users with a matching username regardless of the casing' do + expect(described_class.search(user2.username.upcase)).to eq([user2]) + end - it 'returns users with a partially matching username starting with @' do - expect(described_class.search("@#{user.username[0..2]}")).to eq([user, user2]) - end + it 'returns users with a exact matching username shorter than 3 chars' do + expect(described_class.search(user3.username)).to eq([user3]) + end - it 'returns users with a matching username regardless of the casing' do - expect(described_class.search(user2.username.upcase)).to eq([user2]) - end + 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 - it 'returns users with a exact matching username shorter than 3 chars' do - expect(described_class.search(user3.username)).to eq([user3]) + 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 - 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]) + it 'returns no matches for an empty string' do + expect(described_class.search('')).to be_empty end - 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 + it 'returns no matches for nil' do + expect(described_class.search(nil)).to be_empty end end + end - it 'returns no matches for an empty string' do - expect(described_class.search('')).to be_empty + context 'when the use_keyset_aware_user_search_query FF is on' do + before do + stub_feature_flags(use_keyset_aware_user_search_query: true) end - it 'returns no matches for nil' do - expect(described_class.search(nil)).to be_empty + it_behaves_like '.search examples' + end + + context 'when the use_keyset_aware_user_search_query FF is off' do + before do + stub_feature_flags(use_keyset_aware_user_search_query: false) end + + it_behaves_like '.search examples' end describe '.user_search_minimum_char_limit' do @@ -3001,7 +3034,7 @@ RSpec.describe User do it 'has all ssh keys' do user = create :user - key = create :key, key: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQD33bWLBxu48Sev9Fert1yzEO4WGcWglWF7K/AwblIUFselOt/QdOL9DSjpQGxLagO1s9wl53STIO8qGS4Ms0EJZyIXOEFMjFJ5xmjSy+S37By4sG7SsltQEHMxtbtFOaW5LV2wCrX+rUsRNqLMamZjgjcPO0/EgGCXIGMAYW4O7cwGZdXWYIhQ1Vwy+CsVMDdPkPgBXqK7nR/ey8KMs8ho5fMNgB5hBw/AL9fNGhRw3QTD6Q12Nkhl4VZES2EsZqlpNnJttnPdp847DUsT6yuLRlfiQfz5Cn9ysHFdXObMN5VYIiPFwHeYCZp1X2S4fDZooRE8uOLTfxWHPXwrhqSH", user_id: user.id + key = create :key_without_comment, user_id: user.id expect(user.all_ssh_keys).to include(a_string_starting_with(key.key)) end @@ -3428,6 +3461,15 @@ RSpec.describe User do end end + describe '#followed_by?' do + it 'check if followed by another user' do + follower = create :user + followee = create :user + + expect { follower.follow(followee) }.to change { followee.followed_by?(follower) }.from(false).to(true) + end + end + describe '#follow' do it 'follow another user' do user = create :user @@ -3518,49 +3560,45 @@ RSpec.describe User do end describe '#sort_by_attribute' do - before do - described_class.delete_all - @user = create :user, created_at: Date.today, current_sign_in_at: Date.today, name: 'Alpha' - @user1 = create :user, created_at: Date.today - 1, current_sign_in_at: Date.today - 1, name: 'Omega' - @user2 = create :user, created_at: Date.today - 2, name: 'Beta' - end + let_it_be(:user) { create :user, created_at: Date.today, current_sign_in_at: Date.today, username: 'user0' } + let_it_be(:user1) { create :user, created_at: Date.today - 1, last_activity_on: Date.today - 1, current_sign_in_at: Date.today - 1, username: 'user1' } + let_it_be(:user2) { create :user, created_at: Date.today - 2, username: 'user2' } + let_it_be(:user3) { create :user, created_at: Date.today - 3, last_activity_on: Date.today, username: "user3" } context 'when sort by recent_sign_in' do let(:users) { described_class.sort_by_attribute('recent_sign_in') } - it 'sorts users by recent sign-in time' do - expect(users.first).to eq(@user) - expect(users.second).to eq(@user1) - end - - it 'pushes users who never signed in to the end' do - expect(users.third).to eq(@user2) + it 'sorts users by recent sign-in time with user that never signed in at the end' do + expect(users).to eq([user, user1, user2, user3]) end end context 'when sort by oldest_sign_in' do let(:users) { described_class.sort_by_attribute('oldest_sign_in') } - it 'sorts users by the oldest sign-in time' do - expect(users.first).to eq(@user1) - expect(users.second).to eq(@user) - end - - it 'pushes users who never signed in to the end' do - expect(users.third).to eq(@user2) + it 'sorts users by the oldest sign-in time with users that never signed in at the end' do + expect(users).to eq([user1, user, user2, user3]) end end it 'sorts users in descending order by their creation time' do - expect(described_class.sort_by_attribute('created_desc').first).to eq(@user) + expect(described_class.sort_by_attribute('created_desc')).to eq([user, user1, user2, user3]) end it 'sorts users in ascending order by their creation time' do - expect(described_class.sort_by_attribute('created_asc').first).to eq(@user2) + expect(described_class.sort_by_attribute('created_asc')).to eq([user3, user2, user1, user]) end it 'sorts users by id in descending order when nil is passed' do - expect(described_class.sort_by_attribute(nil).first).to eq(@user2) + expect(described_class.sort_by_attribute(nil)).to eq([user3, user2, user1, user]) + end + + it 'sorts user by latest activity descending, nulls last ordered by ascending id' do + expect(described_class.sort_by_attribute('last_activity_on_desc')).to eq([user3, user1, user, user2]) + end + + it 'sorts user by latest activity ascending, nulls first ordered by descending id' do + expect(described_class.sort_by_attribute('last_activity_on_asc')).to eq([user2, user, user1, user3]) end end @@ -3824,7 +3862,7 @@ RSpec.describe User do let!(:project) { create(:project, group: project_group) } before do - private_group.add_user(user, Gitlab::Access::MAINTAINER) + private_group.add_member(user, Gitlab::Access::MAINTAINER) project.add_maintainer(user) end @@ -3851,7 +3889,7 @@ RSpec.describe User do let_it_be(:parent_group) do create(:group).tap do |g| - g.add_user(user, Gitlab::Access::MAINTAINER) + g.add_member(user, Gitlab::Access::MAINTAINER) end end @@ -4279,7 +4317,7 @@ RSpec.describe User do let!(:runner) { create(:ci_runner, :group, groups: [group]) } def add_user(access) - group.add_user(user, access) + group.add_member(user, access) end it_behaves_like :group_member @@ -4369,7 +4407,7 @@ RSpec.describe User do let!(:project_runner) { create(:ci_runner, :project, projects: [project]) } def add_user(access) - project.add_user(user, access) + project.add_member(user, access) end it_behaves_like :project_member @@ -4391,8 +4429,8 @@ RSpec.describe User do let!(:another_user) { create(:user) } def add_user(access) - subgroup.add_user(user, access) - group.add_user(another_user, :owner) + subgroup.add_member(user, access) + group.add_member(another_user, :owner) end it_behaves_like :group_member @@ -4749,8 +4787,8 @@ RSpec.describe User do let(:group2) { create :group, require_two_factor_authentication: true, two_factor_grace_period: 32 } before do - group1.add_user(user, GroupMember::OWNER) - group2.add_user(user, GroupMember::OWNER) + group1.add_member(user, GroupMember::OWNER) + group2.add_member(user, GroupMember::OWNER) user.update_two_factor_requirement end @@ -4769,7 +4807,7 @@ RSpec.describe User do let!(:group1a) { create :group, parent: group1 } before do - group1a.add_user(user, GroupMember::OWNER) + group1a.add_member(user, GroupMember::OWNER) user.update_two_factor_requirement end @@ -4784,7 +4822,7 @@ RSpec.describe User do let!(:group1a) { create :group, require_two_factor_authentication: true, parent: group1 } before do - group1.add_user(user, GroupMember::OWNER) + group1.add_member(user, GroupMember::OWNER) user.update_two_factor_requirement end @@ -4805,7 +4843,7 @@ RSpec.describe User do group_access: ProjectGroupLink.default_access ) - group2.add_user(user, GroupMember::OWNER) + group2.add_member(user, GroupMember::OWNER) end it 'does not require 2FA' do @@ -4819,7 +4857,7 @@ RSpec.describe User do let(:group) { create :group } before do - group.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) user.update_two_factor_requirement end @@ -4848,8 +4886,8 @@ RSpec.describe User do let(:user) { create :user } before do - group.add_user(user, GroupMember::OWNER) - group_not_requiring_2FA.add_user(user, GroupMember::OWNER) + group.add_member(user, GroupMember::OWNER) + group_not_requiring_2FA.add_member(user, GroupMember::OWNER) end context 'when user is direct member of group requiring 2FA' do @@ -5884,8 +5922,44 @@ RSpec.describe User do end end + describe '#authenticatable_salt' do + let(:user) { create(:user) } + + subject(:authenticatable_salt) { user.authenticatable_salt } + + it 'uses password_salt' do + expect(authenticatable_salt).to eq(user.password_salt) + end + + context 'when the encrypted_password is an unknown type' do + let(:encrypted_password) { '$argon2i$v=19$m=512,t=4,p=2$eM+ZMyYkpDRGaI3xXmuNcQ$c5DeJg3eb5dskVt1mDdxfw' } + + before do + user.update_attribute(:encrypted_password, encrypted_password) + end + + it 'returns the first 30 characters of the encrypted_password' do + expect(authenticatable_salt).to eq(encrypted_password[0, 29]) + end + end + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it 'returns the first 30 characters of the encrypted_password' do + expect(authenticatable_salt).to eq(user.encrypted_password[0, 29]) + end + end + end + + def compare_pbkdf2_password(user, password) + Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.compare(user.encrypted_password, password) + end + describe '#valid_password?' do - subject { user.valid_password?(password) } + subject(:validate_password) { user.valid_password?(password) } context 'user with password not in disallowed list' do let(:user) { create(:user) } @@ -5898,6 +5972,15 @@ RSpec.describe User do it { is_expected.to be_falsey } end + + context 'when pbkdf2_sha512_encryption is disabled and the user password is pbkdf2+sha512' do + it 'does not validate correctly' do + user # Create the user while the feature is enabled + stub_feature_flags(pbkdf2_password_encryption: false) + + expect(validate_password).to be_falsey + end + end end context 'user with disallowed password' do @@ -5912,6 +5995,174 @@ RSpec.describe User do it { is_expected.to be_falsey } end end + + context 'user with a bcrypt password hash' do + # Plaintext password 'eiFubohV6iro' + let(:encrypted_password) { '$2a$10$xLTxCKOa75IU4RQGqqOrTuZOgZdJEzfSzjG6ZSEi/C31TB/yLZYpi' } + let(:user) { create(:user, encrypted_password: encrypted_password) } + + shared_examples 'not re-encrypting with PBKDF2' do + it 'does not re-encrypt with PBKDF2' do + validate_password + + expect(user.reload.encrypted_password).to eq(encrypted_password) + end + end + + context 'using the wrong password' do + let(:password) { 'WRONG PASSWORD' } + + it { is_expected.to be_falsey } + it_behaves_like 'not re-encrypting with PBKDF2' + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it { is_expected.to be_falsey } + it_behaves_like 'not re-encrypting with PBKDF2' + end + end + + context 'using the correct password' do + let(:password) { 'eiFubohV6iro' } + + it { is_expected.to be_truthy } + + it 'validates the password and re-encrypts with PBKDF2' do + validate_password + + current_encrypted_password = user.reload.encrypted_password + + expect(compare_pbkdf2_password(user, password)).to eq(true) + expect { ::BCrypt::Password.new(current_encrypted_password) } + .to raise_error(::BCrypt::Errors::InvalidHash) + end + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it { is_expected.to be_truthy } + it_behaves_like 'not re-encrypting with PBKDF2' + end + + context 'when pbkdf2_password_encryption_write is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption_write: false) + end + + it { is_expected.to be_truthy } + it_behaves_like 'not re-encrypting with PBKDF2' + end + end + end + + context 'user with password hash that is neither PBKDF2 nor BCrypt' do + let(:user) { create(:user, encrypted_password: '$argon2i$v=19$m=512,t=4,p=2$eM+ZMyYkpDRGaI3xXmuNcQ$c5DeJg3eb5dskVt1mDdxfw') } + let(:password) { 'password' } + + it { is_expected.to be_falsey } + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it { is_expected.to be_falsey } + end + end + end + + # These entire test section can be removed once the :pbkdf2_password_encryption feature flag is removed. + describe '#password=' do + let(:user) { create(:user) } + let(:password) { 'Oot5iechahqu' } + + def compare_bcrypt_password(user, password) + Devise::Encryptor.compare(User, user.encrypted_password, password) + end + + context 'when pbkdf2_password_encryption is enabled' do + it 'calls PBKDF2 digest and not the default Devise encryptor' do + expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).to receive(:digest).at_least(:once).and_call_original + expect(Devise::Encryptor).not_to receive(:digest) + + user.password = password + end + + it 'saves the password in PBKDF2 format' do + user.password = password + user.save! + + expect(compare_pbkdf2_password(user, password)).to eq(true) + expect { compare_bcrypt_password(user, password) }.to raise_error(::BCrypt::Errors::InvalidHash) + end + + context 'when pbkdf2_password_encryption_write is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption_write: false) + end + + it 'calls default Devise encryptor and not the PBKDF2 encryptor' do + expect(Devise::Encryptor).to receive(:digest).at_least(:once).and_call_original + expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).not_to receive(:digest) + + user.password = password + end + end + end + + context 'when pbkdf2_password_encryption is disabled' do + before do + stub_feature_flags(pbkdf2_password_encryption: false) + end + + it 'calls default Devise encryptor and not the PBKDF2 encryptor' do + expect(Devise::Encryptor).to receive(:digest).at_least(:once).and_call_original + expect(Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512).not_to receive(:digest) + + user.password = password + end + + it 'saves the password in BCrypt format' do + user.password = password + user.save! + + expect { compare_pbkdf2_password(user, password) }.to raise_error Devise::Pbkdf2Encryptable::Encryptors::InvalidHash + expect(compare_bcrypt_password(user, password)).to eq(true) + end + end + end + + describe '#password_strategy' do + let(:user) { create(:user, encrypted_password: encrypted_password) } + + context 'with a PBKDF2+SHA512 encrypted password' do + let(:encrypted_password) { '$pbkdf2-sha512$20000$boHGAw0hEyI$DBA67J7zNZebyzLtLk2X9wRDbmj1LNKVGnZLYyz6PGrIDGIl45fl/BPH0y1TPZnV90A20i.fD9C3G9Bp8jzzOA' } + + it 'extracts the correct strategy', :aggregate_failures do + expect(user.password_strategy).to eq(:pbkdf2_sha512) + end + end + + context 'with a BCrypt encrypted password' do + let(:encrypted_password) { '$2a$10$xLTxCKOa75IU4RQGqqOrTuZOgZdJEzfSzjG6ZSEi/C31TB/yLZYpi' } + + it 'extracts the correct strategy', :aggregate_failures do + expect(user.password_strategy).to eq(:bcrypt) + end + end + + context 'with an unknown encrypted password' do + let(:encrypted_password) { '$pbkdf2-sha256$6400$.6UI/S.nXIk8jcbdHx3Fhg$98jZicV16ODfEsEZeYPGHU3kbrUrvUEXOPimVSQDD44' } + + it 'returns unknown strategy' do + expect(user.password_strategy).to eq(:unknown) + end + end end describe '#password_expired?' do @@ -6165,6 +6416,96 @@ RSpec.describe User do end end + describe 'Users::NamespaceCallout' do + describe '#dismissed_callout_for_namespace?' do + let_it_be(:user, refind: true) { create(:user) } + let_it_be(:namespace) { create(:namespace) } + let_it_be(:feature_name) { Users::NamespaceCallout.feature_names.each_key.first } + + let(:query) do + { feature_name: feature_name, namespace: namespace } + end + + def have_dismissed_callout + be_dismissed_callout_for_namespace(**query) + end + + context 'when no callout dismissal record exists' do + it 'returns false when no ignore_dismissal_earlier_than provided' do + expect(user).not_to have_dismissed_callout + end + end + + context 'when dismissed callout exists' do + before_all do + create(:namespace_callout, + user: user, + namespace_id: namespace.id, + feature_name: feature_name, + dismissed_at: 4.months.ago) + end + + it 'returns true when no ignore_dismissal_earlier_than provided' do + expect(user).to have_dismissed_callout + end + + it 'returns true when ignore_dismissal_earlier_than is earlier than dismissed_at' do + query[:ignore_dismissal_earlier_than] = 6.months.ago + + expect(user).to have_dismissed_callout + end + + it 'returns false when ignore_dismissal_earlier_than is later than dismissed_at' do + query[:ignore_dismissal_earlier_than] = 2.months.ago + + expect(user).not_to have_dismissed_callout + end + end + end + + describe '#find_or_initialize_namespace_callout' do + let_it_be(:user, refind: true) { create(:user) } + let_it_be(:namespace) { create(:namespace) } + let_it_be(:feature_name) { Users::NamespaceCallout.feature_names.each_key.first } + + subject(:callout_with_source) do + user.find_or_initialize_namespace_callout(feature_name, namespace.id) + end + + context 'when callout exists' do + let!(:callout) do + create(:namespace_callout, user: user, feature_name: feature_name, namespace_id: namespace.id) + end + + it 'returns existing callout' do + expect(callout_with_source).to eq(callout) + end + end + + context 'when callout does not exist' do + context 'when feature name is valid' do + it 'initializes a new callout' do + expect(callout_with_source) + .to be_a_new(Users::NamespaceCallout) + .and be_valid + end + end + + context 'when feature name is not valid' do + let(:feature_name) { 'notvalid' } + + it 'initializes a new callout' do + expect(callout_with_source).to be_a_new(Users::NamespaceCallout) + end + + it 'is not valid' do + expect(callout_with_source).not_to be_valid + end + end + end + end + end + describe '#dismissed_callout_for_group?' do let_it_be(:user, refind: true) { create(:user) } let_it_be(:group) { create(:group) } diff --git a/spec/models/users/namespace_callout_spec.rb b/spec/models/users/namespace_callout_spec.rb new file mode 100644 index 00000000000..f8207f2abc8 --- /dev/null +++ b/spec/models/users/namespace_callout_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::NamespaceCallout do + let_it_be(:user) { create_default(:user) } + let_it_be(:namespace) { create_default(:namespace) } + let_it_be(:callout) { create(:namespace_callout) } + + it_behaves_like 'having unique enum values' + + describe 'relationships' do + it { is_expected.to belong_to(:namespace) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:namespace) } + it { is_expected.to validate_presence_of(:user) } + it { is_expected.to validate_presence_of(:feature_name) } + + specify do + is_expected.to validate_uniqueness_of(:feature_name) + .scoped_to(:user_id, :namespace_id) + .ignoring_case_sensitivity + end + + it { is_expected.to allow_value(:web_hook_disabled).for(:feature_name) } + + it 'rejects invalid feature names' do + expect { callout.feature_name = :non_existent_feature }.to raise_error(ArgumentError) + end + end + + describe '#source_feature_name' do + it 'provides string based off source and feature' do + expect(callout.source_feature_name).to eq "#{callout.feature_name}_#{callout.namespace_id}" + end + end +end diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 51970064c54..96c396f085c 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -24,6 +24,14 @@ RSpec.describe WikiPage do container.wiki end + def disable_front_matter + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => false) + end + + def enable_front_matter_for(thing) + stub_feature_flags(Gitlab::WikiPages::FrontMatterParser::FEATURE_FLAG => thing) + end + # Use for groups of tests that do not modify their `subject`. # # include_context 'subject is persisted page', title: 'my title' @@ -40,6 +48,12 @@ RSpec.describe WikiPage do it { expect(wiki_page).to have_attributes(front_matter: {}, content: content) } end + shared_examples 'a page with front-matter' do + let(:front_matter) { { title: 'Foo', slugs: %w[slug_a slug_b] } } + + it { expect(wiki_page.front_matter).to eq(front_matter) } + end + context 'the wiki page has front matter' do let(:content) do <<~MD @@ -54,13 +68,27 @@ RSpec.describe WikiPage do MD end - it 'has front-matter' do - expect(wiki_page.front_matter).to eq({ title: 'Foo', slugs: %w[slug_a slug_b] }) - end + it_behaves_like 'a page with front-matter' it 'strips the front matter from the content' do expect(wiki_page.content.strip).to eq('My actual content') end + + context 'the feature flag is off' do + before do + disable_front_matter + end + + it_behaves_like 'a page without front-matter' + + context 'but enabled for the container' do + before do + enable_front_matter_for(container) + end + + it_behaves_like 'a page with front-matter' + end + end end context 'the wiki page does not have front matter' do @@ -443,6 +471,29 @@ RSpec.describe WikiPage do end end + context 'the front-matter feature flag is not enabled' do + before do + disable_front_matter + end + + it 'does not update the front-matter' do + content = subject.content + subject.update(front_matter: { slugs: ['x'] }) + + page = wiki.find_page(subject.title) + + expect([subject, page]).to all(have_attributes(front_matter: be_empty, content: content)) + end + + context 'but it is enabled for the container' do + before do + enable_front_matter_for(container) + end + + it_behaves_like 'able to update front-matter' + end + end + it 'updates the wiki-page front-matter and content together' do content = 'totally new content' subject.update(content: content, front_matter: { slugs: ['x'] }) diff --git a/spec/models/work_item_spec.rb b/spec/models/work_item_spec.rb index 5e757c11f99..f33c8e0a186 100644 --- a/spec/models/work_item_spec.rb +++ b/spec/models/work_item_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe WorkItem do + let_it_be(:reusable_project) { create(:project) } + describe 'associations' do + it { is_expected.to belong_to(:namespace) } it { is_expected.to have_one(:work_item_parent).class_name('WorkItem') } it 'has one `parent_link`' do @@ -38,7 +41,9 @@ RSpec.describe WorkItem do it 'returns instances of supported widgets' do is_expected.to match_array([instance_of(WorkItems::Widgets::Description), - instance_of(WorkItems::Widgets::Hierarchy)]) + instance_of(WorkItems::Widgets::Hierarchy), + instance_of(WorkItems::Widgets::Assignees), + instance_of(WorkItems::Widgets::Weight)]) end end @@ -52,5 +57,55 @@ RSpec.describe WorkItem do create(:work_item) end end + + context 'work item namespace' do + let(:work_item) { build(:work_item, project: reusable_project) } + + it 'sets the namespace_id' do + expect(work_item).to be_valid + expect(work_item.namespace).to eq(reusable_project.project_namespace) + end + + context 'when work item is saved' do + it 'sets the namespace_id' do + work_item.save! + expect(work_item.reload.namespace).to eq(reusable_project.project_namespace) + end + end + + context 'when existing work item is saved' do + let(:work_item) { create(:work_item) } + + before do + work_item.update!(namespace_id: nil) + end + + it 'sets the namespace id' do + work_item.update!(title: "#{work_item.title} and something extra") + + expect(work_item.namespace).to eq(work_item.project.project_namespace) + end + end + end + end + + describe 'validations' do + subject { work_item.valid? } + + describe 'issue_type' do + let(:work_item) { build(:work_item, issue_type: issue_type) } + + context 'when a valid type' do + let(:issue_type) { :issue } + + it { is_expected.to eq(true) } + end + + context 'empty type' do + let(:issue_type) { nil } + + it { is_expected.to eq(false) } + end + end end end diff --git a/spec/models/work_items/parent_link_spec.rb b/spec/models/work_items/parent_link_spec.rb index 9516baa7340..a16b15bbfc9 100644 --- a/spec/models/work_items/parent_link_spec.rb +++ b/spec/models/work_items/parent_link_spec.rb @@ -9,38 +9,46 @@ RSpec.describe WorkItems::ParentLink do end describe 'validations' do + subject { build(:parent_link) } + it { is_expected.to validate_presence_of(:work_item) } it { is_expected.to validate_presence_of(:work_item_parent) } + it { is_expected.to validate_uniqueness_of(:work_item) } describe 'hierarchy' do let_it_be(:project) { create(:project) } let_it_be(:issue) { build(:work_item, project: project) } + let_it_be(:incident) { build(:work_item, :incident, project: project) } let_it_be(:task1) { build(:work_item, :task, project: project) } let_it_be(:task2) { build(:work_item, :task, project: project) } - it 'is valid if not-task parent has task child' do + it 'is valid if issue parent has task child' do expect(build(:parent_link, work_item: task1, work_item_parent: issue)).to be_valid end + it 'is valid if incident parent has task child' do + expect(build(:parent_link, work_item: task1, work_item_parent: incident)).to be_valid + end + it 'is not valid if child is not task' do link = build(:parent_link, work_item: issue) expect(link).not_to be_valid - expect(link.errors[:work_item]).to include('Only Task can be assigned as a child in hierarchy.') + expect(link.errors[:work_item]).to include('only Task can be assigned as a child in hierarchy.') end it 'is not valid if parent is task' do link = build(:parent_link, work_item_parent: task1) expect(link).not_to be_valid - expect(link.errors[:work_item_parent]).to include('Only Issue can be parent of Task.') + expect(link.errors[:work_item_parent]).to include('only Issue and Incident can be parent of Task.') end it 'is not valid if parent is in other project' do link = build(:parent_link, work_item_parent: task1, work_item: build(:work_item)) expect(link).not_to be_valid - expect(link.errors[:work_item_parent]).to include('Parent must be in the same project as child.') + expect(link.errors[:work_item_parent]).to include('parent must be in the same project as child.') end context 'when parent already has maximum number of links' do @@ -54,7 +62,7 @@ RSpec.describe WorkItems::ParentLink do link2 = build(:parent_link, work_item_parent: issue, work_item: task2) expect(link2).not_to be_valid - expect(link2.errors[:work_item_parent]).to include('Parent already has maximum number of children.') + expect(link2.errors[:work_item_parent]).to include('parent already has maximum number of children.') end it 'existing link is still valid' do diff --git a/spec/models/work_items/type_spec.rb b/spec/models/work_items/type_spec.rb index 81663d0eb41..e91617effc0 100644 --- a/spec/models/work_items/type_spec.rb +++ b/spec/models/work_items/type_spec.rb @@ -65,7 +65,9 @@ RSpec.describe WorkItems::Type do it 'returns list of all possible widgets' do is_expected.to match_array([::WorkItems::Widgets::Description, - ::WorkItems::Widgets::Hierarchy]) + ::WorkItems::Widgets::Hierarchy, + ::WorkItems::Widgets::Assignees, + ::WorkItems::Widgets::Weight]) end end diff --git a/spec/models/work_items/widgets/assignees_spec.rb b/spec/models/work_items/widgets/assignees_spec.rb new file mode 100644 index 00000000000..a2c93c07fde --- /dev/null +++ b/spec/models/work_items/widgets/assignees_spec.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WorkItems::Widgets::Assignees do + let_it_be(:work_item) { create(:work_item, assignees: [create(:user)]) } + + describe '.type' do + subject { described_class.type } + + it { is_expected.to eq(:assignees) } + end + + describe '#type' do + subject { described_class.new(work_item).type } + + it { is_expected.to eq(:assignees) } + end + + describe '#assignees' do + subject { described_class.new(work_item).assignees } + + it { is_expected.to eq(work_item.assignees) } + end + + describe '#allows_multiple_assignees?' do + subject { described_class.new(work_item).allows_multiple_assignees? } + + it { is_expected.to eq(work_item.allows_multiple_assignees?) } + end +end diff --git a/spec/models/work_items/widgets/hierarchy_spec.rb b/spec/models/work_items/widgets/hierarchy_spec.rb index 0141731529b..ab2bcfee13f 100644 --- a/spec/models/work_items/widgets/hierarchy_spec.rb +++ b/spec/models/work_items/widgets/hierarchy_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe WorkItems::Widgets::Hierarchy do - let_it_be(:work_item) { create(:work_item) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:task) { create(:work_item, :task, project: project) } + let_it_be(:work_item_parent) { create(:work_item, project: project) } describe '.type' do subject { described_class.type } @@ -12,41 +15,57 @@ RSpec.describe WorkItems::Widgets::Hierarchy do end describe '#type' do - subject { described_class.new(work_item).type } + subject { described_class.new(task).type } it { is_expected.to eq(:hierarchy) } end describe '#parent' do - let_it_be(:parent_link) { create(:parent_link) } + let_it_be(:parent_link) { create(:parent_link, work_item: task, work_item_parent: work_item_parent) } subject { described_class.new(parent_link.work_item).parent } - it { is_expected.to eq parent_link.work_item_parent } + it { is_expected.to eq(parent_link.work_item_parent) } - context 'when work_items_hierarchy flag is disabled' do + context 'when work_items flag is disabled' do before do - stub_feature_flags(work_items_hierarchy: false) + stub_feature_flags(work_items: false) end it { is_expected.to be_nil } end + + context 'when work_items flag is enabled for the parent group' do + before do + stub_feature_flags(work_items: group) + end + + it { is_expected.to eq(parent_link.work_item_parent) } + end end describe '#children' do - let_it_be(:parent_link1) { create(:parent_link, work_item_parent: work_item) } - let_it_be(:parent_link2) { create(:parent_link, work_item_parent: work_item) } + let_it_be(:parent_link1) { create(:parent_link, work_item_parent: work_item_parent, work_item: task) } + let_it_be(:parent_link2) { create(:parent_link, work_item_parent: work_item_parent) } - subject { described_class.new(work_item).children } + subject { described_class.new(work_item_parent).children } - it { is_expected.to match_array([parent_link1.work_item, parent_link2.work_item]) } + it { is_expected.to contain_exactly(parent_link1.work_item, parent_link2.work_item) } - context 'when work_items_hierarchy flag is disabled' do + context 'when work_items flag is disabled' do before do - stub_feature_flags(work_items_hierarchy: false) + stub_feature_flags(work_items: false) end it { is_expected.to be_empty } end + + context 'when work_items flag is enabled for the parent group' do + before do + stub_feature_flags(work_items: group) + end + + it { is_expected.to contain_exactly(parent_link1.work_item, parent_link2.work_item) } + end end end diff --git a/spec/models/x509_certificate_spec.rb b/spec/models/x509_certificate_spec.rb index d3b4470d3f4..5723bd80739 100644 --- a/spec/models/x509_certificate_spec.rb +++ b/spec/models/x509_certificate_spec.rb @@ -73,7 +73,9 @@ RSpec.describe X509Certificate do it 'accepts correct subject_key_identifier' do subject_key_identifiers = [ 'AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB', - 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD' + 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD', + '79:FB:C1:E5:6B:53:8B:0A', + '79:fb:c1:e5:6b:53:8b:0a' ] subject_key_identifiers.each do |identifier| @@ -83,7 +85,6 @@ RSpec.describe X509Certificate do it 'rejects invalid subject_key_identifier' do subject_key_identifiers = [ - 'AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB', 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:GG', 'random string', '12321342545356434523412341245452345623453542345234523453245' diff --git a/spec/models/x509_issuer_spec.rb b/spec/models/x509_issuer_spec.rb index f1067cad655..3d04adf7e26 100644 --- a/spec/models/x509_issuer_spec.rb +++ b/spec/models/x509_issuer_spec.rb @@ -39,7 +39,9 @@ RSpec.describe X509Issuer do it 'accepts correct subject_key_identifier' do subject_key_identifiers = [ 'AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB', - 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD' + 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD', + '79:FB:C1:E5:6B:53:8B:0A', + '79:fb:c1:e5:6b:53:8b:0a' ] subject_key_identifiers.each do |identifier| @@ -49,7 +51,6 @@ RSpec.describe X509Issuer do it 'rejects invalid subject_key_identifier' do subject_key_identifiers = [ - 'AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB:AB', 'CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:CD:GG', 'random string', '12321342545356434523412341245452345623453542345234523453245' |