Skip to content

Disallow NO_ACCESS for UnprotectAccessLevel#access_level

Joe Woodward requested to merge fix/unprotect_access_levels into master

What does this MR do and why?

Related to !112035 (merged) Closes #391693 (closed)

I noticed we'd added a method override to prevent NO_ACCESS values in the ProtectedBranch::UnprotectAccessLevel#access_level column. In the merge request above, we added tests for the API which work correctly, however, we also have tests for the model which were still passing when supplying NO_ACCESS.

When ProtectedBranch::UnprotectAccessLevel calls include ProtectedBranchAccess a AR validator is defined which calls the allowed_access_levels method. At this point the ProtectedBranch::UnprotectAcceesLevel class hasn't defined that method yet. The validator expects an Enumarable for the inclusion validators in: option so we must call the allowed_access_levels method at this point in the runtime, we cannot use a proc or an instance method like other options e.g. if: :my_instance_method. As this is evaluated before the ProtectedBranch::UnprotectAccessLevel class has defined the method we fail to override the values. We must include ProtectedBranchAccess after we define the method.

I'm still not sure why the API works, I think it may be related to ruby's class caching mechanisms.

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 charlie ablett

Merge request reports