Improve the uniqueness validation of protected branch access levels.

1. For a user-based access level, don't validate `access_level`
2. For a role-based access level, don't look at `access_level` of user-based access levels

Add tests supporting this behavior.

app/models/concerns/protected_branch_access.rb

@@ -3,7 +3,7 @@ module ProtectedBranchAccess
 
   included do
     validates_uniqueness_of :user_id, scope: :protected_branch, allow_nil: true
-    validates_uniqueness_of :access_level, scope: :protected_branch, unless: :user_id?
+    validates_uniqueness_of :access_level, scope: :protected_branch, unless: :user_id?, conditions: -> { where(user_id: nil) }
   end
 
   def type

spec/models/protected_branch_spec.rb

@@ -9,10 +9,12 @@ describe ProtectedBranch, models: true do
 
   describe "Uniqueness validations" do
     [ProtectedBranch::MergeAccessLevel, ProtectedBranch::PushAccessLevel].each do |access_level_class|
+      let(:user) { create(:user) }
       let(:factory_name) { access_level_class.to_s.underscore.sub('/', '_').to_sym }
       let(:association_name) { access_level_class.to_s.underscore.sub('protected_branch/', '').pluralize.to_sym }
       human_association_name = access_level_class.to_s.underscore.humanize.sub('Protected branch/', '')
 
+      context "while checking uniqueness of a role-based #{human_association_name}" do
         it "allows a single #{human_association_name} for a role (per protected branch)" do
           first_protected_branch = create(:protected_branch, :remove_default_access_levels)
           second_protected_branch = create(:protected_branch, :remove_default_access_levels)
@@ -28,8 +30,19 @@ describe ProtectedBranch, models: true do
           expect(first_protected_branch.errors.full_messages.first).to match("access level has already been taken")
         end
 
+        it "does not count a user-based #{human_association_name} with an `access_level` set" do
+          protected_branch = create(:protected_branch, :remove_default_access_levels)
+
+          protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
+          protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
+
+          expect(protected_branch).to be_valid
+        end
+      end
+
+
+      context "while checking uniqueness of a user-based #{human_association_name}" do
         it "allows a single #{human_association_name} for a user (per protected branch)" do
-        user = create(:user)
           first_protected_branch = create(:protected_branch, :remove_default_access_levels)
           second_protected_branch = create(:protected_branch, :remove_default_access_levels)
 
@@ -43,6 +56,16 @@ describe ProtectedBranch, models: true do
           expect(first_protected_branch).to be_invalid
           expect(first_protected_branch.errors.full_messages.first).to match("user has already been taken")
         end
+
+        it "ignores the `access_level` while validating a user-based #{human_association_name}" do
+          protected_branch = create(:protected_branch, :remove_default_access_levels)
+
+          protected_branch.send(association_name) << build(factory_name, access_level: Gitlab::Access::MASTER)
+          protected_branch.send(association_name) << build(factory_name, user: user, access_level: Gitlab::Access::MASTER)
+
+          expect(protected_branch).to be_valid
+        end
+      end
     end
   end