Extract any-branch-allows-collaboration logic into dedicated method

3 files changed, 35 insertions, 25 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index cd558752080..930f6c9911f 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1928,9 +1928,19 @@ class Project < ActiveRecord::Base
                                 .where('project_authorizations.project_id = merge_requests.target_project_id')
                                 .limit(1)
                                 .select(1)
-    source_of_merge_requests.opened
-      .where(allow_collaboration: true)
-      .where('EXISTS (?)', developer_access_exists)
+    merge_requests_allowing_collaboration.where('EXISTS (?)', developer_access_exists)
+  end
+
+  def any_branch_allows_collaboration?(user)
+    return false unless user
+    return false if empty_repo?
+
+    # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/49322
+    Gitlab::GitalyClient.allow_n_plus_1_calls do
+      merge_requests_allowing_collaboration.any? do |merge_request|
+        merge_request.can_be_merged_by?(user)
+      end
+    end
   end
 
   def branch_allows_collaboration?(user, branch_name)
@@ -2018,6 +2028,10 @@ class Project < ActiveRecord::Base
 
   private
 
+  def merge_requests_allowing_collaboration
+    source_of_merge_requests.opened.where(allow_collaboration: true)
+  end
+
   def create_new_pool_repository
     pool = begin
              create_pool_repository!(shard: Shard.by_name(repository_storage), source_project: self)
@@ -2143,24 +2157,11 @@ class Project < ActiveRecord::Base
   end
 
   def fetch_branch_allows_collaboration?(user, branch_name)
-    check_access = -> do
+    Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
       next false if empty_repo?
 
-      merge_requests = source_of_merge_requests.opened
-                         .where(allow_collaboration: true)
-
-      # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/49322
-      Gitlab::GitalyClient.allow_n_plus_1_calls do
-        if branch_name
-          merge_requests.find_by(source_branch: branch_name)&.can_be_merged_by?(user)
-        else
-          merge_requests.any? { |merge_request| merge_request.can_be_merged_by?(user) }
-        end
-      end
-    end
-
-    Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
-      check_access.call
+      merge_request = merge_requests_allowing_collaboration.find_by(source_branch: branch_name)
+      merge_request&.can_be_merged_by?(user)
     end
   end
 
diff --git a/spec/lib/gitlab/checks/push_check_spec.rb b/spec/lib/gitlab/checks/push_check_spec.rb
index 25f0d428cb9..e1bd52d6c0b 100644
--- a/spec/lib/gitlab/checks/push_check_spec.rb
+++ b/spec/lib/gitlab/checks/push_check_spec.rb
@@ -13,7 +13,7 @@ describe Gitlab::Checks::PushCheck do
     context 'when the user is not allowed to push to the repo' do
       it 'raises an error' do
         expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false)
-        expect(user_access).to receive(:can_push_to_branch?).with('master').and_return(false)
+        expect(project).to receive(:branch_allows_collaboration?).with(user_access.user, 'master').and_return(false)
 
         expect { subject.validate! }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.')
       end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4b6592020c1..131de62fe03 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3875,14 +3875,23 @@ describe Project do
       end
     end
 
-    describe '#branch_allows_collaboration_push?' do
-      it 'allows access if the user can merge the merge request' do
-        expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
+    describe '#any_branch_allows_collaboration?' do
+      it 'allows access when there are merge requests open allowing collaboration' do
+        expect(project.any_branch_allows_collaboration?(user))
           .to be_truthy
       end
 
-      it 'allows access when there are merge requests open but no branch name is given' do
-        expect(project.branch_allows_collaboration?(user, nil))
+      it 'does not allow access when there are no merge requests open allowing collaboration' do
+        merge_request.close!
+
+        expect(project.any_branch_allows_collaboration?(user))
+          .to be_falsey
+      end
+    end
+
+    describe '#branch_allows_collaboration?' do
+      it 'allows access if the user can merge the merge request' do
+        expect(project.branch_allows_collaboration?(user, 'awesome-feature-1'))
           .to be_truthy
       end