Extract ProtectedRef Concern

app/models/concerns/protected_ref.rb

@@ -0,0 +1,47 @@
+module ProtectedRef
+  extend ActiveSupport::Concern
+
+  included do
+    belongs_to :project
+    validates :name, presence: true
+    validates :project, presence: true
+
+    def self.matching_refs_accesible_to(ref, user, action: :push)
+      access_levels_for_ref(ref, action).any? do |access_level|
+        access_level.check_access(user)
+      end
+    end
+
+    def self.access_levels_for_ref(ref, action: :push)
+      self.matching(ref).map(&:"@#{action}_access_levels").flatten
+    end
+
+    private
+
+    def self.matching(ref_name, protected_refs: nil)
+      ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs)
+    end
+  end
+
+  def commit
+    project.commit(self.name)
+  end
+
+  def matching(refs)
+    ref_matcher.matching(refs)
+  end
+
+  def matches?(refs)
+    ref_matcher.matches?(refs)
+  end
+
+  def wildcard?
+    ref_matcher.wildcard?
+  end
+
+  private
+
+  def ref_matcher
+    @ref_matcher ||= ProtectedRefMatcher.new(self)
+  end
+end

app/models/project.rb

@@ -898,14 +898,14 @@ class Project < ActiveRecord::Base
     return true if empty_repo? && default_branch_protected?
 
     @protected_branches ||= self.protected_branches.to_a
-    ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present?
+    ProtectedBranch.matching(branch_name, protected_refs: @protected_branches).present?
   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_tags: @protected_tags).present?
+    ProtectedTag.matching(tag_name, protected_refs: @protected_tags).present?
   end
 
   def user_can_push_to_empty_repo?(user)

app/models/protected_branch.rb

@@ -1,9 +1,6 @@
 class ProtectedBranch < ActiveRecord::Base
   include Gitlab::ShellAdapter
-
-  belongs_to :project
-  validates :name, presence: true
-  validates :project, presence: true
+  include ProtectedRef
 
   has_many :merge_access_levels, dependent: :destroy
   has_many :push_access_levels, dependent: :destroy
@@ -13,30 +10,4 @@ class ProtectedBranch < ActiveRecord::Base
 
   accepts_nested_attributes_for :push_access_levels
   accepts_nested_attributes_for :merge_access_levels
-
-  def commit
-    project.commit(self.name)
-  end
-
-  def self.matching(branch_name, protected_branches: nil)
-    ProtectedRefMatcher.matching(ProtectedBranch, branch_name, protected_refs: protected_branches)
-  end
-
-  def matching(branches)
-    ref_matcher.matching(branches)
-  end
-
-  def matches?(branch_name)
-    ref_matcher.matches?(branch_name)
-  end
-
-  def wildcard?
-    ref_matcher.wildcard?
-  end
-
-  private
-
-  def ref_matcher
-    @ref_matcher ||= ProtectedRefMatcher.new(self)
-  end
 end

app/models/protected_tag.rb

@@ -1,39 +1,10 @@
 class ProtectedTag < ActiveRecord::Base
   include Gitlab::ShellAdapter
-
-  belongs_to :project
-  validates :name, presence: true
-  validates :project, presence: true
+  include ProtectedRef
 
   has_many :push_access_levels, dependent: :destroy
 
   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 commit
-    project.commit(self.name)
-  end
-
-  def self.matching(tag_name, protected_tags: nil)
-    ProtectedRefMatcher.matching(ProtectedTag, tag_name, protected_refs: protected_tags)
-  end
-
-  def matching(branches)
-    ref_matcher.matching(branches)
-  end
-
-  def matches?(tag_name)
-    ref_matcher.matches?(tag_name)
-  end
-
-  def wildcard?
-    ref_matcher.wildcard?
-  end
-
-  private
-
-  def ref_matcher
-    @ref_matcher ||= ProtectedRefMatcher.new(self)
-  end
 end

lib/api/entities.rb

@@ -189,13 +189,13 @@ module API
 
       expose :developers_can_push do |repo_branch, options|
         project = options[:project]
-        access_levels = project.protected_branches.matching(repo_branch.name).map(&:push_access_levels).flatten
+        access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :push)
         access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER }
       end
 
       expose :developers_can_merge do |repo_branch, options|
         project = options[:project]
-        access_levels = project.protected_branches.matching(repo_branch.name).map(&:merge_access_levels).flatten
+        access_levels = project.protected_branches.access_levels_for_ref(repo_branch.name, :merge)
         access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER }
       end
     end

lib/gitlab/user_access.rb

@@ -35,10 +35,7 @@ module Gitlab
       return false unless can_access_git?
 
       if project.protected_tag?(ref)
-        access_levels = project.protected_tags.matching(ref).map(&:push_access_levels).flatten
-        has_access = access_levels.any? { |access_level| access_level.check_access(user) }
-
-        has_access
+        project.protected_tags.matching_refs_accesible_to(ref, user)
       else
         user.can?(:push_code, project)
       end
@@ -50,8 +47,7 @@ module Gitlab
       if project.protected_branch?(ref)
         return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
 
-        access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten
-        has_access = access_levels.any? { |access_level| access_level.check_access(user) }
+        has_access = project.protected_branches.matching_refs_accesible_to(ref, user, action: :push)
 
         has_access || !project.repository.branch_exists?(ref) && can_merge_to_branch?(ref)
       else
@@ -63,8 +59,7 @@ module Gitlab
       return false unless can_access_git?
 
       if project.protected_branch?(ref)
-        access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten
-        access_levels.any? { |access_level| access_level.check_access(user) }
+        project.protected_branches.matching_refs_accesible_to(ref, user, action: :merge)
       else
         user.can?(:push_code, project)
       end

spec/models/protected_branch_spec.rb

@@ -113,8 +113,8 @@ describe ProtectedBranch, models: true do
         staging = build(:protected_branch, name: "staging")
 
         expect(ProtectedBranch.matching("production")).to be_empty
-        expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).to include(production)
-        expect(ProtectedBranch.matching("production", protected_branches: [production, staging])).not_to include(staging)
+        expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).to include(production)
+        expect(ProtectedBranch.matching("production", protected_refs: [production, staging])).not_to include(staging)
       end
     end
 
@@ -132,8 +132,8 @@ describe ProtectedBranch, models: true do
         staging = build(:protected_branch, name: "staging/*")
 
         expect(ProtectedBranch.matching("production/some-branch")).to be_empty
-        expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).to include(production)
-        expect(ProtectedBranch.matching("production/some-branch", protected_branches: [production, staging])).not_to include(staging)
+        expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).to include(production)
+        expect(ProtectedBranch.matching("production/some-branch", protected_refs: [production, staging])).not_to include(staging)
       end
     end
   end

spec/services/protected_tags/create_service_spec.rb

@@ -6,7 +6,6 @@ describe ProtectedTags::CreateService, services: true do
   let(:params) do
     {
       name: 'master',
-      merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }],
       push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }]
     }
   end
@@ -17,7 +16,6 @@ describe ProtectedTags::CreateService, services: true do
     it 'creates a new protected tag' do
       expect { service.execute }.to change(ProtectedTag, :count).by(1)
       expect(project.protected_tags.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
-      expect(project.protected_tags.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
     end
   end
 end