Merge branch '13251-fe-require-approval-from-code-owners' into 'master'

Frontend Implementation of Code Owner Approval

Closes #13251

See merge request gitlab-org/gitlab!15862

app/controllers/projects/protected_branches_controller.rb

@@ -19,9 +19,13 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
     [:merge_access_levels, :push_access_levels]
   end
 
-  def protected_ref_params
-    params.require(:protected_branch).permit(:name,
+  def protected_ref_params(*attrs)
+    attrs = ([:name,
               merge_access_levels_attributes: access_level_attributes,
-                                             push_access_levels_attributes: access_level_attributes)
+              push_access_levels_attributes: access_level_attributes] + attrs).uniq
+
+    params.require(:protected_branch).permit(attrs)
   end
 end
+
+Projects::ProtectedBranchesController.prepend_if_ee('EE::Projects::ProtectedBranchesController')

app/models/protected_branch.rb

@@ -40,6 +40,11 @@ class ProtectedBranch < ApplicationRecord
   def self.protected_refs(project)
     project.protected_branches.select(:name)
   end
+
+  def self.branch_requires_code_owner_approval?(project, branch_name)
+    # NOOP
+    #
+  end
 end
 
 ProtectedBranch.prepend_if_ee('EE::ProtectedBranch')

app/views/projects/protected_branches/shared/_branches_list.html.haml

 .protected-branches-list.js-protected-branches-list.qa-protected-branches-list
   - if @protected_branches.empty?
     .card-header.bg-white
-      Protected branch (#{@protected_branches_count})
+      = s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: @protected_branches_count }
     %p.settings-message.text-center
-      There are currently no protected branches, protect a branch with the form above.
+      = s_("ProtectedBranch|There are currently no protected branches, protect a branch with the form above.")
   - else
     %table.table.table-bordered
       %colgroup
@@ -15,10 +15,17 @@
           %col
       %thead
         %tr
-          %th Protected branch (#{@protected_branches_count})
-          %th Last commit
-          %th Allowed to merge
-          %th Allowed to push
+          %th
+            = s_("ProtectedBranch|Protected branch (%{protected_branches_count})") % { protected_branches_count: @protected_branches_count }
+          %th
+            = s_("ProtectedBranch|Last commit")
+          %th
+            = s_("ProtectedBranch|Allowed to merge")
+          %th
+            = s_("ProtectedBranch|Allowed to push")
+
+          = render_if_exists 'projects/protected_branches/ee/code_owner_approval_table_head'
+
           - if can_admin_project
             %th
       %tbody

app/views/projects/protected_branches/shared/_create_protected_branch.html.haml

@@ -2,7 +2,7 @@
   %input{ type: 'hidden', name: 'update_section', value: 'js-protected-branches-settings' }
   .card
     .card-header
-      Protect a branch
+      = s_("ProtectedBranch|Protect a branch")
     .card-body
       = form_errors(@protected_branch)
       .form-group.row
@@ -11,22 +11,19 @@
         .col-md-10
           = render partial: "projects/protected_branches/shared/dropdown", locals: { f: f }
           .form-text.text-muted
-            = link_to 'Wildcards', help_page_path('user/project/protected_branches', anchor: 'wildcard-protected-branches')
-            such as
-            %code *-stable
-            or
-            %code production/*
-            are supported
+            - wildcards_url = help_page_url('user/project/protected_branches', anchor: 'wildcard-protected-branches')
+            - wildcards_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: wildcards_url }
+            = (s_("ProtectedBranch|%{wildcards_link_start}Wildcards%{wildcards_link_end} such as %{code_tag_start}*-stable%{code_tag_end} or %{code_tag_start}production/*%{code_tag_end} are supported") % { wildcards_link_start: wildcards_link_start, wildcards_link_end: '</a>', code_tag_start: '<code>', code_tag_end: '</code>' }).html_safe
       .form-group.row
         %label.col-md-2.text-right{ for: 'merge_access_levels_attributes' }
-          Allowed to merge:
+          = s_("ProtectedBranch|Allowed to merge:")
         .col-md-10
           = yield :merge_access_levels
       .form-group.row
         %label.col-md-2.text-right{ for: 'push_access_levels_attributes' }
-          Allowed to push:
+          = s_("ProtectedBranch|Allowed to push:")
         .col-md-10
           = yield :push_access_levels
-
+      = render_if_exists 'projects/protected_branches/ee/code_owner_approval_form'
     .card-footer
-      = f.submit 'Protect', class: 'btn-success btn', disabled: true, data: { qa_selector: 'protect_button' }
+      = f.submit s_('ProtectedBranch|Protect'), class: 'btn-success btn', disabled: true, data: { qa_selector: 'protect_button' }

app/views/projects/protected_branches/shared/_protected_branch.html.haml

@@ -19,6 +19,8 @@
 
   = yield
 
+  = render_if_exists 'projects/protected_branches/ee/code_owner_approval_table', protected_branch: protected_branch
+
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch, { update_section: 'js-protected-branches-settings' }], disabled: local_assigns[:disabled], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning"

db/post_migrate/20190827102026_migrate_code_owner_approval_status_to_protected_branches_in_batches.rb

+# frozen_string_literal: true
+
+class MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+  BATCH_SIZE = 200
+
+  class Project < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'projects'
+    self.inheritance_column = :_type_disabled
+
+    has_many :protected_branches
+  end
+
+  class ProtectedBranch < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'protected_branches'
+    self.inheritance_column = :_type_disabled
+
+    belongs_to :project
+  end
+
+  def up
+    add_concurrent_index :projects, :id, name: "temp_active_projects_with_prot_branches", where: 'archived = false and pending_delete = false and merge_requests_require_code_owner_approval = true'
+
+    ProtectedBranch
+      .joins(:project)
+      .where(projects: { archived: false, pending_delete: false, merge_requests_require_code_owner_approval: true })
+      .each_batch(of: BATCH_SIZE) do |batch|
+        batch.update_all(code_owner_approval_required: true)
+      end
+
+    remove_concurrent_index_by_name(:projects, "temp_active_projects_with_prot_branches")
+  end
+
+  def down
+    # noop
+    #
+  end
+end

doc/api/protected_branches.md

@@ -296,3 +296,21 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" 'https://git
 | --------- | ---- | -------- | ----------- |
 | `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `name` | string | yes | The name of the branch |
+
+## Require code owner approvals for a single branch
+
+Update the "code owner approval required" option for the given protected branch protected branch.
+
+```
+PATCH /projects/:id/protected_branches/:name
+```
+
+```bash
+curl --request PATCH --header "PRIVATE-TOKEN: <your_access_token>" 'https://gitlab.example.com/api/v4/projects/5/protected_branches/feature-branch'
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
+| `name` | string | yes | The name of the branch |
+| `code_owner_approval_required`  | boolean        | no  | **(PREMIUM)** Prevent pushes to this branch if it matches an item in the [`CODEOWNERS` file](../user/project/code_owners.md). (defaults: false)|

doc/user/project/protected_branches.md

@@ -86,6 +86,20 @@ Click **Protect** and the branch will appear in the "Protected branch" list.
 
 ![Roles and users list](img/protected_branches_select_roles_and_users_list.png)
 
+## Code Owners approvals **(PREMIUM)**
+
+It is possible to require at least one approval for each entry in the
+[`CODEOWNERS` file](code_owners.md) that matches a file changed in
+the merge request. To enable this feature:
+
+1. Toggle the **Require approval from code owners** slider.
+
+1. Click **Protect**.
+
+When this feature is enabled, all merge requests need approval
+from one code owner per matched rule before they can be merged. Additionally,
+pushes to the protected branch are denied if a rule is matched.
+
 ## Wildcard protected branches
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/4665) in GitLab 8.10.

ee/app/assets/javascripts/protected_branches/protected_branch_create.js

@@ -14,13 +14,19 @@ export default class ProtectedBranchCreate {
     this.currentProjectUserDefaults = {};
     this.buildDropdowns();
     this.$branchInput = this.$form.find('input[name="protected_branch[name]"]');
+    this.$codeOwnerToggle = this.$form.find('.js-code-owner-toggle');
     this.bindEvents();
   }
 
   bindEvents() {
+    this.$codeOwnerToggle.on('click', this.onCodeOwnerToggleClick.bind(this));
     this.$form.on('submit', this.onFormSubmit.bind(this));
   }
 
+  onCodeOwnerToggleClick() {
+    this.$codeOwnerToggle.toggleClass('is-checked');
+  }
+
   buildDropdowns() {
     const $allowedToMergeDropdown = this.$form.find('.js-allowed-to-merge');
     const $allowedToPushDropdown = this.$form.find('.js-allowed-to-push');
@@ -75,6 +81,7 @@ export default class ProtectedBranchCreate {
       authenticity_token: this.$form.find('input[name="authenticity_token"]').val(),
       protected_branch: {
         name: this.$form.find('input[name="protected_branch[name]"]').val(),
+        code_owner_approval_required: this.$codeOwnerToggle.hasClass('is-checked'),
       },
     };
 

ee/app/assets/javascripts/protected_branches/protected_branch_edit.js

-import $ from 'jquery';
 import _ from 'underscore';
 import axios from '~/lib/utils/axios_utils';
 import Flash from '~/flash';
@@ -13,6 +12,7 @@ export default class ProtectedBranchEdit {
     this.$wrap = options.$wrap;
     this.$allowedToMergeDropdown = this.$wrap.find('.js-allowed-to-merge');
     this.$allowedToPushDropdown = this.$wrap.find('.js-allowed-to-push');
+    this.$codeOwnerToggle = this.$wrap.find('.js-code-owner-toggle');
 
     this.$wraps[ACCESS_LEVELS.MERGE] = this.$allowedToMergeDropdown.closest(
       `.${ACCESS_LEVELS.MERGE}-container`,
@@ -22,6 +22,35 @@ export default class ProtectedBranchEdit {
     );
 
     this.buildDropdowns();
+    this.bindEvents();
+  }
+
+  bindEvents() {
+    this.$codeOwnerToggle.on('click', this.onCodeOwnerToggleClick.bind(this));
+  }
+
+  onCodeOwnerToggleClick() {
+    this.$codeOwnerToggle.toggleClass('is-checked');
+    this.$codeOwnerToggle.prop('disabled', true);
+
+    const formData = {
+      code_owner_approval_required: this.$codeOwnerToggle.hasClass('is-checked'),
+    };
+
+    this.updateCodeOwnerApproval(formData);
+  }
+
+  updateCodeOwnerApproval(formData) {
+    axios
+      .patch(this.$wrap.data('url'), {
+        protected_branch: formData,
+      })
+      .then(() => {
+        this.$codeOwnerToggle.prop('disabled', false);
+      })
+      .catch(() => {
+        Flash(__('Failed to update branch!'));
+      });
   }
 
   buildDropdowns() {
@@ -85,7 +114,7 @@ export default class ProtectedBranchEdit {
       .catch(() => {
         this.$allowedToMergeDropdown.enable();
         this.$allowedToPushDropdown.enable();
-        Flash(__('Failed to update branch!'), null, $('.js-protected-branches-list'));
+        Flash(__('Failed to update branch!'));
       });
   }
 

ee/app/controllers/ee/projects/protected_branches_controller.rb

+# frozen_string_literal: true
+
+module EE
+  module Projects
+    module ProtectedBranchesController
+      extend ::Gitlab::Utils::Override
+
+      protected
+
+      override :protected_ref_params
+      def protected_ref_params(*attrs)
+        params_hash = super(:code_owner_approval_required)
+
+        params_hash[:code_owner_approval_required] = false unless project.code_owner_approval_required_available?
+
+        params_hash
+      end
+    end
+  end
+end

ee/app/models/approval_wrapped_rule.rb

@@ -92,16 +92,15 @@ class ApprovalWrappedRule
 
   def code_owner_approvals_required
     strong_memoize(:code_owner_approvals_required) do
-      next 0 unless merge_requests_require_code_owner_approval?
+      next 0 unless branch_requires_code_owner_approval?
 
       approvers.any? ? REQUIRED_APPROVALS_PER_CODE_OWNER_RULE : 0
     end
   end
 
-  def merge_requests_require_code_owner_approval?
+  def branch_requires_code_owner_approval?
     return false unless project.code_owner_approval_required_available?
 
-    project.merge_requests_require_code_owner_approval? ||
-      project.branch_requires_code_owner_approval?(merge_request.target_branch)
+    ProtectedBranch.branch_requires_code_owner_approval?(project, merge_request.target_branch)
   end
 end

ee/app/models/concerns/ee/protected_branch.rb

@@ -8,6 +8,14 @@ module EE
       protected_ref_access_levels :unprotect
     end
 
+    class_methods do
+      def branch_requires_code_owner_approval?(project, branch_name)
+        return false unless project.code_owner_approval_required_available?
+
+        project.protected_branches.requiring_code_owner_approval.matching(branch_name).any?
+      end
+    end
+
     def code_owner_approval_required
       super && project.code_owner_approval_required_available?
     end

ee/app/models/ee/project.rb

@@ -111,7 +111,7 @@ module EE
       scope :verification_failed_wikis, -> { joins(:repository_state).merge(ProjectRepositoryState.verification_failed_wikis) }
       scope :for_plan_name, -> (name) { joins(namespace: :plan).where(plans: { name: name }) }
       scope :requiring_code_owner_approval,
-            -> { where(merge_requests_require_code_owner_approval: true) }
+            -> { joins(:protected_branches).where(protected_branches: { code_owner_approval_required: true }) }
       scope :with_active_services, -> { joins(:services).merge(::Service.active) }
       scope :with_active_jira_services, -> { joins(:services).merge(::JiraService.active) }
       scope :with_jira_dvcs_cloud, -> { joins(:feature_usage).merge(ProjectFeatureUsage.with_jira_dvcs_integration_enabled(cloud: true)) }
@@ -409,13 +409,14 @@ module EE
     end
 
     def merge_requests_require_code_owner_approval?
-      super && code_owner_approval_required_available?
+      code_owner_approval_required_available? &&
+        protected_branches.requiring_code_owner_approval.any?
     end
 
     def branch_requires_code_owner_approval?(branch_name)
       return false unless code_owner_approval_required_available?
 
-      protected_branches.requiring_code_owner_approval.matching(branch_name).any?
+      ::ProtectedBranch.branch_requires_code_owner_approval?(self, branch_name)
     end
 
     def require_password_to_approve

ee/app/services/ee/protected_branches/create_service.rb

@@ -18,6 +18,8 @@ module EE
 
       override :save_protected_branch
       def save_protected_branch
+        protected_branch.code_owner_approval_required = false unless project.feature_available?(:code_owner_approval_required)
+
         super
 
         sync_code_owner_approval_rules if project.feature_available?(:code_owners)

ee/app/views/projects/_merge_request_approvals_settings_form.html.haml

@@ -11,14 +11,6 @@
     .text-center.prepend-top-default
       = sprite_icon('spinner', size: 24, css_class: 'gl-spinner')
 
-- if project.code_owner_approval_required_available?
-  .form-group.require-code-owner-approval
-    .form-check
-      = form.check_box(:merge_requests_require_code_owner_approval, class: 'form-check-input qa-require-code-owners-approval-checkbox')
-      = form.label :merge_requests_require_code_owner_approval, class: 'form-check-label' do
-        %span= _('Require approval from code owners')
-        = link_to icon('question-circle'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'editing-approvals-premium'), target: '_blank'
-
 .form-group
   .form-check
     = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input' }, false, true)

ee/app/views/projects/merge_requests/_code_owner_approval_rules.html.haml

-- return unless @project.merge_requests_require_code_owner_approval?
+- return unless ProtectedBranch.branch_requires_code_owner_approval?(@project, merge_request.target_branch)
 - code_owner_rules = merge_request.code_owner_rules_with_users
 - return unless code_owner_rules.any?
 

ee/app/views/projects/protected_branches/ee/_code_owner_approval_form.html.haml

+- if @project.feature_available?(:code_owner_approval_required)
+  .form-group.row
+    %label.col-md-2.text-right{ for: 'code_owner_approval_required' }
+      = s_("ProtectedBranch|Require approval from code owners:")
+    .col-md-10
+      %button{ type: 'button',
+        class: "js-code-owner-toggle project-feature-toggle is-checked",
+        "aria-label": s_("ProtectedBranch|Toggle code owner approval") }
+        %span.toggle-icon
+          = sprite_icon('status_success_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-checked')
+          = sprite_icon('status_failed_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-unchecked')
+      .form-text.text-muted
+        = s_("ProtectedBranch|Pushes that change filenames matched by the CODEOWNERS file will be rejected")

ee/app/views/projects/protected_branches/ee/_code_owner_approval_table.html.haml

+- if @project.feature_available?(:code_owner_approval_required)
+  %td
+    %button{ type: 'button',
+      class: "js-code-owner-toggle project-feature-toggle mr-5 #{'is-checked' if protected_branch.code_owner_approval_required}",
+      "aria-label": s_("ProtectedBranch|Toggle code owner approval") }
+      %span.toggle-icon
+        = sprite_icon('status_success_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-checked')
+        = sprite_icon('status_failed_borderless', size: 16, css_class: 'toggle-icon-svg toggle-status-unchecked')

ee/app/views/projects/protected_branches/ee/_code_owner_approval_table_head.html.haml

+- if @project.feature_available?(:code_owner_approval_required)
+  %th
+    = s_("ProtectedBranch|Code owner approval")

ee/changelogs/unreleased/13251-fe-require-approval-from-code-owners.yml

+---
+title: Add configurable Code Owner approvals for protected branches
+merge_request: 15862
+author:
+type: added

ee/lib/ee/api/protected_branches.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module ProtectedBranches
+      extend ActiveSupport::Concern
+
+      BRANCH_ENDPOINT_REQUIREMENTS = ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS.merge(name: ::API::API::NO_SLASH_URL_PART_REGEX)
+
+      prepended do
+        params do
+          requires :id, type: String, desc: 'The ID of a project'
+        end
+        resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
+          desc 'Update the code_owner_approval_required state of an existing protected branch' do
+            success ::API::Entities::ProtectedBranch
+          end
+          params do
+            requires :name, type: String, desc: 'The name of the branch'
+
+            use :optional_params_ee
+          end
+          # rubocop: disable CodeReuse/ActiveRecord
+          patch ':id/protected_branches/:name', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
+            render_api_error!("Feature 'Code Owner Approval Required' is not enabled", 403) unless user_project.code_owner_approval_required_available?
+
+            protected_branch = user_project.protected_branches.find_by!(name: params[:name])
+
+            protected_branch.update_attribute(:code_owner_approval_required, declared_params[:code_owner_approval_required])
+
+            present protected_branch, with: ::API::Entities::ProtectedBranch, project: user_project
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/ee/projects/protected_branches_controller_spec.rb

+# frozen_string_literal: true
+require "spec_helper"
+
+describe Projects::ProtectedBranchesController do
+  let(:project) { create(:project, :repository) }
+  let(:protected_branch) { create(:protected_branch, project: project) }
+  let(:project_params) { { namespace_id: project.namespace.to_param, project_id: project } }
+  let(:user) { create(:user) }
+
+  before do
+    project.add_maintainer(user)
+  end
+
+  shared_examples "protected branch with code owner approvals feature" do |boolean|
+    it "sets code owner approvals to #{boolean} when protecting the branch" do
+      expect do
+        post(:create, params: project_params.merge(protected_branch: create_params))
+      end.to change(ProtectedBranch, :count).by(1)
+
+      expect(ProtectedBranch.last.attributes["code_owner_approval_required"]).to eq(boolean)
+    end
+  end
+
+  describe "POST #create" do
+    let(:maintainer_access_level) { [{ access_level: Gitlab::Access::MAINTAINER }] }
+    let(:access_level_params) do
+      { merge_access_levels_attributes: maintainer_access_level,
+        push_access_levels_attributes: maintainer_access_level }
+    end
+    let(:create_params) do
+      attributes_for(:protected_branch).merge(access_level_params)
+    end
+
+    before do
+      sign_in(user)
+    end
+
+    context "when code_owner_approval_required is 'false'" do
+      before do
+        create_params[:code_owner_approval_required] = false
+      end
+
+      it_behaves_like "protected branch with code owner approvals feature", false
+    end
+
+    context "when code_owner_approval_required is 'true'" do
+      before do
+        create_params[:code_owner_approval_required] = true
+      end
+
+      context "when the feature is enabled" do
+        before do
+          stub_licensed_features(code_owner_approval_required: true)
+        end
+
+        it_behaves_like "protected branch with code owner approvals feature", true
+      end
+
+      context "when the feature is not enabled" do
+        before do
+          stub_licensed_features(code_owner_approval_required: false)
+        end
+
+        it_behaves_like "protected branch with code owner approvals feature", false
+      end
+    end
+  end
+end

ee/spec/features/merge_request/user_sees_approval_widget_spec.rb

@@ -109,7 +109,11 @@ describe 'Merge request > User sees approval widget', :js do
         context 'when code owner approval is required' do
           before do
             stub_licensed_features(code_owner_approval_required: true, multiple_approval_rules: true)
+
             project.update!(merge_requests_require_code_owner_approval: true)
+
+            allow(ProtectedBranch)
+              .to receive(:branch_requires_code_owner_approval?).and_return(true)
           end
 
           it 'shows the code owner rule as required' do

ee/spec/features/projects/merge_requests/user_edits_merge_request_spec.rb

@@ -18,7 +18,6 @@ describe 'Projects > Merge Requests > User edits a merge request' do
 
     let(:project) do
       create(:project, :custom_repo,
-             merge_requests_require_code_owner_approval: true,
              files: { 'docs/CODEOWNERS' => "*.rb @ruby-owner\n*.js @js-owner" })
     end
 
@@ -38,6 +37,11 @@ describe 'Projects > Merge Requests > User edits a merge request' do
                                      message: 'Add a ruby file',
                                      branch_name: 'feature')
 
+      create(:protected_branch,
+        name: 'master',
+        code_owner_approval_required: true,
+        project: project)
+
       # To make sure the rules are created for the merge request, the services
       # that do that aren't triggered from factories
       MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute

ee/spec/features/projects/settings/user_manages_approval_settings_spec.rb

@@ -34,24 +34,6 @@ describe 'EE > Projects > Settings > User manages approval rule settings' do
     end
   end
 
-  context 'when `code_owner_approval_required` is available' do
-    let(:licensed_features) { { code_owner_approval_required: true } }
-
-    it_behaves_like 'dirty submit form', [{ form: '#js-merge-request-approval-settings', input: '#project_merge_requests_author_approval' }]
-
-    it 'allows the user to enforce code owner approval' do
-      within('.require-code-owner-approval') do
-        check('Require approval from code owners')
-      end
-
-      within('.merge-request-approval-settings-form') do
-        click_on('Save changes')
-      end
-
-      expect(project.reload.merge_requests_require_code_owner_approval?).to be_truthy
-    end
-  end
-
   context 'when `code_owner_approval_required` is not available' do
     let(:licensed_features) { { code_owner_approval_required: false } }
 

ee/spec/features/protected_branches_spec.rb

@@ -12,6 +12,117 @@ describe 'Protected Branches', :js do
     sign_in(user)
   end
 
+  describe 'code owner approval' do
+    describe 'when project requires code owner approval' do
+      before do
+        stub_licensed_features(protected_refs_for_users: true, code_owner_approval_required: true)
+      end
+
+      describe 'protect a branch form' do
+        let!(:protected_branch) { create(:protected_branch, project: project) }
+        let(:container) { page.find('#new_protected_branch') }
+        let(:code_owner_toggle) { container.find('.js-code-owner-toggle') }
+        let(:branch_input) { container.find('.js-protected-branch-select') }
+        let(:allowed_to_merge_input) { container.find('.js-allowed-to-merge') }
+        let(:allowed_to_push) { container.find('.js-allowed-to-push') }
+
+        before do
+          visit project_settings_repository_path(project)
+        end
+
+        def fill_in_form(branch_name)
+          branch_input.click
+          click_on branch_name
+
+          allowed_to_merge_input.click
+          wait_for_requests
+          page.find('.dropdown.show').click_on 'No one'
+
+          allowed_to_push.click
+          wait_for_requests
+          page.find('.dropdown.show').click_on 'No one'
+        end
+
+        def submit_form
+          click_on 'Protect'
+          wait_for_requests
+        end
+
+        it 'has code owner toggle' do
+          expect(page).to have_content("Require approval from code owners")
+          expect(code_owner_toggle[:class]).to include("is-checked")
+        end
+
+        it 'can create new protected branch with code owner disabled' do
+          fill_in_form "with-codeowners"
+
+          code_owner_toggle.click
+          expect(code_owner_toggle[:class]).not_to include("is-checked")
+
+          submit_form
+
+          expect(project.protected_branches.find_by_name("with-codeowners").code_owner_approval_required).to be(false)
+        end
+
+        it 'can create new protected branch with code owner enabled' do
+          fill_in_form "with-codeowners"
+
+          expect(code_owner_toggle[:class]).to include("is-checked")
+
+          submit_form
+
+          expect(project.protected_branches.find_by_name("with-codeowners").code_owner_approval_required).to be(true)
+        end
+      end
+
+      describe 'protect branch table' do
+        context 'has a protected branch with code owner approval toggled on' do
+          let!(:protected_branch) { create(:protected_branch, project: project, code_owner_approval_required: true) }
+
+          before do
+            visit project_settings_repository_path(project)
+          end
+
+          it 'shows code owner approval toggle' do
+            expect(page).to have_content("Code owner approval")
+          end
+
+          it 'displays toggle on' do
+            expect(page).to have_css('.js-code-owner-toggle.is-checked')
+          end
+        end
+
+        context 'has a protected branch with code owner approval toggled off ' do
+          let!(:protected_branch) { create(:protected_branch, project: project, code_owner_approval_required: false) }
+
+          it 'displays toggle off' do
+            visit project_settings_repository_path(project)
+
+            page.within '.qa-protected-branches-list' do
+              expect(page).not_to have_css('.js-code-owner-toggle.is-checked')
+            end
+          end
+        end
+      end
+    end
+
+    describe 'when project does not require code owner approval' do
+      before do
+        stub_licensed_features(protected_refs_for_users: true, code_owner_approval_required: false)
+
+        visit project_settings_repository_path(project)
+      end
+
+      it 'does not have code owner approval in the form' do
+        expect(page).not_to have_content("Require approval from code owners")
+      end
+
+      it 'does not have code owner approval in the table' do
+        expect(page).not_to have_content("Code owner approval")
+      end
+    end
+  end
+
   describe 'access control' do
     describe 'with ref permissions for users enabled' do
       before do

ee/spec/frontend/protected_branches/protected_branch_edit_spec.js

+import $ from 'jquery';
+import MockAdapter from 'axios-mock-adapter';
+import ProtectedBranchEdit from 'ee/protected_branches/protected_branch_edit';
+import flash from '~/flash';
+import axios from '~/lib/utils/axios_utils';
+import { TEST_HOST } from 'helpers/test_constants';
+
+jest.mock('~/flash');
+
+const TEST_URL = `${TEST_HOST}/url`;
+const IS_CHECKED_CLASS = 'is-checked';
+
+describe('EE ProtectedBranchEdit', () => {
+  let mock;
+
+  beforeEach(() => {
+    setFixtures(`<div id="wrap" data-url="${TEST_URL}">
+      <button class="js-code-owner-toggle">Toggle</button>
+    </div>`);
+
+    jest.spyOn(ProtectedBranchEdit.prototype, 'buildDropdowns').mockImplementation();
+
+    mock = new MockAdapter(axios);
+  });
+
+  const findCodeOwnerToggle = () => document.querySelector('.js-code-owner-toggle');
+
+  const create = ({ isChecked = false }) => {
+    if (isChecked) {
+      findCodeOwnerToggle().classList.add(IS_CHECKED_CLASS);
+    }
+
+    return new ProtectedBranchEdit({ $wrap: $('#wrap') });
+  };
+
+  afterEach(() => {
+    mock.restore();
+  });
+
+  describe('when unchecked toggle button', () => {
+    let toggle;
+
+    beforeEach(() => {
+      create({ isChecked: false });
+
+      toggle = findCodeOwnerToggle();
+    });
+
+    it('is not changed', () => {
+      expect(toggle).not.toHaveClass(IS_CHECKED_CLASS);
+      expect(toggle).not.toBeDisabled();
+    });
+
+    describe('when clicked', () => {
+      beforeEach(() => {
+        mock
+          .onPatch(TEST_URL, { protected_branch: { code_owner_approval_required: true } })
+          .replyOnce(200, {});
+
+        toggle.click();
+      });
+
+      it('checks and disables button', () => {
+        expect(toggle).toHaveClass(IS_CHECKED_CLASS);
+        expect(toggle).toBeDisabled();
+      });
+
+      it('sends update to BE', () =>
+        axios.waitForAll().then(() => {
+          // Args are asserted in the `.onPatch` call
+          expect(mock.history.patch.length).toEqual(1);
+
+          expect(toggle).not.toBeDisabled();
+          expect(flash).not.toHaveBeenCalled();
+        }));
+    });
+
+    describe('when clikced and BE error', () => {
+      beforeEach(() => {
+        mock.onPatch(TEST_URL).replyOnce(500);
+        toggle.click();
+      });
+
+      it('flashes error', () =>
+        axios.waitForAll().then(() => {
+          expect(flash).toHaveBeenCalled();
+        }));
+    });
+  });
+});

ee/spec/lib/ee/gitlab/usage_data_spec.rb

@@ -95,7 +95,7 @@ describe Gitlab::UsageData do
             deploy_keys: 1,
             keys: 1,
             merge_requests: 1,
-            projects_enforcing_code_owner_approval: 1,
+            projects_enforcing_code_owner_approval: 0,
             projects_imported_from_github: 1,
             projects_with_repositories_enabled: 1,
             protected_branches: 1,

ee/spec/lib/gitlab/usage_data_spec.rb

@@ -194,9 +194,15 @@ describe Gitlab::UsageData do
 
   describe 'code owner approval required' do
     before do
-      create(:project, :archived, :requiring_code_owner_approval)
-      create(:project, :requiring_code_owner_approval, pending_delete: true)
-      create(:project, :requiring_code_owner_approval)
+      create(:protected_branch, code_owner_approval_required: true)
+
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: create(:project, :archived))
+
+      create(:protected_branch,
+        code_owner_approval_required: true,
+        project: create(:project, pending_delete: true))
     end
 
     it 'counts the projects actively requiring code owner approval' do

ee/spec/models/approval_wrapped_rule_spec.rb

@@ -196,23 +196,12 @@ describe ApprovalWrappedRule do
               .to receive(:code_owner_approval_required_available?).and_return(true)
           end
 
-          context "when no protected_branches require code owner approval" do
-            it 'returns the correct number of approvals' do
-              allow(subject.project)
-                .to receive(:merge_requests_require_code_owner_approval?).and_return(feature_enabled)
-              allow(subject.project)
-                .to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(false)
-
-              expect(subject.approvals_required).to eq(expected_required_approvals)
-            end
-          end
-
           context "when the project doesn't require code owner approval on all MRs" do
             it 'returns the expected number of approvals for protected_branches that do require approval' do
               allow(subject.project)
                 .to receive(:merge_requests_require_code_owner_approval?).and_return(false)
-              allow(subject.project)
-                .to receive(:branch_requires_code_owner_approval?).with(branch.name).and_return(feature_enabled)
+              allow(ProtectedBranch)
+                .to receive(:branch_requires_code_owner_approval?).with(subject.project, branch.name).and_return(feature_enabled)
 
               expect(subject.approvals_required).to eq(expected_required_approvals)
             end

ee/spec/models/project_spec.rb

@@ -45,11 +45,15 @@ describe Project do
 
   context 'scopes' do
     describe '.requiring_code_owner_approval' do
+      let!(:project) { create(:project) }
+      let!(:expected_project) { protected_branch_needing_approval.project }
+      let!(:protected_branch_needing_approval) { create(:protected_branch, code_owner_approval_required: true) }
+
       it 'only includes the right projects' do
-        create(:project)
-        expected_project = create(:project, merge_requests_require_code_owner_approval: true)
+        scoped_query_result = described_class.requiring_code_owner_approval
 
-        expect(described_class.requiring_code_owner_approval).to contain_exactly(expected_project)
+        expect(described_class.count).to eq(2)
+        expect(scoped_query_result).to contain_exactly(expected_project)
       end
     end
 
@@ -1016,13 +1020,17 @@ describe Project do
       true  | true  | true
       false | true  | false
       true  | false | false
-      true  | nil   | false
     end
 
     with_them do
       before do
         stub_licensed_features(code_owner_approval_required: feature_available)
-        project.merge_requests_require_code_owner_approval = feature_enabled
+
+        if feature_enabled
+          create(:protected_branch,
+            project: project,
+            code_owner_approval_required: true)
+        end
       end
 
       it 'requires code owner approval when needed' do

ee/spec/requests/api/protected_branches_spec.rb

@@ -82,6 +82,57 @@ describe API::ProtectedBranches do
     end
   end
 
+  describe "PATCH /projects/:id/protected_branches/:branch" do
+    let(:route) { "/projects/#{project.id}/protected_branches/#{branch_name}" }
+
+    context 'when authenticated as a maintainer' do
+      before do
+        project.add_maintainer(user)
+      end
+
+      context "when the feature is enabled" do
+        before do
+          stub_licensed_features(code_owner_approval_required: true)
+        end
+
+        it "updates the protected branch" do
+          expect do
+            patch api(route, user), params: { code_owner_approval_required: true }
+          end.to change { protected_branch.reload.code_owner_approval_required }
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(json_response['code_owner_approval_required']).to eq(true)
+        end
+      end
+
+      context "when the feature is disabled" do
+        before do
+          stub_licensed_features(code_owner_approval_required: false)
+        end
+
+        it "does not change the protected branch" do
+          expect do
+            patch api(route, user), params: { code_owner_approval_required: true }
+          end.not_to change { protected_branch.reload.code_owner_approval_required }
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context 'when authenticated as a guest' do
+      before do
+        project.add_guest(user)
+      end
+
+      it "returns a 403 response" do
+        patch api(route, user)
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+  end
+
   describe 'POST /projects/:id/protected_branches' do
     let(:branch_name) { 'new_branch' }
     let(:post_endpoint) { api("/projects/#{project.id}/protected_branches", user) }

ee/spec/services/ee/protected_branches/create_service_spec.rb

@@ -25,6 +25,41 @@ describe ProtectedBranches::CreateService do
       target_project.add_user(user, :developer)
     end
 
+    context "code_owner_approval_required" do
+      context "when unavailable" do
+        before do
+          stub_licensed_features(code_owner_approval_required: false)
+
+          params[:code_owner_approval_required] = true
+        end
+
+        it "ignores incoming params and sets code_owner_approval_required to false" do
+          expect { service.execute }.to change(ProtectedBranch, :count).by(1)
+          expect(ProtectedBranch.last.code_owner_approval_required).to be_falsy
+        end
+      end
+
+      context "when available" do
+        before do
+          stub_licensed_features(code_owner_approval_required: true)
+        end
+
+        it "sets code_owner_approval_required to true when param is true" do
+          params[:code_owner_approval_required] = true
+
+          expect { service.execute }.to change(ProtectedBranch, :count).by(1)
+          expect(ProtectedBranch.last.code_owner_approval_required).to be_truthy
+        end
+
+        it "sets code_owner_approval_required to false when param is false" do
+          params[:code_owner_approval_required] = false
+
+          expect { service.execute }.to change(ProtectedBranch, :count).by(1)
+          expect(ProtectedBranch.last.code_owner_approval_required).to be_falsy
+        end
+      end
+    end
+
     context "when there are open merge requests" do
       let!(:merge_request) do
         create(:merge_request,

ee/spec/views/shared/issuable/_approvals.html.haml_spec.rb

@@ -18,6 +18,7 @@ describe 'shared/issuable/_approvals.html.haml' do
     allow(MergeRequestApproverPresenter).to receive(:new).and_return(approver_presenter)
     assign(:project, project)
     assign(:target_project, project)
+    assign(:mr_presenter, merge_request.present(current_user: user))
   end
 
   context 'has no approvers' do

lib/api/protected_branches.rb

@@ -42,7 +42,7 @@ module API
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      desc 'Protect a single branch or wildcard' do
+      desc 'Protect a single branch' do
         success Entities::ProtectedBranch
       end
       params do
@@ -93,3 +93,5 @@ module API
     end
   end
 end
+
+API::ProtectedBranches.prepend_if_ee('EE::API::ProtectedBranches')

locale/gitlab.pot

@@ -12815,6 +12815,48 @@ msgstr ""
 msgid "Protected branches"
 msgstr ""
 
+msgid "ProtectedBranch|%{wildcards_link_start}Wildcards%{wildcards_link_end} such as %{code_tag_start}*-stable%{code_tag_end} or %{code_tag_start}production/*%{code_tag_end} are supported"
+msgstr ""
+
+msgid "ProtectedBranch|Allowed to merge"
+msgstr ""
+
+msgid "ProtectedBranch|Allowed to merge:"
+msgstr ""
+
+msgid "ProtectedBranch|Allowed to push"
+msgstr ""
+
+msgid "ProtectedBranch|Allowed to push:"
+msgstr ""
+
+msgid "ProtectedBranch|Code owner approval"
+msgstr ""
+
+msgid "ProtectedBranch|Last commit"
+msgstr ""
+
+msgid "ProtectedBranch|Protect"
+msgstr ""
+
+msgid "ProtectedBranch|Protect a branch"
+msgstr ""
+
+msgid "ProtectedBranch|Protected branch (%{protected_branches_count})"
+msgstr ""
+
+msgid "ProtectedBranch|Pushes that change filenames matched by the CODEOWNERS file will be rejected"
+msgstr ""
+
+msgid "ProtectedBranch|Require approval from code owners:"
+msgstr ""
+
+msgid "ProtectedBranch|There are currently no protected branches, protect a branch with the form above."
+msgstr ""
+
+msgid "ProtectedBranch|Toggle code owner approval"
+msgstr ""
+
 msgid "ProtectedEnvironment|%{environment_name} will be writable for developers. Are you sure?"
 msgstr ""
 
@@ -13471,9 +13513,6 @@ msgstr ""
 msgid "Require all users to accept Terms of Service and Privacy Policy when they access GitLab."
 msgstr ""
 
-msgid "Require approval from code owners"
-msgstr ""
-
 msgid "Require user password to approve"
 msgstr ""
 

qa/qa/ee.rb

@@ -115,7 +115,6 @@ module QA
           autoload :MirroringRepositories, 'qa/ee/page/project/settings/mirroring_repositories'
           autoload :Main, 'qa/ee/page/project/settings/main'
           autoload :MergeRequest, 'qa/ee/page/project/settings/merge_request'
-          autoload :MergeRequestApproval, 'qa/ee/page/project/settings/merge_request_approval'
           autoload :Repository, 'qa/ee/page/project/settings/repository'
           autoload :PushRules, 'qa/ee/page/project/settings/push_rules'
         end

qa/qa/ee/page/project/settings/merge_request_approval.rb

-# frozen_string_literal: true
-
-module QA
-  module EE
-    module Page
-      module Project
-        module Settings
-          class MergeRequestApproval < QA::Page::Base
-            view 'ee/app/views/projects/_merge_request_approvals_settings_form.html.haml' do
-              element :require_code_owners_approval_checkbox
-            end
-
-            view 'ee/app/views/projects/_merge_request_approvals_settings.html.haml' do
-              element :save_merge_request_approval_settings_button
-            end
-
-            def click_require_code_owners_approval_checkbox
-              check_element :require_code_owners_approval_checkbox
-            end
-
-            def click_save_merge_request_approval_button
-              click_element :save_merge_request_approval_settings_button
-            end
-          end
-        end
-      end
-    end
-  end
-end

spec/migrations/migrate_code_owner_approval_status_to_protected_branches_in_batches_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20190827102026_migrate_code_owner_approval_status_to_protected_branches_in_batches.rb')
+
+describe MigrateCodeOwnerApprovalStatusToProtectedBranchesInBatches, :migration do
+  let(:namespaces)         { table(:namespaces) }
+  let(:projects)           { table(:projects) }
+  let(:protected_branches) { table(:protected_branches) }
+
+  let(:namespace) do
+    namespaces.create!(
+      path: 'gitlab-instance-administrators',
+      name: 'GitLab Instance Administrators'
+    )
+  end
+
+  let(:project) do
+    projects.create!(
+      namespace_id: namespace.id,
+      name: 'GitLab Instance Administration'
+    )
+  end
+
+  let!(:protected_branch_1) do
+    protected_branches.create!(
+      name: "branch name",
+      project_id: project.id
+    )
+  end
+
+  describe '#up' do
+    context "when there's no projects needing approval" do
+      it "doesn't change any protected branch records" do
+        expect { migrate! }
+          .not_to change { ProtectedBranch.where(code_owner_approval_required: true).count }
+      end
+    end
+
+    context "when there's a project needing approval" do
+      let!(:project_needing_approval) do
+        projects.create!(
+          namespace_id: namespace.id,
+          name: 'GitLab Instance Administration',
+          merge_requests_require_code_owner_approval: true
+        )
+      end
+
+      let!(:protected_branch_2) do
+        protected_branches.create!(
+          name: "branch name",
+          project_id: project_needing_approval.id
+        )
+      end
+
+      it "changes N protected branch records" do
+        expect { migrate! }
+          .to change { ProtectedBranch.where(code_owner_approval_required: true).count }
+          .by(1)
+      end
+    end
+  end
+end