Fix cascading attribute ability to set value back to same as ancestor
What does this MR do and why?
Describe in detail what your merge request does and why.
Blocks https://gitlab.com/gitlab-org/gitlab/-/issues/359553.
@10io identified an issue where a subgroup could not set a cascaded attribute value back to a value that matches the ancestor if the subgroup had previously changed the setting value. That statement could be confusing so let's share an illustration.
- Setting
delayed_project_removal
- Top level group:
delayed_project_removal
istrue
- Subgroup:
delayed_project_removal
isfalse
.
Now the subgroup wants to change the value back to true
. Before this fix, when using the setter method (delayed_project_removal=
) that process would silently fail to set the value back to true
because it matches the value of the nearest ancestor. This behavior was initially put in place as part of !59910 (merged) to fix 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.
In reality, the behavior as fixed before only needs to occur if the local subgroup value is currently nil
. If the value is non-nil, cascading was already disabled/defeated so allowing the value to be saved back to a matching value is not a concern.
This also fixes an issue where the update service for groups/namespace settings now uses mass attribute assignment, which bypasses the custom setter. A new before_save
method ensures the same custom setter behavior is followed whether setting the individual attribute or mass assigning.
Is there an option other than a callback? Unfortunately, none that I could find. See also https://stackoverflow.com/questions/16309287/how-to-get-mass-assignment-to-utilize-custom-assignment-methods
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
How to set up and validate locally
Numbered steps to set up and validate the change are strongly suggested.
- In a top-level group, navigate to Settings -> General -> Permissions and group features -> Deletion protection and set the value to `Keep deleted projects for 7 days'. Save changes
- In a subgroup within the same hierarchy, navigate to Settings -> General -> Permissions and group features -> Deletion protection and set the value to
None, delete immediately
. Save changes - Validate the actual database setting values via the Rails console:
top_group = Group.find(123) subgroup = Group.find(456) top_group.namespace_settings.read_attribute(:delayed_project_removal) # => true subgroup.namespace_settings.read_attribute(:delayed_project_removal) # => false
- Again in the same subgroup, change the same setting value back to `Keep deleted projects for 7 days'.
- Prior to this fix, observe that running the read_attribute statement on the subgroup in the Rails console would yield:
subgroup.reload.namespace_settings.read_attribute(:delayed_project_removal) # => false
- After this fix, observe that running the read_attribute statement on the subgroup in the Rails console would yield:
subgroup.reload.namespace_settings.read_attribute(:delayed_project_removal) # => true
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.