Moved Project#protected_branch? to ProtectedBranch, similar for tags

16 files changed, 94 insertions, 81 deletions

diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb
index 3fc85dc6b2b..a852b90c57e 100644
--- a/app/helpers/branches_helper.rb
+++ b/app/helpers/branches_helper.rb
@@ -1,6 +1,6 @@
 module BranchesHelper
   def can_remove_branch?(project, branch_name)
-    if project.protected_branch? branch_name
+    if ProtectedBranch.protected?(project, branch_name)
       false
     elsif branch_name == project.repository.root_ref
       false
diff --git a/app/helpers/tags_helper.rb b/app/helpers/tags_helper.rb
index 6672e3da348..31aaf9e5607 100644
--- a/app/helpers/tags_helper.rb
+++ b/app/helpers/tags_helper.rb
@@ -23,6 +23,6 @@ module TagsHelper
   end
 
   def protected_tag?(project, tag)
-    project.protected_tag?(tag.name)
+    ProtectedTag.protected?(project, tag.name)
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index cef8ad76b07..38eefad96b6 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -442,7 +442,7 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def can_remove_source_branch?(current_user)
-    !source_project.protected_branch?(source_branch) &&
+    !ProtectedBranch.protected?(source_project, source_branch) &&
       !source_project.root_ref?(source_branch) &&
       Ability.allowed?(current_user, :push_code, source_project) &&
       diff_head_commit == source_branch_head
diff --git a/app/models/project.rb b/app/models/project.rb
index 4175bfab0a9..5cc224b4440 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -883,20 +883,19 @@ class Project < ActiveRecord::Base
     "#{url}.git"
   end
 
-  # Check if current branch name is marked as protected in the system
-  #TODO: Move elsewhere
-  def protected_branch?(branch_name)
-    return true if empty_repo? && default_branch_protected?
 
-    @protected_branches ||= self.protected_branches.to_a
-    ProtectedBranch.matching(branch_name, protected_refs: @protected_branches).present?
+  def empty_and_default_branch_protected?
+    empty_repo? && default_branch_protected?
   end
 
-  #TODO: Move elsewhere
-  def protected_tag?(tag_name)
-    #TODO: Check if memoization necessary, find way to have it work elsewhere
-    @protected_tags ||= self.protected_tags.to_a
-    ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present?
+  #TODO: Check with if this is still needed, maybe because of `.select {` in ProtectedRefsMatcher
+  #Either with tests or by asking Tim
+  def protected_tags_array
+    @protected_tags_array ||= self.protected_tags.to_a
+  end
+
+  def protected_branches_array
+    @protected_branches_array ||= self.protected_branches.to_a
   end
 
   def user_can_push_to_empty_repo?(user)
@@ -1367,6 +1366,7 @@ class Project < ActiveRecord::Base
     "projects/#{id}/pushes_since_gc"
   end
 
+  #TODO: Move this and methods which depend upon it
   def default_branch_protected?
     current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_FULL ||
       current_application_settings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index a0dbcf80c3d..eca8d5e0f7d 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -10,4 +10,12 @@ class ProtectedBranch < ActiveRecord::Base
 
   accepts_nested_attributes_for :push_access_levels
   accepts_nested_attributes_for :merge_access_levels
+
+  # Check if branch name is marked as protected in the system
+  def self.protected?(project, ref_name)
+    return true if project.empty_and_default_branch_protected?
+
+    protected_refs = project.protected_branches_array
+    self.matching(ref_name, protected_refs: protected_refs).present?
+  end
 end
diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb
index 301fe2092e9..bca5522759d 100644
--- a/app/models/protected_tag.rb
+++ b/app/models/protected_tag.rb
@@ -7,4 +7,9 @@ class ProtectedTag < ActiveRecord::Base
   validates :push_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." }
 
   accepts_nested_attributes_for :push_access_levels
+
+  def self.protected?(project, ref_name)
+    protected_refs = project.protected_tags_array
+    self.matching(ref_name, protected_refs: protected_refs).present?
+  end
 end
diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb
index 11a045f4c31..38a113caec7 100644
--- a/app/services/delete_branch_service.rb
+++ b/app/services/delete_branch_service.rb
@@ -11,7 +11,7 @@ class DeleteBranchService < BaseService
       return error('Cannot remove HEAD branch', 405)
     end
 
-    if project.protected_branch?(branch_name)
+    if ProtectedBranch.protected?(project, branch_name)
       return error('Protected branch cant be removed', 405)
     end
 
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index bc7431c89a8..45411c779cc 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -127,7 +127,7 @@ class GitPushService < BaseService
     project.change_head(branch_name)
 
     # Set protection on the default branch if configured
-    if current_application_settings.default_branch_protection != PROTECTION_NONE && !@project.protected_branch?(@project.default_branch)
+    if current_application_settings.default_branch_protection != PROTECTION_NONE && !ProtectedBranch.protected?(@project, @project.default_branch)
 
       params = {
         name: @project.default_branch,
diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml
index 9eb610ba9c0..d84fa9e55c0 100644
--- a/app/views/projects/branches/_branch.html.haml
+++ b/app/views/projects/branches/_branch.html.haml
@@ -15,7 +15,7 @@
       %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" }
         merged
 
-    - if @project.protected_branch? branch.name
+    - if ProtectedBranch.protected?(@project, branch.name)
       %span.label.label-success
         protected
     .controls.hidden-xs<
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e000e3e33d0..0fe7eb864e4 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -184,7 +184,7 @@ module API
       end
 
       expose :protected do |repo_branch, options|
-        options[:project].protected_branch?(repo_branch.name)
+        ProtectedBranch.protected?(options[:project], repo_branch.name)
       end
 
       expose :developers_can_push do |repo_branch, options|
diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb
index 540d95f2d1f..6f574a41727 100644
--- a/lib/gitlab/checks/change_access.rb
+++ b/lib/gitlab/checks/change_access.rb
@@ -33,7 +33,7 @@ module Gitlab
       def protected_branch_checks
         return if skip_authorization
         return unless @branch_name
-        return unless project.protected_branch?(@branch_name)
+        return unless ProtectedBranch.protected?(project, @branch_name)
 
         if forced_push?
           return "You are not allowed to force push code to a protected branch on this project."
@@ -85,7 +85,7 @@ module Gitlab
       end
 
       def tag_protected?
-        project.protected_tag?(@tag_name)
+        ProtectedTag.protected?(project, @tag_name)
       end
 
       def push_checks
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 5a5a4ebd08b..5541a45e948 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -34,7 +34,7 @@ module Gitlab
     def can_push_tag?(ref)
       return false unless can_access_git?
 
-      if project.protected_tag?(ref)
+      if ProtectedTag.protected?(project, ref)
         project.protected_tags.matching_refs_accesible_to(ref, user)
       else
         user.can?(:push_code, project)
@@ -44,7 +44,7 @@ module Gitlab
     def can_push_to_branch?(ref)
       return false unless can_access_git?
 
-      if project.protected_branch?(ref)
+      if ProtectedBranch.protected?(project, ref)
         return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
 
         has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push)
@@ -58,7 +58,7 @@ module Gitlab
     def can_merge_to_branch?(ref)
       return false unless can_access_git?
 
-      if project.protected_branch?(ref)
+      if ProtectedBranch.protected?(project, ref)
         project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge)
       else
         user.can?(:push_code, project)
diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb
index cb2989c32df..8525422908b 100644
--- a/spec/lib/gitlab/checks/change_access_spec.rb
+++ b/spec/lib/gitlab/checks/change_access_spec.rb
@@ -99,7 +99,7 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
 
     context 'protected branches check' do
       before do
-        allow(project).to receive(:protected_branch?).with('master').and_return(true)
+        allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true)
       end
 
       it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 24e7c1b17d9..2f6614c133e 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -441,7 +441,7 @@ describe MergeRequest, models: true do
     end
 
     it "can't be removed when its a protected branch" do
-      allow(subject.source_project).to receive(:protected_branch?).and_return(true)
+      allow(ProtectedBranch).to receive(:protected?).and_return(true)
       expect(subject.can_remove_source_branch?(user)).to be_falsey
     end
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index cc06949974e..e6b23a1cc05 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1272,62 +1272,6 @@ describe Project, models: true do
     end
   end
 
-  describe '#protected_branch?' do
-    context 'existing project' do
-      let(:project) { create(:project, :repository) }
-
-      it 'returns true when the branch matches a protected branch via direct match' do
-        create(:protected_branch, project: project, name: "foo")
-
-        expect(project.protected_branch?('foo')).to eq(true)
-      end
-
-      it 'returns true when the branch matches a protected branch via wildcard match' do
-        create(:protected_branch, project: project, name: "production/*")
-
-        expect(project.protected_branch?('production/some-branch')).to eq(true)
-      end
-
-      it 'returns false when the branch does not match a protected branch via direct match' do
-        expect(project.protected_branch?('foo')).to eq(false)
-      end
-
-      it 'returns false when the branch does not match a protected branch via wildcard match' do
-        create(:protected_branch, project: project, name: "production/*")
-
-        expect(project.protected_branch?('staging/some-branch')).to eq(false)
-      end
-    end
-
-    context "new project" do
-      let(:project) { create(:empty_project) }
-
-      it 'returns false when default_protected_branch is unprotected' do
-        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
-
-        expect(project.protected_branch?('master')).to be false
-      end
-
-      it 'returns false when default_protected_branch lets developers push' do
-        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
-
-        expect(project.protected_branch?('master')).to be false
-      end
-
-      it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do
-        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
-
-        expect(project.protected_branch?('master')).to be true
-      end
-
-      it 'returns true when default_branch_protection is in full protection' do
-        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
-
-        expect(project.protected_branch?('master')).to be true
-      end
-    end
-  end
-
   describe '#user_can_push_to_empty_repo?' do
     let(:project) { create(:empty_project) }
     let(:user)    { create(:user) }
diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb
index 1c02f8bfc3f..179a443c43d 100644
--- a/spec/models/protected_branch_spec.rb
+++ b/spec/models/protected_branch_spec.rb
@@ -137,4 +137,60 @@ describe ProtectedBranch, models: true do
       end
     end
   end
+
+  describe '#protected?' do
+    context 'existing project' do
+      let(:project) { create(:project, :repository) }
+
+      it 'returns true when the branch matches a protected branch via direct match' do
+        create(:protected_branch, project: project, name: "foo")
+
+        expect(ProtectedBranch.protected?(project, 'foo')).to eq(true)
+      end
+
+      it 'returns true when the branch matches a protected branch via wildcard match' do
+        create(:protected_branch, project: project, name: "production/*")
+
+        expect(ProtectedBranch.protected?(project, 'production/some-branch')).to eq(true)
+      end
+
+      it 'returns false when the branch does not match a protected branch via direct match' do
+        expect(ProtectedBranch.protected?(project, 'foo')).to eq(false)
+      end
+
+      it 'returns false when the branch does not match a protected branch via wildcard match' do
+        create(:protected_branch, project: project, name: "production/*")
+
+        expect(ProtectedBranch.protected?(project, 'staging/some-branch')).to eq(false)
+      end
+    end
+
+    context "new project" do
+      let(:project) { create(:empty_project) }
+
+      it 'returns false when default_protected_branch is unprotected' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+
+        expect(ProtectedBranch.protected?(project, 'master')).to be false
+      end
+
+      it 'returns false when default_protected_branch lets developers push' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        expect(ProtectedBranch.protected?(project, 'master')).to be false
+      end
+
+      it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(ProtectedBranch.protected?(project, 'master')).to be true
+      end
+
+      it 'returns true when default_branch_protection is in full protection' do
+        stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL)
+
+        expect(ProtectedBranch.protected?(project, 'master')).to be true
+      end
+    end
+  end
 end