Applied suggestions from reviewer

Cleaned up specs mostly.

app/models/protected_branch/push_access_level.rb

@@ -37,7 +37,8 @@ class ProtectedBranch::PushAccessLevel < ApplicationRecord
   end
 
   def enabled_deploy_key_for_user?(deploy_key, user)
-    DeployKey.with_write_access_for_project(protected_branch.project, deploy_key: deploy_key).any? &&
-      deploy_key.user_id == user.id
+    return false unless deploy_key.user_id == user.id
+
+    DeployKey.with_write_access_for_project(protected_branch.project, deploy_key: deploy_key).any?
   end
 end

lib/gitlab/deploy_key_access.rb

@@ -2,11 +2,10 @@
 
 module Gitlab
   class DeployKeyAccess < UserAccess
-    def initialize(deploy_key, container: nil, push_ability: :push_code)
+    def initialize(deploy_key, container: nil)
       @deploy_key = deploy_key
       @user = deploy_key.user
       @container = container
-      @push_ability = push_ability
     end
 
     private
@@ -16,10 +15,12 @@ module Gitlab
     def protected_tag_accessible_to?(ref, action:)
       assert_project!
 
+      # a deploy key can always push a protected tag
+      # (which is not always the case when pushing to a protected branch)
       true
     end
 
-    def can_collaborate?(ref)
+    def can_collaborate?(_ref)
       assert_project!
 
       project_has_active_user_keys?

spec/lib/gitlab/deploy_key_access_spec.rb

@@ -7,56 +7,58 @@ RSpec.describe Gitlab::DeployKeyAccess do
   let_it_be(:deploy_key) { create(:deploy_key, user: user) }
   let(:project) { create(:project, :repository) }
   let(:protected_branch) { create(:protected_branch, :no_one_can_push, project: project) }
-  let(:access) { described_class.new(deploy_key, container: project) }
+
+  subject(:access) { described_class.new(deploy_key, container: project) }
 
   before do
     project.add_guest(user)
-    allow(project).to receive(:branch_allows_collaboration?).and_return(false)
-    stub_feature_flags(deploy_keys_on_protected_branches: true)
+    create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key)
   end
 
-  context 'push tag that matches a protected tag pattern via a deploy key' do
-    it 'still pushes that tag' do
-      create(:deploy_keys_project, :write_access, project: project, deploy_key: deploy_key)
-      create(:protected_tag, project: project, name: 'v*')
+  describe '#can_create_tag?' do
+    context 'push tag that matches a protected tag pattern via a deploy key' do
+      it 'still pushes that tag' do
+        create(:protected_tag, project: project, name: 'v*')
 
-      expect(access.can_create_tag?('v0.1.2')).to be_truthy
+        expect(access.can_create_tag?('v0.1.2')).to be_truthy
+      end
     end
   end
 
-  context 'push to a protected branch of this project via a deploy key' do
-    before do
-      create(:deploy_keys_project, :write_access, project: protected_branch.project, deploy_key: deploy_key)
-      create(:protected_branch_push_access_level, protected_branch: protected_branch, deploy_key: deploy_key)
-    end
+  describe '#can_push_to_branch?' do
+    context 'push to a protected branch of this project via a deploy key' do
+      before do
+        create(:protected_branch_push_access_level, protected_branch: protected_branch, deploy_key: deploy_key)
+      end
 
-    context 'when the project has active deploy key owned by this user' do
-      it 'returns true' do
-        expect(access.can_push_to_branch?(protected_branch.name)).to be_truthy
+      context 'when the project has active deploy key owned by this user' do
+        it 'returns true' do
+          expect(access.can_push_to_branch?(protected_branch.name)).to be_truthy
+        end
       end
-    end
 
-    context 'when the project has active deploy keys, but not by this user' do
-      let(:deploy_key) { create(:deploy_key, user: create(:user)) }
+      context 'when the project has active deploy keys, but not by this user' do
+        let(:deploy_key) { create(:deploy_key, user: create(:user)) }
 
-      it 'returns false' do
-        expect(access.can_push_to_branch?(protected_branch.name)).to be_falsey
+        it 'returns false' do
+          expect(access.can_push_to_branch?(protected_branch.name)).to be_falsey
+        end
       end
-    end
 
-    context 'when there is another branch no one can push to' do
-      let(:another_branch) { create(:protected_branch, :no_one_can_push, name: 'another_branch', project: project) }
+      context 'when there is another branch no one can push to' do
+        let(:another_branch) { create(:protected_branch, :no_one_can_push, name: 'another_branch', project: project) }
 
-      it 'returns false when trying to push to that other branch' do
-        expect(access.can_push_to_branch?(another_branch.name)).to be_falsey
-      end
+        it 'returns false when trying to push to that other branch' do
+          expect(access.can_push_to_branch?(another_branch.name)).to be_falsey
+        end
 
-      context 'and the deploy key added for the first protected branch is also added for this other branch' do
-        it 'returns true for both protected branches' do
-          create(:protected_branch_push_access_level, protected_branch: another_branch, deploy_key: deploy_key)
+        context 'and the deploy key added for the first protected branch is also added for this other branch' do
+          it 'returns true for both protected branches' do
+            create(:protected_branch_push_access_level, protected_branch: another_branch, deploy_key: deploy_key)
 
-          expect(access.can_push_to_branch?(protected_branch.name)).to be_truthy
-          expect(access.can_push_to_branch?(another_branch.name)).to be_truthy
+            expect(access.can_push_to_branch?(protected_branch.name)).to be_truthy
+            expect(access.can_push_to_branch?(another_branch.name)).to be_truthy
+          end
         end
       end
     end

spec/models/protected_branch/push_access_level_spec.rb

@@ -40,24 +40,35 @@ RSpec.describe ProtectedBranch::PushAccessLevel do
     let_it_be(:protected_branch) { create(:protected_branch, :no_one_can_push, project: project) }
     let_it_be(:user) { create(:user) }
     let_it_be(:deploy_key) { create(:deploy_key, user: user) }
-    let!(:deploy_keys_project) { create(:deploy_keys_project, project: project, deploy_key: deploy_key) }
+    let!(:deploy_keys_project) { create(:deploy_keys_project, project: project, deploy_key: deploy_key, can_push: can_push) }
+    let(:can_push) { true }
 
-    before do
+    before_all do
       project.add_guest(user)
-      stub_feature_flags(deploy_keys_on_protected_branches: true)
     end
+
     context 'when this push_access_level is tied to a deploy key' do
       let(:push_access_level) { create(:protected_branch_push_access_level, protected_branch: protected_branch, deploy_key: deploy_key) }
 
       context 'when the deploy key is among the active keys for this project' do
-        it 'is true' do
-          deploy_key.deploy_keys_projects.last.update!(can_push: true)
-
+        specify do
           expect(push_access_level.check_access(user)).to be_truthy
         end
+
+        context 'when the deploy_keys_on_protected_branches FF is false' do
+          before do
+            stub_feature_flags(deploy_keys_on_protected_branches: false)
+          end
+
+          it 'is false' do
+            expect(push_access_level.check_access(user)).to be_falsey
+          end
+        end
       end
 
       context 'when the deploy key is not among the active keys of this project' do
+        let(:can_push) { false }
+
         it 'is false' do
           expect(push_access_level.check_access(user)).to be_falsey
         end