Merge branch 'refactor-after-create-default-branch' into 'master'

Refactor Project#after_create_default_branch

See merge request gitlab-org/gitlab-ce!24329

app/models/project.rb

@@ -1602,24 +1602,7 @@ class Project < ActiveRecord::Base
 
   # rubocop: disable CodeReuse/ServiceClass
   def after_create_default_branch
-    return unless default_branch
-
-    # Ensure HEAD points to the default branch in case it is not master
-    change_head(default_branch)
-
-    if Gitlab::CurrentSettings.default_branch_protection != Gitlab::Access::PROTECTION_NONE && !ProtectedBranch.protected?(self, default_branch)
-      params = {
-        name: default_branch,
-        push_access_levels_attributes: [{
-          access_level: Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MAINTAINER
-        }],
-        merge_access_levels_attributes: [{
-          access_level: Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MAINTAINER
-        }]
-      }
-
-      ProtectedBranches::CreateService.new(self, creator, params).execute(skip_authorization: true)
-    end
+    Projects::ProtectDefaultBranchService.new(self).execute
   end
   # rubocop: enable CodeReuse/ServiceClass
 

app/services/projects/protect_default_branch_service.rb

+# frozen_string_literal: true
+
+module Projects
+  # Service class that can be used to execute actions necessary after creating a
+  # default branch.
+  class ProtectDefaultBranchService
+    attr_reader :project, :default_branch_protection
+
+    # @param [Project] project
+    def initialize(project)
+      @project = project
+
+      @default_branch_protection = Gitlab::Access::BranchProtection
+        .new(Gitlab::CurrentSettings.default_branch_protection)
+    end
+
+    def execute
+      protect_default_branch if default_branch
+    end
+
+    def protect_default_branch
+      # Ensure HEAD points to the default branch in case it is not master
+      project.change_head(default_branch)
+
+      create_protected_branch if protect_branch?
+    end
+
+    def create_protected_branch
+      params = {
+        name: default_branch,
+        push_access_levels_attributes: [{ access_level: push_access_level }],
+        merge_access_levels_attributes: [{ access_level: merge_access_level }]
+      }
+
+      # The creator of the project is always allowed to create protected
+      # branches, so we skip the authorization check in this service class.
+      ProtectedBranches::CreateService
+        .new(project, project.creator, params)
+        .execute(skip_authorization: true)
+    end
+
+    def protect_branch?
+      default_branch_protection.any? &&
+        !ProtectedBranch.protected?(project, default_branch)
+    end
+
+    def default_branch
+      project.default_branch
+    end
+
+    def push_access_level
+      if default_branch_protection.developer_can_push?
+        Gitlab::Access::DEVELOPER
+      else
+        Gitlab::Access::MAINTAINER
+      end
+    end
+
+    def merge_access_level
+      if default_branch_protection.developer_can_merge?
+        Gitlab::Access::DEVELOPER
+      else
+        Gitlab::Access::MAINTAINER
+      end
+    end
+  end
+end

lib/gitlab/access/branch_protection.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Access
+    # A wrapper around Integer based branch protection levels.
+    #
+    # This wrapper can be used to work with branch protection levels without
+    # having to directly refer to the constants. For example, instead of this:
+    #
+    #     if access_level == Gitlab::Access::PROTECTION_DEV_CAN_PUSH
+    #       ...
+    #     end
+    #
+    # You can write this instead:
+    #
+    #     protection = BranchProtection.new(access_level)
+    #
+    #     if protection.developer_can_push?
+    #       ...
+    #     end
+    class BranchProtection
+      attr_reader :level
+
+      # @param [Integer] level The branch protection level as an Integer.
+      def initialize(level)
+        @level = level
+      end
+
+      def any?
+        level != PROTECTION_NONE
+      end
+
+      def developer_can_push?
+        level == PROTECTION_DEV_CAN_PUSH
+      end
+
+      def developer_can_merge?
+        level == PROTECTION_DEV_CAN_MERGE
+      end
+    end
+  end
+end

spec/lib/gitlab/access/branch_protection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Access::BranchProtection do
+  describe '#any?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:level, :result) do
+      Gitlab::Access::PROTECTION_NONE          | false
+      Gitlab::Access::PROTECTION_DEV_CAN_PUSH  | true
+      Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true
+      Gitlab::Access::PROTECTION_FULL          | true
+    end
+
+    with_them do
+      it { expect(described_class.new(level).any?).to eq(result) }
+    end
+  end
+
+  describe '#developer_can_push?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:level, :result) do
+      Gitlab::Access::PROTECTION_NONE          | false
+      Gitlab::Access::PROTECTION_DEV_CAN_PUSH  | true
+      Gitlab::Access::PROTECTION_DEV_CAN_MERGE | false
+      Gitlab::Access::PROTECTION_FULL          | false
+    end
+
+    with_them do
+      it do
+        expect(described_class.new(level).developer_can_push?).to eq(result)
+      end
+    end
+  end
+
+  describe '#developer_can_merge?' do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:level, :result) do
+      Gitlab::Access::PROTECTION_NONE          | false
+      Gitlab::Access::PROTECTION_DEV_CAN_PUSH  | false
+      Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true
+      Gitlab::Access::PROTECTION_FULL          | false
+    end
+
+    with_them do
+      it do
+        expect(described_class.new(level).developer_can_merge?).to eq(result)
+      end
+    end
+  end
+end

spec/services/projects/protect_default_branch_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ProtectDefaultBranchService do
+  let(:service) { described_class.new(project) }
+  let(:project) { instance_spy(Project) }
+
+  describe '#execute' do
+    before do
+      allow(service)
+        .to receive(:protect_default_branch)
+    end
+
+    context 'without a default branch' do
+      it 'does nothing' do
+        allow(service)
+          .to receive(:default_branch)
+          .and_return(nil)
+
+        service.execute
+
+        expect(service)
+          .not_to have_received(:protect_default_branch)
+      end
+    end
+
+    context 'with a default branch' do
+      it 'protects the default branch' do
+        allow(service)
+          .to receive(:default_branch)
+          .and_return('master')
+
+        service.execute
+
+        expect(service)
+          .to have_received(:protect_default_branch)
+      end
+    end
+  end
+
+  describe '#protect_default_branch' do
+    before do
+      allow(service)
+        .to receive(:default_branch)
+        .and_return('master')
+
+      allow(project)
+        .to receive(:change_head)
+        .with('master')
+
+      allow(service)
+        .to receive(:create_protected_branch)
+    end
+
+    context 'when branch protection is needed' do
+      before do
+        allow(service)
+          .to receive(:protect_branch?)
+          .and_return(true)
+
+        allow(service)
+          .to receive(:create_protected_branch)
+      end
+
+      it 'changes the HEAD of the project' do
+        service.protect_default_branch
+
+        expect(project)
+          .to have_received(:change_head)
+      end
+
+      it 'protects the default branch' do
+        service.protect_default_branch
+
+        expect(service)
+          .to have_received(:create_protected_branch)
+      end
+    end
+
+    context 'when branch protection is not needed' do
+      before do
+        allow(service)
+          .to receive(:protect_branch?)
+          .and_return(false)
+      end
+
+      it 'changes the HEAD of the project' do
+        service.protect_default_branch
+
+        expect(project)
+          .to have_received(:change_head)
+      end
+
+      it 'does not protect the default branch' do
+        service.protect_default_branch
+
+        expect(service)
+          .not_to have_received(:create_protected_branch)
+      end
+    end
+  end
+
+  describe '#create_protected_branch' do
+    it 'creates the protected branch' do
+      creator = instance_spy(User)
+      create_service = instance_spy(ProtectedBranches::CreateService)
+      access_level = Gitlab::Access::DEVELOPER
+      params = {
+        name: 'master',
+        push_access_levels_attributes: [{ access_level: access_level }],
+        merge_access_levels_attributes: [{ access_level: access_level }]
+      }
+
+      allow(project)
+        .to receive(:creator)
+        .and_return(creator)
+
+      allow(ProtectedBranches::CreateService)
+        .to receive(:new)
+        .with(project, creator, params)
+        .and_return(create_service)
+
+      allow(service)
+        .to receive(:push_access_level)
+        .and_return(access_level)
+
+      allow(service)
+        .to receive(:merge_access_level)
+        .and_return(access_level)
+
+      allow(service)
+        .to receive(:default_branch)
+        .and_return('master')
+
+      allow(create_service)
+        .to receive(:execute)
+        .with(skip_authorization: true)
+
+      service.create_protected_branch
+
+      expect(create_service)
+        .to have_received(:execute)
+    end
+  end
+
+  describe '#protect_branch?' do
+    context 'when default branch protection is disabled' do
+      it 'returns false' do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_NONE)
+
+        expect(service.protect_branch?).to eq(false)
+      end
+    end
+
+    context 'when default branch protection is enabled' do
+      before do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        allow(service)
+          .to receive(:default_branch)
+          .and_return('master')
+      end
+
+      it 'returns false if the branch is already protected' do
+        allow(ProtectedBranch)
+          .to receive(:protected?)
+          .with(project, 'master')
+          .and_return(true)
+
+        expect(service.protect_branch?).to eq(false)
+      end
+
+      it 'returns true if the branch is not yet protected' do
+        allow(ProtectedBranch)
+          .to receive(:protected?)
+          .with(project, 'master')
+          .and_return(false)
+
+        expect(service.protect_branch?).to eq(true)
+      end
+    end
+  end
+
+  describe '#default_branch' do
+    it 'returns the default branch of the project' do
+      allow(project)
+        .to receive(:default_branch)
+        .and_return('master')
+
+      expect(service.default_branch).to eq('master')
+    end
+  end
+
+  describe '#push_access_level' do
+    context 'when developers can push' do
+      it 'returns the DEVELOPER access level' do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        expect(service.push_access_level).to eq(Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    context 'when developers can not push' do
+      it 'returns the MAINTAINER access level' do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(service.push_access_level).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+  end
+
+  describe '#merge_access_level' do
+    context 'when developers can merge' do
+      it 'returns the DEVELOPER access level' do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE)
+
+        expect(service.merge_access_level).to eq(Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    context 'when developers can not merge' do
+      it 'returns the MAINTAINER access level' do
+        allow(Gitlab::CurrentSettings)
+          .to receive(:default_branch_protection)
+          .and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
+
+        expect(service.merge_access_level).to eq(Gitlab::Access::MAINTAINER)
+      end
+    end
+  end
+end