Commit 5c4f6a73 authored by Drew Blessing's avatar Drew Blessing Committed by Sean McGivern

Fix cascading settings attr writer behavior

If set value matches the cascaded value, do not save locally.
This fixes an issue where a simple settings form save by a subgroup
would cause a cascaded value to be saved at the subgroup level,
defeating future cascading lookups. Also, when setting a lock
attribute and the local setting is `nil`, copy the value locally to
ensure ancestors can't change the intended value for subgroups later.

Changelog: fixed
parent 42c050db
...@@ -55,6 +55,7 @@ module CascadingNamespaceSettingAttribute ...@@ -55,6 +55,7 @@ module CascadingNamespaceSettingAttribute
# public methods # public methods
define_attr_reader(attribute) define_attr_reader(attribute)
define_attr_writer(attribute) define_attr_writer(attribute)
define_lock_attr_writer(attribute)
define_lock_methods(attribute) define_lock_methods(attribute)
alias_boolean(attribute) alias_boolean(attribute)
...@@ -97,7 +98,17 @@ module CascadingNamespaceSettingAttribute ...@@ -97,7 +98,17 @@ module CascadingNamespaceSettingAttribute
def define_attr_writer(attribute) def define_attr_writer(attribute)
define_method("#{attribute}=") do |value| define_method("#{attribute}=") do |value|
return value if value == cascaded_ancestor_value(attribute)
clear_memoization(attribute) clear_memoization(attribute)
super(value)
end
end
def define_lock_attr_writer(attribute)
define_method("lock_#{attribute}=") do |value|
attr_value = public_send(attribute) # rubocop:disable GitlabSecurity/PublicSend
write_attribute(attribute, attr_value) if self[attribute].nil?
super(value) super(value)
end end
......
---
title: Fix cascading settings attr writer behavior
merge_request: 59910
author:
type: fixed
...@@ -142,7 +142,7 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do ...@@ -142,7 +142,7 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do
end end
it 'does not allow the local value to be saved' do it 'does not allow the local value to be saved' do
subgroup_settings.delayed_project_removal = nil subgroup_settings.delayed_project_removal = false
expect { subgroup_settings.save! } expect { subgroup_settings.save! }
.to raise_error(ActiveRecord::RecordInvalid, /Delayed project removal cannot be changed because it is locked by an ancestor/) .to raise_error(ActiveRecord::RecordInvalid, /Delayed project removal cannot be changed because it is locked by an ancestor/)
...@@ -164,6 +164,19 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do ...@@ -164,6 +164,19 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do
end end
end end
describe '#delayed_project_removal=' do
before do
subgroup_settings.update!(delayed_project_removal: nil)
group_settings.update!(delayed_project_removal: true)
end
it 'does not save the value locally when it matches the cascaded value' do
subgroup_settings.update!(delayed_project_removal: true)
expect(subgroup_settings.read_attribute(:delayed_project_removal)).to eq(nil)
end
end
describe '#delayed_project_removal_locked?' do describe '#delayed_project_removal_locked?' do
shared_examples 'not locked' do shared_examples 'not locked' do
it 'is not locked by an ancestor' do it 'is not locked by an ancestor' do
...@@ -277,6 +290,13 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do ...@@ -277,6 +290,13 @@ RSpec.describe NamespaceSetting, 'CascadingNamespaceSettingAttribute' do
expect { subgroup_settings.save! } expect { subgroup_settings.save! }
.to raise_error(ActiveRecord::RecordInvalid, /Delayed project removal cannot be nil when locking the attribute/) .to raise_error(ActiveRecord::RecordInvalid, /Delayed project removal cannot be nil when locking the attribute/)
end end
it 'copies the cascaded value when locking the attribute if the local value is nil', :aggregate_failures do
subgroup_settings.delayed_project_removal = nil
subgroup_settings.lock_delayed_project_removal = true
expect(subgroup_settings.read_attribute(:delayed_project_removal)).to eq(false)
end
end end
context 'when application settings locks the attribute' do context 'when application settings locks the attribute' do
......
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