Commit 681af5bc authored by Stan Hu's avatar Stan Hu

Fix Error 500 when application settings are saved

Due to a Rails bug, fetching the application settings from Redis
may prevent the attribute methods from being loaded for the `ApplicationSetting`
model. More details here: https://github.com/rails/rails/issues/27348

There was also a secondary problem introduced by overriding these
association methods which caused all default visibility levels to be
set to `nil`. Before, the previous implementation allowed the string
"20" to be saved as an integer, while now a table lookup happens
before that. We fix this by enforcing the integer value in the
controller and default to PRIVATE.

Closes #29674
parent 5d71d9fb
...@@ -45,6 +45,18 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController ...@@ -45,6 +45,18 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
end end
def application_setting_params def application_setting_params
default_visibilities = [:default_project_visibility, :default_snippet_visibility, :default_group_visibility]
default_visibilities.each do |visibility|
value = params[:application_setting][visibility]
params[:application_setting][visibility] =
if value.present?
value.to_i
else
Gitlab::VisibilityLevel::PRIVATE
end
end
restricted_levels = params[:application_setting][:restricted_visibility_levels] restricted_levels = params[:application_setting][:restricted_visibility_levels]
if restricted_levels.nil? if restricted_levels.nil?
params[:application_setting][:restricted_visibility_levels] = [] params[:application_setting][:restricted_visibility_levels] = []
......
...@@ -163,6 +163,8 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -163,6 +163,8 @@ class ApplicationSetting < ActiveRecord::Base
end end
def self.current def self.current
ensure_cache_setup
Rails.cache.fetch(CACHE_KEY) do Rails.cache.fetch(CACHE_KEY) do
ApplicationSetting.last ApplicationSetting.last
end end
...@@ -176,9 +178,16 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -176,9 +178,16 @@ class ApplicationSetting < ActiveRecord::Base
end end
def self.cached def self.cached
ensure_cache_setup
Rails.cache.fetch(CACHE_KEY) Rails.cache.fetch(CACHE_KEY)
end end
def self.ensure_cache_setup
# This is a workaround for a Rails bug that causes attribute methods not
# to be loaded when read from cache: https://github.com/rails/rails/issues/27348
ApplicationSetting.define_attribute_methods
end
def self.defaults_ce def self.defaults_ce
{ {
after_sign_up_text: nil, after_sign_up_text: nil,
......
...@@ -9,6 +9,14 @@ feature 'Admin updates settings', feature: true do ...@@ -9,6 +9,14 @@ feature 'Admin updates settings', feature: true do
visit admin_application_settings_path visit admin_application_settings_path
end end
scenario 'Change visibility settings' do
first(:radio_button, 'Public').set(true)
click_button 'Save'
expect(page).to have_content "Application settings saved successfully"
expect(ApplicationSetting.current.default_project_visibility).to eq(Gitlab::VisibilityLevel::PUBLIC)
end
scenario 'Change application settings' do scenario 'Change application settings' do
uncheck 'Gravatar enabled' uncheck 'Gravatar enabled'
fill_in 'Home page URL', with: 'https://about.gitlab.com/' fill_in 'Home page URL', with: 'https://about.gitlab.com/'
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment