diff options
4 files changed, 55 insertions, 23 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 4319db42019..54139f61a16 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -382,7 +382,7 @@ class ApplicationSetting < ActiveRecord::Base end def restricted_visibility_levels=(levels) - super(levels.map { |level| Gitlab::VisibilityLevel.level_value(level) }) + super(levels&.map { |level| Gitlab::VisibilityLevel.level_value(level) }) end def strip_sentry_values diff --git a/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml b/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml new file mode 100644 index 00000000000..8fd127acf2b --- /dev/null +++ b/changelogs/unreleased/54953-error-500-viewing-merge-request-due-to-nil-commit_email_hostname.yml @@ -0,0 +1,5 @@ +--- +title: Return an ApplicationSetting in CurrentSettings +merge_request: 23766 +author: +type: fixed diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 477f9101e98..e6c80bed5a5 100644 --- a/lib/gitlab/current_settings.rb +++ b/lib/gitlab/current_settings.rb @@ -50,7 +50,15 @@ module Gitlab # and other callers from failing, use any loaded settings and return # defaults for missing columns. if ActiveRecord::Migrator.needs_migration? - return fake_application_settings(current_settings&.attributes) + db_attributes = current_settings&.attributes || {} + column_names = ::ApplicationSetting.column_names + final_attributes = ::ApplicationSetting + .defaults + .merge(db_attributes) + .stringify_keys + .slice(*column_names) + + return ::ApplicationSetting.new(final_attributes) end return current_settings if current_settings.present? 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 |