Improve validations for Protected Ref protected_ref_access_levels

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

This came up in an import-related MR: !131958 (comment 1564294464)

I updated the validates :"#{type}_access_levels validation in app/models/concerns/protected_ref.rb in the wrong way but no specs failed as far as we could tell. I tried to backfill specs myself but it was getting complex so I decided to open an issue instead.

Add test coverage for this validation

• validates :"#{type}_access_levels", length: { is: 1, message: "are restricted to a single instance per #{self.model_name.human}." }, unless: -> { allow_multiple?(type) } is part of the ProtectedRef concern, which is included in ProtectedBranch and ProtectedTag.
• Ths validation is overridden in ee/app/models/concerns/ee/protected_ref.rb to validates :"#{type}_access_levels", length: { is: 1 }, if: -> { false } so that the validation is actually not run in an EE context.
• There are no EE-specific models for ProtectedBranch and ProtectedTag so I am not sure where would make most sense to add unit tests for this differing behavior in EE vs non-EE contexts. If I added a unit test to spec/models/protected_branch_spec.rb, it will pass when run with FOSS_ONLY=1 prepended to the spec run but not otherwise (or vice versa depending on the spec expectation)
  • Idea: We sometimes have return if/unless Gitlab.ee? in our tests, so a test will only execute in the FOSS-only pipeline, or only in the regular pipeline that “runs as EE”. That might give the control to isolate the test to either EE or FOSS
• I opened a Draft MR that changes this validation to never run here: Draft: Check for test coverage of validation (!132077 - closed) -- this should help determine if we have any indirect test coverage for it

Update EE validation

In the regular ProtectedRef concern, we validate that there is exactly 1 "#{type}_access_levels". In the EE::ProtectedRef concern, we do not validate the length at all. Shouldn't we update the validation to ensure that there is at least 1? It seems like the intention is to allow multiple but it seems odd to me that ProtectedRef requires exactly 1 and EE::ProtectedRef has no size limitation for this relation.