If migrations are pending, make CurrentSettings use existing values and populate missing columns with defaultssh-refactor-current-settings

master was failing because `ApplicationSetting.create_from_defaults` attempted
to write to a column that did not exist in the database. This occurred in a
`rake db:migrate` task, which was unable to perform the migration that would
have added the missing column in the first place.

In 9.3 RC2, we also had a bug where password sign-ins were disabled because
there were many pending migrations. The problem occurred because
`fake_application_settings` was being returned with an OpenStruct that did not
include the predicate method `signup_enabled?`. As a result, the value would
erroneously return `nil` instead of `true`. This commit uses the values of the
defaults to mimic this behavior.

This commit also refactors some of the logic to be clearer.

3 files changed, 67 insertions, 0 deletions

diff --git a/spec/factories/application_settings.rb b/spec/factories/application_settings.rb
new file mode 100644
index 00000000000..aef65e724c2
--- /dev/null
+++ b/spec/factories/application_settings.rb
@@ -0,0 +1,4 @@
+FactoryGirl.define do
+  factory :application_setting do
+  end
+end
diff --git a/spec/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb
index fda39d78610..a566f24f6a6 100644
--- a/spec/lib/gitlab/current_settings_spec.rb
+++ b/spec/lib/gitlab/current_settings_spec.rb
@@ -32,6 +32,37 @@ describe Gitlab::CurrentSettings do
 
         expect(current_application_settings).to be_a(ApplicationSetting)
       end
+
+      context 'with migrations pending' do
+        before do
+          expect(ActiveRecord::Migrator).to receive(:needs_migration?).and_return(true)
+        end
+
+        it 'returns an in-memory ApplicationSetting object' do
+          settings = current_application_settings
+
+          expect(settings).to be_a(OpenStruct)
+          expect(settings.sign_in_enabled?).to eq(settings.sign_in_enabled)
+          expect(settings.sign_up_enabled?).to eq(settings.sign_up_enabled)
+        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 = current_application_settings
+          app_defaults = ApplicationSetting.last
+
+          expect(settings).to be_a(OpenStruct)
+          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]) }
+        end
+      end
     end
 
     context 'with DB unavailable' do
diff --git a/spec/lib/gitlab/fake_application_settings_spec.rb b/spec/lib/gitlab/fake_application_settings_spec.rb
new file mode 100644
index 00000000000..b793176d84a
--- /dev/null
+++ b/spec/lib/gitlab/fake_application_settings_spec.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe Gitlab::FakeApplicationSettings do
+  let(:defaults) { { signin_enabled: false, foobar: 'asdf', signup_enabled: true, 'test?' => 123 } }
+
+  subject { described_class.new(defaults) }
+
+  it 'wraps OpenStruct variables properly' do
+    expect(subject.signin_enabled).to be_falsey
+    expect(subject.signup_enabled).to be_truthy
+    expect(subject.foobar).to eq('asdf')
+  end
+
+  it 'defines predicate methods' do
+    expect(subject.signin_enabled?).to be_falsey
+    expect(subject.signup_enabled?).to be_truthy
+  end
+
+  it 'predicate method changes when value is updated' do
+    subject.signin_enabled = true
+
+    expect(subject.signin_enabled?).to be_truthy
+  end
+
+  it 'does not define a predicate method' do
+    expect(subject.foobar?).to be_nil
+  end
+
+  it 'does not override an existing predicate method' do
+    expect(subject.test?).to eq(123)
+  end
+end