Skip to content

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 is true
  • Subgroup: delayed_project_removal is false.

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.

  1. 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
  2. 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
  3. 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
  4. Again in the same subgroup, change the same setting value back to `Keep deleted projects for 7 days'.
  5. 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
  6. 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.

Edited by Drew Blessing

Merge request reports

Loading