Merge branch '46758-fallout-of-cacheable-attribute' into 'master'

Ensure ApplicationSetting#performance_bar_allowed_group_id is properly set when retrieved from cache

Closes #46758

See merge request gitlab-org/gitlab-ce!19144

5 files changed, 129 insertions, 124 deletions

diff --git a/spec/factories/application_settings.rb b/spec/factories/application_settings.rb
index 3ecc90b6573..00c063c49f8 100644
--- a/spec/factories/application_settings.rb
+++ b/spec/factories/application_settings.rb
@@ -1,4 +1,5 @@
 FactoryBot.define do
   factory :application_setting do
+    default_projects_limit 42
   end
 end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 7e47043a1cb..f8f07205623 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -391,68 +391,6 @@ describe ApplicationSetting do
   end
 
   describe 'performance bar settings' do
-    describe 'performance_bar_allowed_group_id=' do
-      context 'with a blank path' do
-        before do
-          setting.performance_bar_allowed_group_id = create(:group).full_path
-        end
-
-        it 'persists nil for a "" path and clears allowed user IDs cache' do
-          expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
-
-          setting.performance_bar_allowed_group_id = ''
-
-          expect(setting.performance_bar_allowed_group_id).to be_nil
-        end
-      end
-
-      context 'with an invalid path' do
-        it 'does not persist an invalid group path' do
-          setting.performance_bar_allowed_group_id = 'foo'
-
-          expect(setting.performance_bar_allowed_group_id).to be_nil
-        end
-      end
-
-      context 'with a path to an existing group' do
-        let(:group) { create(:group) }
-
-        it 'persists a valid group path and clears allowed user IDs cache' do
-          expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
-
-          setting.performance_bar_allowed_group_id = group.full_path
-
-          expect(setting.performance_bar_allowed_group_id).to eq(group.id)
-        end
-
-        context 'when the given path is the same' do
-          context 'with a blank path' do
-            before do
-              setting.performance_bar_allowed_group_id = nil
-            end
-
-            it 'clears the cached allowed user IDs' do
-              expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
-
-              setting.performance_bar_allowed_group_id = ''
-            end
-          end
-
-          context 'with a valid path' do
-            before do
-              setting.performance_bar_allowed_group_id = group.full_path
-            end
-
-            it 'clears the cached allowed user IDs' do
-              expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
-
-              setting.performance_bar_allowed_group_id = group.full_path
-            end
-          end
-        end
-      end
-    end
-
     describe 'performance_bar_allowed_group' do
       context 'with no performance_bar_allowed_group_id saved' do
         it 'returns nil' do
@@ -464,11 +402,11 @@ describe ApplicationSetting do
         let(:group) { create(:group) }
 
         before do
-          setting.performance_bar_allowed_group_id = group.full_path
+          setting.update!(performance_bar_allowed_group_id: group.id)
         end
 
         it 'returns the group' do
-          expect(setting.performance_bar_allowed_group).to eq(group)
+          expect(setting.reload.performance_bar_allowed_group).to eq(group)
         end
       end
     end
@@ -478,67 +416,11 @@ describe ApplicationSetting do
         let(:group) { create(:group) }
 
         before do
-          setting.performance_bar_allowed_group_id = group.full_path
+          setting.update!(performance_bar_allowed_group_id: group.id)
         end
 
         it 'returns true' do
-          expect(setting.performance_bar_enabled).to be_truthy
-        end
-      end
-    end
-
-    describe 'performance_bar_enabled=' do
-      context 'when the performance bar is enabled' do
-        let(:group) { create(:group) }
-
-        before do
-          setting.performance_bar_allowed_group_id = group.full_path
-        end
-
-        context 'when passing true' do
-          it 'does not clear allowed user IDs cache' do
-            expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
-
-            setting.performance_bar_enabled = true
-
-            expect(setting.performance_bar_allowed_group_id).to eq(group.id)
-            expect(setting.performance_bar_enabled).to be_truthy
-          end
-        end
-
-        context 'when passing false' do
-          it 'disables the performance bar and clears allowed user IDs cache' do
-            expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
-
-            setting.performance_bar_enabled = false
-
-            expect(setting.performance_bar_allowed_group_id).to be_nil
-            expect(setting.performance_bar_enabled).to be_falsey
-          end
-        end
-      end
-
-      context 'when the performance bar is disabled' do
-        context 'when passing true' do
-          it 'does nothing and does not clear allowed user IDs cache' do
-            expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
-
-            setting.performance_bar_enabled = true
-
-            expect(setting.performance_bar_allowed_group_id).to be_nil
-            expect(setting.performance_bar_enabled).to be_falsey
-          end
-        end
-
-        context 'when passing false' do
-          it 'does nothing and does not clear allowed user IDs cache' do
-            expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
-
-            setting.performance_bar_enabled = false
-
-            expect(setting.performance_bar_allowed_group_id).to be_nil
-            expect(setting.performance_bar_enabled).to be_falsey
-          end
+          expect(setting.reload.performance_bar_enabled).to be_truthy
         end
       end
     end
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 8b22d1e72f3..aead8978dd4 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -24,10 +24,15 @@ describe API::Settings, 'Settings' do
       expect(json_response['ecdsa_key_restriction']).to eq(0)
       expect(json_response['ed25519_key_restriction']).to eq(0)
       expect(json_response['circuitbreaker_failure_count_threshold']).not_to be_nil
+      expect(json_response['performance_bar_allowed_group_id']).to be_nil
+      expect(json_response).not_to have_key('performance_bar_allowed_group_path')
+      expect(json_response).not_to have_key('performance_bar_enabled')
     end
   end
 
   describe "PUT /application/settings" do
+    let(:group) { create(:group) }
+
     context "custom repository storage type set in the config" do
       before do
         storages = { 'custom' => 'tmp/tests/custom_repositories' }
@@ -56,7 +61,8 @@ describe API::Settings, 'Settings' do
           ed25519_key_restriction: 256,
           circuitbreaker_check_interval: 2,
           enforce_terms: true,
-          terms: 'Hello world!'
+          terms: 'Hello world!',
+          performance_bar_allowed_group_path: group.full_path
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['default_projects_limit']).to eq(3)
@@ -80,9 +86,27 @@ describe API::Settings, 'Settings' do
         expect(json_response['circuitbreaker_check_interval']).to eq(2)
         expect(json_response['enforce_terms']).to be(true)
         expect(json_response['terms']).to eq('Hello world!')
+        expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
       end
     end
 
+    it "supports legacy performance_bar_allowed_group_id" do
+      put api("/application/settings", admin),
+        performance_bar_allowed_group_id: group.full_path
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['performance_bar_allowed_group_id']).to eq(group.id)
+    end
+
+    it "supports legacy performance_bar_enabled" do
+      put api("/application/settings", admin),
+        performance_bar_enabled: false,
+        performance_bar_allowed_group_id: group.full_path
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['performance_bar_allowed_group_id']).to be_nil
+    end
+
     context "missing koding_url value when koding_enabled is true" do
       it "returns a blank parameter error message" do
         put api("/application/settings", admin), koding_enabled: true
diff --git a/spec/rubocop/cop/line_break_around_conditional_block_spec.rb b/spec/rubocop/cop/line_break_around_conditional_block_spec.rb
index 7ddf9141fcd..03eeffe6483 100644
--- a/spec/rubocop/cop/line_break_around_conditional_block_spec.rb
+++ b/spec/rubocop/cop/line_break_around_conditional_block_spec.rb
@@ -256,6 +256,18 @@ describe RuboCop::Cop::LineBreakAroundConditionalBlock do
       expect(cop.offenses).to be_empty
     end
 
+    it "doesn't flag violation for #{conditional} followed by a comment" do
+      source = <<~RUBY
+          #{conditional} condition
+            do_something
+          end
+          # a short comment
+      RUBY
+      inspect_source(source)
+
+      expect(cop.offenses).to be_empty
+    end
+
     it "doesn't flag violation for #{conditional} followed by an end" do
       source = <<~RUBY
           class Foo
diff --git a/spec/services/application_settings/update_service_spec.rb b/spec/services/application_settings/update_service_spec.rb
index fb07ecc6ae8..6337ee7d724 100644
--- a/spec/services/application_settings/update_service_spec.rb
+++ b/spec/services/application_settings/update_service_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe ApplicationSettings::UpdateService do
-  let(:application_settings) { Gitlab::CurrentSettings.current_application_settings }
+  let(:application_settings) { create(:application_setting) }
   let(:admin) { create(:user, :admin) }
   let(:params) { {} }
 
@@ -54,4 +54,90 @@ describe ApplicationSettings::UpdateService do
       end
     end
   end
+
+  describe 'performance bar settings' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:params_performance_bar_enabled,
+      :params_performance_bar_allowed_group_path,
+      :previous_performance_bar_allowed_group_id,
+      :expected_performance_bar_allowed_group_id) do
+      true | '' | nil | nil
+      true | '' | 42_000_000 | nil
+      true | nil | nil | nil
+      true | nil | 42_000_000 | nil
+      true | 'foo' | nil | nil
+      true | 'foo' | 42_000_000 | nil
+      true | 'group_a' | nil | 42_000_000
+      true | 'group_b' | 42_000_000 | 43_000_000
+      true | 'group_a' | 42_000_000 | 42_000_000
+      false | '' | nil | nil
+      false | '' | 42_000_000 | nil
+      false | nil | nil | nil
+      false | nil | 42_000_000 | nil
+      false | 'foo' | nil | nil
+      false | 'foo' | 42_000_000 | nil
+      false | 'group_a' | nil | nil
+      false | 'group_b' | 42_000_000 | nil
+      false | 'group_a' | 42_000_000 | nil
+    end
+
+    with_them do
+      let(:params) do
+        {
+          performance_bar_enabled: params_performance_bar_enabled,
+          performance_bar_allowed_group_path: params_performance_bar_allowed_group_path
+        }
+      end
+
+      before do
+        if previous_performance_bar_allowed_group_id == 42_000_000 || params_performance_bar_allowed_group_path == 'group_a'
+          create(:group, id: 42_000_000, path: 'group_a')
+        end
+
+        if expected_performance_bar_allowed_group_id == 43_000_000 || params_performance_bar_allowed_group_path == 'group_b'
+          create(:group, id: 43_000_000, path: 'group_b')
+        end
+
+        application_settings.update!(performance_bar_allowed_group_id: previous_performance_bar_allowed_group_id)
+      end
+
+      it 'sets performance_bar_allowed_group_id when present and performance_bar_enabled == true' do
+        expect(application_settings.performance_bar_allowed_group_id).to eq(previous_performance_bar_allowed_group_id)
+
+        if previous_performance_bar_allowed_group_id != expected_performance_bar_allowed_group_id
+          expect { subject.execute }
+            .to change(application_settings, :performance_bar_allowed_group_id)
+            .from(previous_performance_bar_allowed_group_id).to(expected_performance_bar_allowed_group_id)
+        else
+          expect { subject.execute }
+            .not_to change(application_settings, :performance_bar_allowed_group_id)
+        end
+      end
+    end
+
+    context 'when :performance_bar_allowed_group_path is not present' do
+      let(:group) { create(:group) }
+
+      before do
+        application_settings.update!(performance_bar_allowed_group_id: group.id)
+      end
+
+      it 'does not change the performance bar settings' do
+        expect { subject.execute }
+          .not_to change(application_settings, :performance_bar_allowed_group_id)
+      end
+    end
+
+    context 'when :performance_bar_enabled is not present' do
+      let(:group) { create(:group) }
+      let(:params) { { performance_bar_allowed_group_path: group.full_path } }
+
+      it 'implicitely defaults to true' do
+        expect { subject.execute }
+          .to change(application_settings, :performance_bar_allowed_group_id)
+          .from(nil).to(group.id)
+      end
+    end
+  end
 end