From 71672dfa6afbb374b24ae5457a58204708d948ed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Wed, 12 Dec 2018 12:59:06 +0100
Subject: Return an ApplicationSetting in CurrentSettings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This replaces the use of fake_application_settings with
`::ApplicationSetting.build`_from_defaults. The reason is that
`fake_application_settings` doesn't have the custom accessors that
`ApplicationSetting` has, e.g. `#commit_email_hostname`, thus this
can lead to unexpected `nil` values which comes from the database
column instead of `.default_commit_email_hostname` returned by
`ApplicationSetting#commit_email_hostname`.

Using `::ApplicationSetting.build_from_defaults` should be safe as it
doesn't try to `INSERT` a DB record, in contrary to
`::ApplicationSetting.create_from_defaults` which we used to use, and
which created issues that the introduction of
`fake_application_settings` tried to resolve (575dced5).

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 spec/lib/gitlab/current_settings_spec.rb | 61 +++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 21 deletions(-)

(limited to 'spec/lib/gitlab/current_settings_spec.rb')

diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index 55490f37ac7..06772fffce7 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -91,6 +91,14 @@ describe Gitlab::CurrentSettings do
           allow(ActiveRecord::Base.connection).to receive(:cached_table_exists?).with('application_settings').and_return(true)
         end
 
+        context 'with RequestStore enabled', :request_store do
+          it 'fetches the settings from DB only once' do
+            described_class.current_application_settings # warm the cache
+
+            expect(ActiveRecord::QueryRecorder.new { described_class.current_application_settings }.count).to eq(0)
+          end
+        end
+
         it 'creates default ApplicationSettings if none are present' do
           settings = described_class.current_application_settings
 
@@ -99,34 +107,45 @@ describe Gitlab::CurrentSettings do
           expect(settings).to have_attributes(settings_from_defaults)
         end
 
-        context 'with migrations pending' do
+        context 'with pending migrations' do
           before do
             expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
           end
 
-          it 'returns an in-memory ApplicationSetting object' do
-            settings = described_class.current_application_settings
+          shared_examples 'a non-persisted ApplicationSetting object' do
+            it 'returns a non-persisted ApplicationSetting object' do
+              expect(current_settings).to be_a(ApplicationSetting)
+              expect(current_settings).not_to be_persisted
+            end
+
+            it 'uses the default value from ApplicationSetting.defaults' do
+              expect(current_settings.signup_enabled).to eq(ApplicationSetting.defaults[:signup_enabled])
+            end
 
-            expect(settings).to be_a(Gitlab::FakeApplicationSettings)
-            expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled)
-            expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled)
+            it 'uses the default value from custom ApplicationSetting accessors' do
+              expect(current_settings.commit_email_hostname).to eq(ApplicationSetting.default_commit_email_hostname)
+            end
+
+            it 'responds to predicate methods' do
+              expect(current_settings.signup_enabled?).to eq(current_settings.signup_enabled)
+            end
+          end
+
+          context 'with no ApplicationSetting DB record' do
+            it_behaves_like 'a non-persisted ApplicationSetting object' do
+              let(:current_settings) { described_class.current_application_settings }
+            end
           end
 
-          it 'uses the existing database settings and falls back to defaults' do
-            db_settings = create(:application_setting,
-                                 home_page_url: 'http://mydomain.com',
-                                 signup_enabled: false)
-            settings = described_class.current_application_settings
-            app_defaults = ApplicationSetting.last
-
-            expect(settings).to be_a(Gitlab::FakeApplicationSettings)
-            expect(settings.home_page_url).to eq(db_settings.home_page_url)
-            expect(settings.signup_enabled?).to be_falsey
-            expect(settings.signup_enabled).to be_falsey
-
-            # Check that unspecified values use the defaults
-            settings.reject! { |key, _| [:home_page_url, :signup_enabled].include? key }
-            settings.each { |key, _| expect(settings[key]).to eq(app_defaults[key]) }
+          context 'with an existing ApplicationSetting DB record' do
+            let!(:db_settings) { ApplicationSetting.build_from_defaults(home_page_url: 'http://mydomain.com').save! && ApplicationSetting.last }
+            let(:current_settings) { described_class.current_application_settings }
+
+            it_behaves_like 'a non-persisted ApplicationSetting object'
+
+            it 'uses the value from the DB attribute if present and not overriden by an accessor' do
+              expect(current_settings.home_page_url).to eq(db_settings.home_page_url)
+            end
           end
         end
 
-- 
cgit v1.2.1