Remove approval_rules feature flag

Remove spec for haml rendering which are moved to vue

Remove unused overall_approer_groups

doc/user/project/merge_requests/merge_request_approvals.md

@@ -3,7 +3,7 @@
 > Introduced in [GitLab Enterprise Edition 7.12](https://about.gitlab.com/2015/06/22/gitlab-7-12-released/#merge-request-approvers-ee-only).
 
 NOTE: **Note:**
-If you are running a self-managed instance, the new interface shown on
+Prior to 12.0, if you are running a self-managed instance, the new interface shown on
 this page will not be available unless the feature flag
 `approval_rules` is enabled, which can be done from the Rails console by
 instance administrators.

ee/app/controllers/ee/projects/merge_requests_controller.rb

@@ -9,7 +9,6 @@ module EE
 
       prepended do
         before_action only: [:show] do
-          push_frontend_feature_flag(:approval_rules, merge_request.project, default_enabled: true)
           push_frontend_feature_flag(:visual_review_app, merge_request.project, default_enabled: false)
         end
 
@@ -70,13 +69,10 @@ module EE
       def render_approvals_json
         respond_to do |format|
           format.json do
-            entity = if ::Feature.enabled?(:approval_rules, merge_request.project, default_enabled: true)
-                       EE::API::Entities::ApprovalState.new(merge_request.approval_state, current_user: current_user)
-                     else
-                       EE::API::Entities::MergeRequestApprovals.new(merge_request, current_user: current_user)
-                     end
-
-            render json: entity
+            render json: EE::API::Entities::ApprovalState.new(
+              merge_request.approval_state,
+              current_user: current_user
+            )
           end
         end
       end

ee/app/models/ee/merge_request.rb

@@ -107,7 +107,6 @@ module EE
     end
 
     def validate_approval_rule_source
-      return if ::Feature.disabled?(:approval_rules, project, default_enabled: true)
       return unless approval_rules.any?
 
       local_project_rule_ids = approval_rules.map { |rule| rule.approval_merge_request_rule_source&.approval_project_rule_id }
@@ -126,16 +125,7 @@ module EE
       strong_memoize(:participant_approvers) do
         next [] unless approval_needed?
 
-        if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
         approval_state.filtered_approvers(code_owner: false, unactioned: true)
-        else
-          approvers = [
-            *overall_approvers(exclude_code_owners: true),
-            *approvers_from_groups
-          ]
-
-          ::User.where(id: approvers.map(&:id)).where.not(id: approved_by_users.select(:id))
-        end
       end
     end
 

ee/app/models/ee/project.rb

@@ -324,8 +324,6 @@ module EE
     end
 
     def visible_regular_approval_rules
-      return approval_rules.none unless ::Feature.enabled?(:approval_rules, self, default_enabled: true)
-
       strong_memoize(:visible_regular_approval_rules) do
         regular_rules = approval_rules.regular.order(:id)
 

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -40,30 +40,11 @@ module EE
       def update_approvers
         return yield unless project.feature_available?(:code_owners)
 
-        if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
         results = yield
 
         merge_requests_for_source_branch.each do |merge_request|
           ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
         end
-        else
-          previous_diffs = fetch_latest_merge_request_diffs
-
-          results = yield
-
-          merge_requests = merge_requests_for_source_branch
-          ActiveRecord::Associations::Preloader.new.preload(merge_requests, :latest_merge_request_diff) # rubocop: disable CodeReuse/ActiveRecord)
-
-          merge_requests.each do |merge_request|
-            previous_diff = previous_diffs.find { |diff| diff.merge_request == merge_request }
-            previous_code_owners = ::Gitlab::CodeOwners.for_merge_request(merge_request, merge_request_diff: previous_diff)
-            new_code_owners = merge_request.code_owners - previous_code_owners
-
-            create_approvers(merge_request, new_code_owners)
-
-            merge_request.sync_code_owners_with_approvers
-          end
-        end
 
         results
       end

ee/app/services/ee/merge_requests/update_service.rb

@@ -15,7 +15,6 @@ module EE
         end
 
         merge_request = super(merge_request)
-        sync_approval_rules(merge_request)
 
         if should_remove_old_approvers && merge_request.valid?
           cleanup_approvers(merge_request, reload: true)
@@ -49,16 +48,6 @@ module EE
 
         merge_request.approvals.delete_all if target_project.reset_approvals_on_push
       end
-
-      # TODO remove after #1979 is closed
-      def sync_approval_rules(merge_request)
-        return if ::Feature.enabled?(:approval_rules, merge_request.target_project, default_enabled: true)
-        return if merge_request.merged?
-        return unless merge_request.previous_changes.include?(:approvals_before_merge)
-
-        approvals_required = merge_request.approvals_before_merge || merge_request.target_project.approvals_before_merge
-        merge_request.approval_rules.regular.update_all(approvals_required: approvals_required)
-      end
     end
   end
 end

ee/app/services/ee/projects/update_service.rb

@@ -35,8 +35,6 @@ module EE
           sync_wiki_on_enable if !wiki_was_enabled && project.wiki_enabled?
           project.import_state.force_import_job! if params[:mirror].present? && project.mirror?
           project.remove_import_data if project.previous_changes.include?('mirror') && !project.mirror?
-
-          sync_approval_rules
         end
 
         result
@@ -67,14 +65,6 @@ module EE
       def sync_wiki_on_enable
         ::Geo::RepositoryUpdatedService.new(project.wiki.repository).execute
       end
-
-      # TODO remove after #1979 is closed
-      def sync_approval_rules
-        return if ::Feature.enabled?(:approval_rules, project, default_enabled: true)
-        return unless project.previous_changes.include?(:approvals_before_merge)
-
-        project.approval_rules.update_all(approvals_required: project.approvals_before_merge)
-      end
     end
   end
 end

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

 - can_override_approvers = project.can_override_approvers?
 
-- if Feature.enabled?(:approval_rules, project, default_enabled: true)
-  = render 'shared/merge_request_approvals_settings/multiple_rules_form', form: form, project: project
-- else
-  = render 'shared/merge_request_approvals_settings/single_rule_form', form: form, project: project
+= render 'shared/merge_request_approvals_settings/multiple_rules_form', form: form, project: project
 
 - if project.code_owner_approval_required_available?
   .form-group.require-code-owner-approval

ee/app/views/shared/issuable/_approvals.html.haml

@@ -11,10 +11,7 @@
   = form.label :approver_ids, class: 'col-form-label col-sm-2' do
     Approvers
   .col-sm-10
-    - if Feature.enabled?(:approval_rules, @target_project, default_enabled: true)
     = render 'shared/issuable/approvals_multiple_rule', issuable: issuable, presenter: presenter
-    - else
-      = render 'shared/issuable/approvals_single_rule', issuable: issuable, presenter: presenter, form: form
     - if can_update_approvers
       - approver_presenter = MergeRequestApproverPresenter.new(issuable, skip_user: current_user)
       .form-text.text-muted.suggested-approvers

ee/app/views/shared/issuable/_approvals_single_rule.html.haml

-- issuable = local_assigns.fetch(:issuable)
-- presenter = local_assigns.fetch(:presenter)
-- form = local_assigns.fetch(:form)
-
-- can_update_approvers = can?(current_user, :update_approvers, issuable)
-
-- if can_update_approvers
-  - skip_users = [*presenter.all_approvers_including_groups, (issuable.author unless presenter.authors_can_approve?), *(issuable.committers unless presenter.committers_can_approve?)].compact
-
-  = users_select_tag("merge_request[approver_ids]",
-    multiple: true,
-    class: 'input-large',
-    email_user: true,
-    skip_users: skip_users,
-    project: issuable.target_project)
-
-  .form-text.text-muted
-    = _('This merge request must be approved by these users. You can override the project settings by setting your own list of approvers.')
-
-  - skip_groups = presenter.overall_approver_groups.pluck(:group_id) # rubocop: disable CodeReuse/ActiveRecord
-  = groups_select_tag('merge_request[approver_group_ids]', multiple: true, data: { skip_groups: skip_groups, all_available: true, project: issuable.target_project }, class: 'input-large')
-  .form-text.text-muted
-    = _('This merge request must be approved by members of these groups. You can override the project settings by setting your own list of approvers.')
-
-.card.prepend-top-10
-  .card-header
-    = _('Approvers')
-  %ul.content-list.approver-list.qa-approver-list
-    - if presenter.all_approvers_including_groups.empty?
-      %li.no-approvers= _('There are no approvers')
-    - else
-      - unsaved_approvers = !presenter.approvers_overwritten?
-      - item_classes = unsaved_approvers ? ['unsaved-approvers'] : []
-      - presenter.overall_approvers.each do |approver|
-        %li{ id: dom_id(approver), class: item_classes + ['approver'] }
-          = link_to approver.name, approver, class: 'qa-approver'
-          - if can_update_approvers
-            .float-right
-              - if unsaved_approvers
-                %button{ class: 'btn-sm btn btn-remove', title: _('Remove approver'), data: { confirm: _("Are you sure you want to remove approver %{name}") % { name: approver.name } } }
-                  = icon("sign-out")
-                  = _('Remove')
-              - else
-                = link_to project_merge_request_approver_via_user_id_path(@project, issuable, user_id: approver.id), data: { confirm: _("Are you sure you want to remove approver %{name}") % { name: approver.name } }, method: :delete, class: "btn-sm btn btn-remove", title: _('Remove approver') do
-                  = icon("sign-out")
-                  = _('Remove')
-      - presenter.overall_approver_groups.each do |approver_group|
-        %li{ id: dom_id(approver_group.group), class: item_classes + ['approver-group'] }
-          = _('Group:')
-          = link_to approver_group.group.name, approver_group.group
-          - if can_update_approvers
-            .float-right
-              - if unsaved_approvers
-                %button{ class: "btn-sm btn btn-remove", title: _('Remove group'), data: { confirm: _("Are you sure you want to remove group %{name}") % { name: approver_group.group.name } } }
-                  = icon("sign-out")
-                  = _('Remove')
-              - else
-                = link_to project_merge_request_approver_group_path(@project, issuable, approver_group), data: { confirm: _("Are you sure you want to remove group %{name}") % { name: approver_group.group.name } }, method: :delete, class: "btn-sm btn btn-remove", title: _('Remove group') do
-                  = icon("sign-out")
-                  = _('Remove')
-
-.col-sm-12
-  .form-group.row
-    = form.label :approvals_before_merge, class: 'label-bold' do
-      = _('Approvals required')
-    = form.number_field :approvals_before_merge, class: 'form-control', value: issuable.approvals_required, readonly: !can_update_approvers

ee/app/views/shared/merge_request_approvals_settings/_single_rule_form.html.haml

-.form-group
-  = form.label :approver_ids, class: 'label-bold' do
-    = _("Add approvers")
-  = hidden_field_tag "project[approver_ids]"
-  = hidden_field_tag "project[approver_group_ids]"
-  .row
-    .col-md-9
-      .input-group.input-btn-group
-        = hidden_field_tag :approver_user_and_group_ids, '', { class: 'js-select-user-and-group input-large', tabindex: 1, 'data-name': 'project', :style => "max-width: unset;" }
-        %button.btn.btn-success.js-add-approvers.js-dirty-submit{ type: 'button', title: _('Add approver(s)') }
-          = _("Add")
-      .form-text.text-muted
-        = _("Add users or groups who are allowed to approve every merge request")
-
-  .card.prepend-top-10.js-current-approvers
-    .load-wrapper.hidden
-      = icon('spinner spin', class: 'approver-list-loader')
-    .card-header
-      = _("Approvers")
-      %span.badge.badge-pill
-        - ids = []
-        - project.approvers.each do |user|
-          - ids << user.user_id
-        - project.approver_groups.each do |group|
-          - group.users.each do |user|
-            - unless ids.include?(user.id)
-              - ids << user.id
-        = ids.count
-    %ul.content-list.approver-list
-      - project.approvers.each do |approver|
-        %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
-          = link_to approver.user.name, approver.user
-          .float-right
-            - confirm_message = _("Are you sure you want to remove approver %{name}?") % { name: approver.user.name }
-            %button{ href: project_approver_path(project, approver), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove approver") }
-              = icon("trash")
-      - project.approver_groups.each do |approver_group|
-        %li.approver-group.settings-flex-row.js-approver-group{ data: { id: approver_group.group.id } }
-          %span
-            %span.light
-              = _("Group:")
-            = link_to approver_group.group.name, approver_group.group
-            %span.badge.badge-pill
-              = approver_group.group.members.count
-          .float-right
-            - confirm_message = _("Are you sure you want to remove group %{name}?") % { name: approver_group.group.name }
-            %button{ href: project_approver_group_path(project, approver_group), data: { confirm: confirm_message }, class: "btn btn-remove js-approver-remove", title: _("Remove group") }
-              = icon("trash")
-      - if project.approvers.empty? && project.approver_groups.empty?
-        %li= _("There are no approvers")
-
-.form-group
-  = form.label :approvals_before_merge, class: 'label-bold' do
-    = _("Approvals required")
-  = form.number_field :approvals_before_merge, class: "form-control w-auto", min: 0
-  .form-text.text-muted
-    = _("Set number of approvers required before open merge requests can be merged")

ee/changelogs/unreleased/10685-remove-feature-flag.yml

+---
+title: Remove old approver system in favor of new approval rule system
+merge_request: 12436
+author:
+type: removed

ee/lib/api/helpers/approval_helpers.rb

@@ -4,11 +4,7 @@ module API
   module Helpers
     module ApprovalHelpers
       def present_approval(merge_request)
-        if Feature.enabled?(:approval_rules, merge_request.project, default_enabled: true)
         present merge_request.approval_state, with: ::EE::API::Entities::ApprovalState, current_user: current_user
-        else
-          present merge_request.present(current_user: current_user), with: ::EE::API::Entities::MergeRequestApprovals, current_user: current_user
-        end
       end
     end
   end

ee/lib/api/merge_request_approvals.rb

@@ -54,8 +54,6 @@ module API
           hidden: true
         }
         get 'approval_settings' do
-          not_found! unless ::Feature.enabled?(:approval_rules, user_project, default_enabled: true)
-
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
           present merge_request.approval_state, with: ::EE::API::Entities::MergeRequestApprovalRules, current_user: current_user

ee/lib/api/project_approval_rules.rb

@@ -3,7 +3,6 @@
 module API
   class ProjectApprovalRules < ::Grape::API
     before { authenticate! }
-    before { not_found! unless ::Feature.enabled?(:approval_rules, user_project, default_enabled: true) }
 
     ARRAY_COERCION_LAMBDA = ->(val) { val.empty? ? [] : Array.wrap(val) }
 

ee/spec/controllers/projects/merge_requests_controller_spec.rb

@@ -5,12 +5,12 @@ shared_examples 'approvals' do
     JSON.parse(response.body)
   end
 
-  let!(:approver) { create(:approver, target: project) }
-  let!(:user_approver) { create(:approver, target: project, user: user) }
+  let!(:approver) { create(:user) }
+  let!(:approval_rule) { create(:approval_project_rule, project: project, users: [approver, user], approvals_required: 2) }
 
   before do
-    merge_request.update_attribute :approvals_before_merge, 2
-    project.add_developer(approver.user)
+    # merge_request.update_attribute :approvals_before_merge, 2
+    project.add_developer(approver)
   end
 
   describe 'approve' do
@@ -34,12 +34,12 @@ shared_examples 'approvals' do
       expect(approvals['user_has_approved']).to be true
       expect(approvals['user_can_approve']).to be false
       expect(approvals['suggested_approvers'].size).to eq 1
-      expect(approvals['suggested_approvers'][0]['username']).to eq approver.user.username
+      expect(approvals['suggested_approvers'][0]['username']).to eq approver.username
     end
   end
 
   describe 'approvals' do
-    let!(:approval) { create(:approval, merge_request: merge_request, user: approver.user) }
+    let!(:approval) { create(:approval, merge_request: merge_request, user: approver) }
 
     def get_approvals
       get :approvals,
@@ -59,29 +59,12 @@ shared_examples 'approvals' do
       expect(response).to be_success
       expect(approvals['approvals_left']).to eq 1
       expect(approvals['approved_by'].size).to eq 1
-      expect(approvals['approved_by'][0]['user']['username']).to eq approver.user.username
+      expect(approvals['approved_by'][0]['user']['username']).to eq approver.username
       expect(approvals['user_has_approved']).to be false
       expect(approvals['user_can_approve']).to be true
       expect(approvals['suggested_approvers'].size).to eq 1
       expect(approvals['suggested_approvers'][0]['username']).to eq user.username
     end
-
-    context 'with unauthorized group' do
-      let(:private_group) { create(:group_with_members, :private) }
-
-      before do
-        create(:approver_group, target: merge_request, group: private_group)
-      end
-
-      it 'does not expose approvers from a private group the current user has no access to' do
-        get_approvals
-
-        approvals = json_response
-
-        expect(response).to be_success
-        expect(approvals['suggested_approvers'].size).to eq(0)
-      end
-    end
   end
 
   describe 'unapprove' do
@@ -119,7 +102,6 @@ describe Projects::MergeRequestsController do
   let(:viewer)        { user }
 
   before do
-    stub_feature_flags(approval_rules: false)
     sign_in(viewer)
   end
 

ee/spec/features/merge_request/user_creates_merge_request_spec.rb

@@ -46,17 +46,15 @@ describe "User creates a merge request", :js do
     end
 
     # TODO: Fix https://gitlab.com/gitlab-org/gitlab-ee/issues/11527
-=begin
-    page.within(".suggested-approvers") do
-      expect(page).to have_content(user2.name)
-    end
-
-    click_link(user2.name)
-
-    page.within("ul.approver-list") do
-      expect(page).to have_content(user2.name)
-  end
-=end
+    # page.within(".suggested-approvers") do
+    #   expect(page).to have_content(user2.name)
+    # end
+    #
+    # click_link(user2.name)
+    #
+    # page.within("ul.approver-list") do
+    #   expect(page).to have_content(user2.name)
+    # end
 
     fill_in("Title", with: title)
     click_button("Submit merge request")
@@ -65,10 +63,8 @@ describe "User creates a merge request", :js do
       click_link("Edit", match: :first)
     end
 
-=begin
-    page.within("ul.approver-list") do
-      expect(page).to have_content(user2.name)
-    end
-=end
+    # page.within("ul.approver-list") do
+    #   expect(page).to have_content(user2.name)
+    # end
   end
 end

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -2,37 +2,13 @@ require 'rails_helper'
 
 describe 'Merge request > User sets approvers', :js do
   include ProjectForksHelper
+  include FeatureApprovalHelper
 
   let(:user) { create(:user) }
   let(:project) { create(:project, :public, :repository, approvals_before_merge: 1) }
   let!(:config_selector) { '.js-approval-rules' }
   let!(:modal_selector) { '#mr-edit-approvals-create-modal' }
 
-  def open_modal
-    page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
-    within(config_selector) do
-      click_on('Edit')
-    end
-  end
-
-  def open_approver_select
-    within(modal_selector) do
-      find('.select2-input').click
-    end
-    wait_for_requests
-  end
-
-  def close_approver_select
-    within(modal_selector) do
-      find('.select2-input').send_keys :escape
-    end
-  end
-
-  def remove_approver(name)
-    el = page.find("#{modal_selector} .content-list li", text: /#{name}/i)
-    el.find('button').click
-  end
-
   context 'when editing an MR with a different author' do
     let(:author) { create(:user) }
     let(:merge_request) { create(:merge_request, author: author, source_project: project) }
@@ -72,7 +48,6 @@ describe 'Merge request > User sets approvers', :js do
       open_approver_select
 
       expect(find('.select2-results')).to have_content(other_user.name)
-      # expect(find('.select2-results')).not_to have_content(user.name) # TODO
       expect(find('.select2-results')).not_to have_content(non_member.name)
     end
   end
@@ -111,7 +86,7 @@ describe 'Merge request > User sets approvers', :js do
         click_button 'Update approvers'
 
         click_on("Submit merge request")
-        wait_for_requests
+        wait_for_all_requests
 
         expect(page).to have_content("Requires approval.")
         expect(page).to have_selector("img[alt='#{other_user.name}']")
@@ -136,13 +111,12 @@ describe 'Merge request > User sets approvers', :js do
 
         click_button 'Update approvers'
         click_on("Submit merge request")
-        wait_for_requests
+        wait_for_all_requests
         click_on("View eligible approvers") if page.has_button?("View eligible approvers")
         wait_for_requests
 
         expect(page).not_to have_selector(".js-approvers img[alt='#{other_user.name}']")
         expect(page).to have_selector(".js-approvers img[alt='#{approver.name}']")
-        expect(page).to have_content("Requires approval.")
       end
     end
 
@@ -179,7 +153,7 @@ describe 'Merge request > User sets approvers', :js do
         click_button 'Update approvers'
 
         click_on("Save changes")
-        wait_for_requests
+        wait_for_all_requests
 
         expect(page).to have_content("Requires approval.")
         expect(page).to have_selector("img[alt='#{other_user.name}']")
@@ -205,7 +179,7 @@ describe 'Merge request > User sets approvers', :js do
 
         click_button 'Update approvers'
         click_on("Save changes")
-        wait_for_requests
+        wait_for_all_requests
 
         click_on("View eligible approvers")
         wait_for_requests
@@ -235,7 +209,7 @@ describe 'Merge request > User sets approvers', :js do
 
         click_button 'Update approvers'
         click_on('Save changes')
-        wait_for_requests
+        wait_for_all_requests
 
         # new MR setting on the show MR page
         expect(page).to have_content("Requires 3 more approvals")

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

@@ -2,6 +2,7 @@ require 'spec_helper'
 
 describe 'Project settings > [EE] Merge Requests', :js do
   include GitlabRoutingHelper
+  include FeatureApprovalHelper
 
   let(:user) { create(:user) }
   let(:project) { create(:project, approvals_before_merge: 1) }
@@ -11,45 +12,6 @@ describe 'Project settings > [EE] Merge Requests', :js do
   let!(:config_selector) { '.js-approval-rules' }
   let!(:modal_selector) { '#project-settings-approvals-create-modal' }
 
-  def open_modal
-    page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
-    within(config_selector) do
-      click_on('Edit')
-    end
-  end
-
-  def open_approver_select
-    within(modal_selector) do
-      find('.select2-input').click
-    end
-    wait_for_requests
-  end
-
-  def close_approver_select
-    within(modal_selector) do
-      find('.select2-input').send_keys :escape
-    end
-  end
-
-  def remove_approver(name)
-    el = page.find("#{modal_selector} .content-list li", text: /#{name}/i)
-    el.find('button').click
-  end
-
-  def expect_avatar(container, users)
-    users = Array(users)
-
-    members = container.all('.js-members img.avatar').map do |member|
-      member['alt']
-    end
-
-    users.each do |user|
-      expect(members).to include(user.name)
-    end
-
-    expect(members.size).to eq(users.size)
-  end
-
   before do
     sign_in(user)
     project.add_maintainer(user)

ee/spec/javascripts/approvals/setup_single_rule_approvals_spec.js

-import $ from 'jquery';
-import setup from 'ee/approvals/setup_single_rule_approvals';
-
-describe('EE setup_single_rule_approvals', () => {
-  preloadFixtures('merge_requests_ee/merge_request_edit.html');
-
-  let $approversEl;
-  let $suggestionEl;
-
-  beforeEach(() => {
-    loadFixtures('merge_requests_ee/merge_request_edit.html');
-    $approversEl = $('ul.approver-list');
-    $suggestionEl = $('.suggested-approvers');
-    setup();
-  });
-
-  describe('add suggested approver', () => {
-    it('should add approver when suggested user is clicked', () => {
-      expect($approversEl.find('li.approver').length).toEqual(0);
-
-      $suggestionEl
-        .find('a')
-        .first()
-        .click();
-
-      $suggestionEl
-        .find('a')
-        .last()
-        .click();
-
-      expect($approversEl.find('li.approver').length).toEqual(2);
-    });
-
-    it('only adds approver once when the same suggested user is clicked multiple times', () => {
-      expect($approversEl.find('li.approver').length).toEqual(0);
-
-      $suggestionEl
-        .find('a')
-        .first()
-        .click();
-
-      $suggestionEl
-        .find('a')
-        .first()
-        .click();
-
-      expect($approversEl.find('li.approver').length).toEqual(1);
-    });
-  });
-
-  describe('remove unsaved approver', () => {
-    beforeEach(() => {
-      $suggestionEl.find('a').click(); // Adds two approvers
-    });
-
-    it('should remove approver if confirm window result is positive', () => {
-      spyOn(window, 'confirm').and.returnValue(true);
-
-      $approversEl
-        .find('.unsaved-approvers.approver .btn-remove')
-        .first()
-        .click();
-
-      expect($approversEl.find('li.approver').length).toEqual(1);
-    });
-
-    it('should not remove approver if confirm window result is negative', () => {
-      spyOn(window, 'confirm').and.returnValue(false);
-
-      $approversEl
-        .find('.unsaved-approvers.approver .btn-remove')
-        .first()
-        .click();
-
-      expect($approversEl.find('li.approver').length).toEqual(2);
-    });
-  });
-
-  describe('remove unsaved approver group', () => {
-    it('should remove approver group if confirm window result is positive', () => {
-      spyOn(window, 'confirm').and.returnValue(true);
-
-      expect($approversEl.find('li.approver-group').length).toEqual(1);
-
-      $approversEl
-        .find('.unsaved-approvers.approver-group .btn-remove')
-        .first()
-        .click();
-
-      expect($approversEl.find('li.approver-group').length).toEqual(0);
-    });
-
-    it('should not remove approver group if confirm window result is negative', () => {
-      spyOn(window, 'confirm').and.returnValue(false);
-
-      expect($approversEl.find('li.approver-group').length).toEqual(1);
-
-      $approversEl
-        .find('.unsaved-approvers.approver-group .btn-remove')
-        .first()
-        .click();
-
-      expect($approversEl.find('li.approver-group').length).toEqual(1);
-    });
-  });
-});

ee/spec/models/merge_request_spec.rb

@@ -170,7 +170,6 @@ describe MergeRequest do
     let(:code_owners) { [double(:code_owner)] }
 
     before do
-      stub_feature_flags(approval_rules: false)
       allow(subject).to receive(:code_owners).and_return(code_owners)
     end
 
@@ -536,36 +535,6 @@ describe MergeRequest do
     end
   end
 
-  describe "#overall_approver_groups" do
-    it 'returns a merge request group approver' do
-      project = create :project
-      create :approver_group, target: project
-
-      merge_request = create :merge_request, target_project: project, source_project: project
-      approver_group2 = create :approver_group, target: merge_request
-
-      expect(merge_request.overall_approver_groups).to eq([approver_group2])
-    end
-
-    it 'returns a project group approver' do
-      project = create :project
-      approver_group1 = create :approver_group, target: project
-
-      merge_request = create :merge_request, target_project: project, source_project: project
-
-      expect(merge_request.overall_approver_groups).to eq([approver_group1])
-    end
-
-    it 'returns a merge request approver if there is no project group approver' do
-      project = create :project
-
-      merge_request = create :merge_request, target_project: project, source_project: project
-      approver_group1 = create :approver_group, target: merge_request
-
-      expect(merge_request.overall_approver_groups).to eq([approver_group1])
-    end
-  end
-
   describe '#all_approvers_including_groups' do
     it 'returns correct set of users' do
       user = create :user

ee/spec/models/project_spec.rb

@@ -1024,16 +1024,6 @@ describe Project do
         expect(project.visible_regular_approval_rules).to contain_exactly(approval_rules.first)
       end
     end
-
-    context 'when approval rules are disabled' do
-      before do
-        stub_feature_flags(approval_rules: false)
-      end
-
-      it 'does not return any approval rules' do
-        expect(project.visible_regular_approval_rules).to be_empty
-      end
-    end
   end
 
   describe '#min_fallback_approvals' do
@@ -1060,12 +1050,6 @@ describe Project do
 
         expect(project.min_fallback_approvals).to eq(2)
       end
-
-      it 'returns approvals before merge when code owner rules is disabled' do
-        stub_feature_flags(approval_rules: false)
-
-        expect(project.min_fallback_approvals).to eq(1)
-      end
     end
   end
 

ee/spec/presenters/merge_request_presenter_spec.rb

@@ -84,110 +84,36 @@ describe MergeRequestPresenter do
   describe '#approvers_left' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
-    let!(:approver) { create(:approver, target: merge_request) }
+    let!(:approver) { create(:user) }
+    let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [approver], groups: [private_group, public_group]) }
 
     before do
-      stub_feature_flags(approval_rules: false)
-      merge_request.approvals.create!(user: approver.user)
-    end
-
-    subject { described_class.new(merge_request, current_user: user).approvers_left }
-
-    it { is_expected.to match_array(public_approver_group.users) }
-
-    context 'when user has access to private group' do
-      before do
-        private_group.add_user(user, Gitlab::Access::DEVELOPER)
-      end
-
-      it do
-        approvers = public_approver_group.users + private_approver_group.users - [user]
-
-        is_expected.to match_array(approvers)
-      end
-    end
-  end
-
-  describe '#approvers_left with approval_rule enabled' do
-    let!(:private_group) { create(:group_with_members, :private) }
-    let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
-    let!(:approver) { create(:approver, target: merge_request) }
-
-    before do
-      merge_request.approvals.create!(user: approver.user)
+      merge_request.approvals.create!(user: approver)
     end
 
     subject { described_class.new(merge_request, current_user: user).approvers_left }
 
     it 'contains all approvers' do
-      approvers = public_approver_group.users + private_approver_group.users - [user]
+      approvers = public_group.users + private_group.users - [user]
 
       is_expected.to match_array(approvers)
     end
   end
 
-  describe '#overall_approver_groups' do
-    let!(:private_group) { create(:group_with_members, :private) }
-    let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
-
-    subject { described_class.new(merge_request, current_user: user).overall_approver_groups }
-
-    it { is_expected.to match_array([public_approver_group]) }
-
-    context 'when user has access to private group' do
-      before do
-        private_group.add_user(user, Gitlab::Access::DEVELOPER)
-      end
-
-      it { is_expected.to match_array([public_approver_group, private_approver_group]) }
-    end
-  end
-
-  describe '#all_approvers_including_groups' do
+  describe '#all_approvers_including_groups with approval_rule enabled' do
     let!(:private_group) { create(:group_with_members, :private) }
     let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
-    let!(:approver) { create(:approver, target: merge_request) }
-
-    subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
-
-    before do
-      stub_feature_flags(approval_rules: false)
-    end
+    let!(:approver) { create(:user) }
+    let!(:approval_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: [approver], groups: [private_group, public_group]) }
 
-    it { is_expected.to match_array(public_approver_group.users + [approver.user]) }
-
-    context 'when user has access to private group' do
     before do
-        private_group.add_user(user, Gitlab::Access::DEVELOPER)
-      end
-
-      it do
-        approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
-
-        is_expected.to match_array(approvers)
+      project.add_developer(approver)
     end
-    end
-  end
-
-  describe '#all_approvers_including_groups with approval_rule enabled' do
-    let!(:private_group) { create(:group_with_members, :private) }
-    let!(:public_group) { create(:group_with_members) }
-    let!(:public_approver_group) { create(:approver_group, target: merge_request, group: public_group) }
-    let!(:private_approver_group) { create(:approver_group, target: merge_request, group: private_group) }
-    let!(:approver) { create(:approver, target: merge_request) }
 
     subject { described_class.new(merge_request, current_user: user).all_approvers_including_groups }
 
     it do
-      approvers = [public_approver_group.users, private_approver_group.users, approver.user].flatten - [user]
+      approvers = [public_group.users, private_group.users, approver].flatten - [user]
 
       is_expected.to match_array(approvers)
     end

ee/spec/requests/api/merge_request_approvals_spec.rb

 require 'spec_helper'
 
 describe API::MergeRequestApprovals do
-  set(:user)          { create(:user) }
-  set(:user2)         { create(:user) }
-  set(:user3)         { create(:user) }
-  set(:admin)         { create(:user, :admin) }
-  set(:project)       { create(:project, :public, :repository, creator: user, namespace: user.namespace, only_allow_merge_if_pipeline_succeeds: false) }
-  set(:merge_request) { create(:merge_request, :simple, author: user, assignees: [user, user3], source_project: project, target_project: project, title: "Test", created_at: Time.now) }
-
-  before do
-    project.update_attribute(:approvals_before_merge, 2)
-
-    stub_feature_flags(approval_rules: false)
-  end
-
-  describe 'GET :id/merge_requests/:merge_request_iid/approvals' do
-    it 'retrieves the approval status' do
-      approver = create :user
-      group = create :group
-      project.add_developer(approver)
-      project.add_developer(create(:user))
-      merge_request.approvals.create(user: approver)
-      merge_request.approvers.create(user: approver)
-      merge_request.approver_groups.create(group: group)
-
-      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
-
-      expect(response).to have_gitlab_http_status(200)
-      expect(json_response['approvals_required']).to eq 2
-      expect(json_response['approvals_left']).to eq 1
-      expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
-      expect(json_response['user_can_approve']).to be false
-      expect(json_response['user_has_approved']).to be false
-      expect(json_response['approvers'][0]['user']['username']).to eq(approver.username)
-      expect(json_response['approver_groups'][0]['group']['name']).to eq(group.name)
-    end
-
-    context 'when private group approver' do
-      before do
-        private_group = create(:group, :private)
-        merge_request.approver_groups.create(group: private_group)
-      end
-
-      it 'only shows group approvers visible to the user' do
-        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['approver_groups']).to be_empty
-      end
-
-      context 'when admin' do
-        it 'shows all approver groups' do
-          get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", admin)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['approver_groups'].size).to eq(1)
-        end
-      end
-    end
-
-    context 'when approvers are set to zero' do
-      before do
-        project.update!(approvals_before_merge: 0)
-        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user)
-      end
-
-      it 'returns a 200' do
-        expect(response).to have_gitlab_http_status(200)
-      end
-
-      it 'does not include an error in the response' do
-        expect(json_response['message']).to eq(nil)
-      end
-    end
-  end
-
-  describe 'POST :id/merge_requests/:merge_request_iid/approvals' do
-    shared_examples_for 'user allowed to override approvals required' do
-      context 'when disable_overriding_approvers_per_merge_request is false on the project' do
-        before do
-          project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
-        end
-
-        it 'allows you to set approvals_before_merge' do
-          expect do
-            post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
-          end.to change { merge_request.reload.approvals_before_merge }.from(nil).to(5)
-
-          expect(response).to have_gitlab_http_status(201)
-          expect(json_response['approvals_required']).to eq(5)
-        end
-      end
-
-      context 'when disable_overriding_approvers_per_merge_request is true on the project' do
-        before do
-          project.update_attribute(:disable_overriding_approvers_per_merge_request, true)
-        end
-
-        it 'does not allow you to set approvals_before_merge' do
-          expect do
-            post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
-          end.not_to change { merge_request.reload.approvals_before_merge }
-
-          expect(response).to have_gitlab_http_status(422)
-        end
-      end
-
-      it 'only shows approver groups that are visible to current user' do
-        private_group = create(:group, :private)
-        merge_request.approver_groups.create(group: private_group)
-
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", current_user), params: { approvals_required: 5 }
-
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['approver_groups'].size).to eq(approver_groups_count)
-      end
-    end
-
-    context 'as a project admin' do
-      it_behaves_like 'user allowed to override approvals required' do
-        let(:current_user) { user }
-        let(:approver_groups_count) { 0 }
-      end
-    end
-
-    context 'as a global admin' do
-      it_behaves_like 'user allowed to override approvals required' do
-        let(:current_user) { admin }
-        let(:approver_groups_count) { 1 }
-      end
-    end
-
-    context 'as a random user' do
-      before do
-        project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
-      end
-
-      it 'does not allow you to override approvals required' do
-        expect do
-          post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals", user2), params: { approvals_required: 5 }
-        end.not_to change { merge_request.reload.approvals_before_merge }
-
-        expect(response).to have_gitlab_http_status(403)
-      end
-    end
-  end
-
-  describe 'PUT :id/merge_requests/:merge_request_iid/approvers' do
-    set(:approver) { create(:user) }
-    set(:approver_group) { create(:group) }
-
-    RSpec::Matchers.define_negated_matcher :not_change, :change
-
-    shared_examples_for 'user allowed to change approvers' do
-      context 'when disable_overriding_approvers_per_merge_request is true on the project' do
-        before do
-          project.update_attribute(:disable_overriding_approvers_per_merge_request, true)
-        end
-
-        it 'does not allow overriding approvers' do
-          expect do
-            put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [approver.id], approver_group_ids: [approver_group.id] }
-          end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count }
-        end
-      end
-
-      context 'when disable_overriding_approvers_per_merge_request is false on the project' do
-        before do
-          project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
-        end
-
-        it 'allows overriding approvers' do
-          expect do
-            put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [approver.id], approver_group_ids: [approver_group.id] }
-          end.to change { merge_request.approvers.count }.from(0).to(1)
-             .and change { merge_request.approver_groups.count }.from(0).to(1)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['approvers'][0]['user']['username']).to eq(approver.username)
-          expect(json_response['approver_groups'][0]['group']['name']).to eq(approver_group.name)
-        end
-
-        it 'removes approvers not in the payload' do
-          merge_request.approvers.create(user: approver)
-          merge_request.approver_groups.create(group: approver_group)
-
-          expect do
-            put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-              params: { approver_ids: [], approver_group_ids: [] }.to_json, headers: { CONTENT_TYPE: 'application/json' }
-          end.to change { merge_request.approvers.count }.from(1).to(0)
-             .and change { merge_request.approver_groups.count }.from(1).to(0)
-
-          expect(response).to have_gitlab_http_status(200)
-          expect(json_response['approvers']).to eq([])
-        end
-
-        context 'when sending form-encoded data' do
-          it 'removes approvers not in the payload' do
-            merge_request.approvers.create(user: approver)
-            merge_request.approver_groups.create(group: approver_group)
-
-            expect do
-              put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-                params: { approver_ids: '', approver_group_ids: '' }
-            end.to change { merge_request.approvers.count }.from(1).to(0)
-              .and change { merge_request.approver_groups.count }.from(1).to(0)
-
-            expect(response).to have_gitlab_http_status(200)
-            expect(json_response['approvers']).to eq([])
-          end
-        end
-      end
-
-      it 'only shows approver groups that are visible to current user' do
-        private_group = create(:group, :private)
-        merge_request.approver_groups.create(group: private_group)
-
-        put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", current_user),
-          params: { approver_ids: [approver.id], approver_group_ids: [private_group.id, approver_group.id] }
-
-        expect(response).to have_gitlab_http_status(200)
-        expect(json_response['approver_groups'].size).to eq(approver_groups_count)
-      end
-    end
-
-    context 'as a project admin' do
-      it_behaves_like 'user allowed to change approvers' do
-        let(:current_user) { user }
-        let(:approver_groups_count) { 1 }
-      end
-    end
-
-    context 'as a global admin' do
-      it_behaves_like 'user allowed to change approvers' do
-        let(:current_user) { admin }
-        let(:approver_groups_count) { 2 }
-      end
-    end
-
-    context 'as a random user' do
-      before do
-        project.update_attribute(:disable_overriding_approvers_per_merge_request, false)
-      end
-
-      it 'does not allow overriding approvers' do
-        expect do
-          put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvers", user2),
-            params: { approver_ids: [approver.id], approver_group_ids: [approver_group.id] }
-        end.to not_change { merge_request.approvers.count }.and not_change { merge_request.approver_groups.count }
-
-        expect(response).to have_gitlab_http_status(403)
-      end
-    end
-  end
-
-  describe 'POST :id/merge_requests/:merge_request_iid/approve' do
-    context 'as the author of the merge request' do
-      before do
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", user)
-      end
-
-      it 'returns a 401' do
-        expect(response).to have_gitlab_http_status(401)
-      end
-    end
-
-    context 'as a valid approver' do
-      set(:approver) { create(:user) }
-
-      before do
-        project.add_developer(approver)
-        project.add_developer(create(:user))
-      end
-
-      def approve(extra_params = {})
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/approve", approver), params: extra_params
-      end
-
-      context 'when the sha param is not set' do
-        before do
-          approve
-        end
-
-        it 'approves the merge request' do
-          expect(response).to have_gitlab_http_status(201)
-          expect(json_response['approvals_left']).to eq(1)
-          expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
-          expect(json_response['user_has_approved']).to be true
-        end
-      end
-
-      context 'when the sha param is correct' do
-        before do
-          approve(sha: merge_request.diff_head_sha)
-        end
-
-        it 'approves the merge request' do
-          expect(response).to have_gitlab_http_status(201)
-          expect(json_response['approvals_left']).to eq(1)
-          expect(json_response['approved_by'][0]['user']['username']).to eq(approver.username)
-          expect(json_response['user_has_approved']).to be true
-        end
-      end
-
-      context 'when the sha param is incorrect' do
-        before do
-          approve(sha: merge_request.diff_head_sha.reverse)
-        end
-
-        it 'returns a 409' do
-          expect(response).to have_gitlab_http_status(409)
-        end
-
-        it 'does not approve the merge request' do
-          expect(merge_request.reload.approvals_left).to eq(2)
-        end
-      end
-
-      context 'when project requires force auth for approval' do
-        before do
-          project.update(require_password_to_approve: true)
-          approver.update(password: 'password')
-        end
-
-        it 'returns a 401 with no password' do
-          approve
-          expect(response).to have_gitlab_http_status(401)
-        end
-
-        it 'does not approve the merge request with no password' do
-          approve
-          expect(merge_request.reload.approvals_left).to eq(2)
-        end
-
-        it 'returns a 401 with incorrect password' do
-          approve
-          expect(response).to have_gitlab_http_status(401)
-        end
-
-        it 'does not approve the merge request with incorrect password' do
-          approve
-          expect(merge_request.reload.approvals_left).to eq(2)
-        end
-
-        it 'approves the merge request with correct password' do
-          approve(approval_password: 'password')
-          expect(response).to have_gitlab_http_status(201)
-          expect(merge_request.reload.approvals_left).to eq(1)
-        end
-      end
-
-      it 'only shows group approvers visible to the user' do
-        private_group = create(:group, :private)
-        merge_request.approver_groups.create(group: private_group)
-
-        approve
-
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['approver_groups']).to be_empty
-      end
-    end
-  end
-
-  describe 'POST :id/merge_requests/:merge_request_iid/unapprove' do
-    context 'as a user who has approved the merge request' do
-      set(:approver) { create(:user) }
-      set(:unapprover) { create(:user) }
-
-      before do
-        project.add_developer(approver)
-        project.add_developer(unapprover)
-        project.add_developer(create(:user))
-        merge_request.approvals.create(user: approver)
-        merge_request.approvals.create(user: unapprover)
-      end
-
-      it 'unapproves the merge request' do
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
-
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['approvals_left']).to eq(1)
-        usernames = json_response['approved_by'].map { |u| u['user']['username'] }
-        expect(usernames).not_to include(unapprover.username)
-        expect(usernames.size).to be 1
-        expect(json_response['user_has_approved']).to be false
-        expect(json_response['user_can_approve']).to be true
-      end
-
-      it 'only shows group approvers visible to the user' do
-        private_group = create(:group, :private)
-        merge_request.approver_groups.create(group: private_group)
-
-        post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/unapprove", unapprover)
-
-        expect(response).to have_gitlab_http_status(201)
-        expect(json_response['approver_groups']).to be_empty
-      end
-    end
-  end
-end
-
-describe "API::MergeRequestApprovals with approval_rule enabled" do
   set(:user)          { create(:user) }
   set(:user2)         { create(:user) }
   set(:admin)         { create(:user, :admin) }
@@ -801,6 +399,39 @@ describe "API::MergeRequestApprovals with approval_rule enabled" do
         end
       end
 
+      context 'when project requires force auth for approval' do
+        before do
+          project.update(require_password_to_approve: true)
+          approver.update(password: 'password')
+        end
+
+        it 'returns a 401 with no password' do
+          approve
+          expect(response).to have_gitlab_http_status(401)
+        end
+
+        it 'does not approve the merge request with no password' do
+          approve
+          expect(merge_request.reload.approvals_left).to eq(2)
+        end
+
+        it 'returns a 401 with incorrect password' do
+          approve
+          expect(response).to have_gitlab_http_status(401)
+        end
+
+        it 'does not approve the merge request with incorrect password' do
+          approve
+          expect(merge_request.reload.approvals_left).to eq(2)
+        end
+
+        it 'approves the merge request with correct password' do
+          approve(approval_password: 'password')
+          expect(response).to have_gitlab_http_status(201)
+          expect(merge_request.reload.approvals_left).to eq(1)
+        end
+      end
+
       it 'only shows group approvers visible to the user' do
         private_group = create(:group, :private)
         merge_request.approver_groups.create(group: private_group)

ee/spec/requests/api/project_approvals_spec.rb

@@ -10,15 +10,10 @@ describe API::ProjectApprovals do
 
   let(:url) { "/projects/#{project.id}/approvals" }
 
-  before do
-    stub_feature_flags(approval_rules: false)
-  end
-
-  describe 'GET /projects/:id/approvers' do
+  describe 'GET /projects/:id/approvals' do
     context 'when the request is correct' do
       before do
-        project.approvers.create(user: approver)
-        project.approver_groups.create(group: group)
+        create(:approval_project_rule, project: project, users: [approver], groups: [group])
 
         get api(url, user)
       end
@@ -34,7 +29,7 @@ describe API::ProjectApprovals do
 
     it 'only shows approver groups that are visible to the user' do
       private_group = create(:group, :private)
-      project.approver_groups.create(group: private_group)
+      create(:approval_project_rule, project: project, users: [approver], groups: [private_group])
 
       get api(url, user)
 
@@ -43,7 +38,7 @@ describe API::ProjectApprovals do
     end
   end
 
-  describe 'POST /projects/:id/approvers' do
+  describe 'POST /projects/:id/approvals' do
     shared_examples_for 'a user with access' do
       context 'when missing parameters' do
         it 'returns 400 status' do

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -73,72 +73,7 @@ describe MergeRequests::RefreshService do
         end
       end
 
-      context 'when code owners enabled, with approval_rule disabled' do
-        let(:old_owners) { [] }
-        let(:new_owners) { [] }
-        let(:relevant_merge_requests) { [merge_request, another_merge_request] }
-
-        before do
-          stub_feature_flags(approval_rules: false)
-
-          relevant_merge_requests.each do |merge_request|
-            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request).and_return(new_owners)
-            expect(::Gitlab::CodeOwners).to receive(:for_merge_request).with(merge_request, merge_request_diff: anything).and_wrap_original do |m, *args|
-              expect(args.last[:merge_request_diff]).to eq(merge_request.merge_request_diffs.order(id: :desc).offset(1).first)
-
-              old_owners
-            end
-          end
-
-          [forked_merge_request].each do |merge_request|
-            expect(::Gitlab::CodeOwners).not_to receive(:for_merge_request).with(merge_request, anything)
-          end
-
-          expect(service.todo_service).not_to receive(:add_merge_request_approvers)
-          expect(service.notification_service).not_to receive(:add_merge_request_approvers)
-        end
-
-        context 'merge request has overwritten approvers' do
-          context 'when new owners are being added' do
-            let(:new_owners) { [owner] }
-
-            it 'does not create Approver' do
-              expect { subject }.not_to change { Approver.count }
-
-              rule = merge_request.approval_rules.code_owner.first
-              expect(rule.users).to eq(new_owners)
-            end
-          end
-        end
-
-        context 'merge request has default approvers' do
-          let(:existing_approver) { create(:user) }
-
-          before do
-            create(:approver, target: merge_request, user: existing_approver)
-          end
-
-          context 'when new owners are being added' do
-            let(:new_owners) { [owner] }
-
-            it 'creates Approver' do
-              expect { subject }.to change { Approver.count }.by(1)
-
-              new_approver = merge_request.approvers.last
-
-              expect(merge_request.approvers.first.user).to eq(existing_approver)
-              expect(new_approver.user).to eq(owner)
-              expect(new_approver.created_at).to be_present
-              expect(new_approver.updated_at).to be_present
-
-              rule = merge_request.approval_rules.code_owner.first
-              expect(rule.users).to eq(new_owners)
-            end
-          end
-        end
-      end
-
-      context 'when code owners enabled, with approval_rule enabled' do
+      context 'when code owners enabled' do
         let(:relevant_merge_requests) { [merge_request, another_merge_request] }
 
         it 'refreshes the code owner rules for all relevant merge requests' do

ee/spec/services/ee/notification_service_spec.rb

@@ -491,8 +491,6 @@ describe EE::NotificationService, :mailer do
     end
 
     before do
-      stub_feature_flags(approval_rules: false)
-
       project.add_maintainer(merge_request.author)
       merge_request.assignees.each { |assignee| project.add_maintainer(assignee) }
       build_team(merge_request.target_project)
@@ -511,10 +509,10 @@ describe EE::NotificationService, :mailer do
 
       context 'when the target project has approvers set' do
         let(:project_approvers) { create_list(:user, 3) }
+        let!(:rule) { create(:approval_project_rule, project: project, users: project_approvers, approvals_required: 1 )}
 
         before do
           merge_request.target_project.update(approvals_before_merge: 1)
-          project_approvers.each { |approver| create(:approver, user: approver, target: merge_request.target_project) }
           reset_delivered_emails!
         end
 
@@ -525,7 +523,7 @@ describe EE::NotificationService, :mailer do
         end
 
         it 'does not email the approvers when approval is not necessary' do
-          merge_request.target_project.update(approvals_before_merge: 0)
+          rule.update(approvals_required: 0)
           notification.new_merge_request(merge_request, @u_disabled)
 
           project_approvers.each { |approver| should_not_email(approver) }
@@ -533,9 +531,9 @@ describe EE::NotificationService, :mailer do
 
         context 'when the merge request has approvers set' do
           let(:mr_approvers) { create_list(:user, 3) }
+          let!(:mr_rule) { create(:approval_merge_request_rule, merge_request: merge_request, users: mr_approvers, approvals_required: 1 )}
 
           before do
-            mr_approvers.each { |approver| create(:approver, user: approver, target: merge_request) }
             reset_delivered_emails!
           end
 

ee/spec/services/merge_requests/remove_approval_service_spec.rb

@@ -8,10 +8,6 @@ describe MergeRequests::RemoveApprovalService do
 
     subject(:service) { described_class.new(project, user) }
 
-    before do
-      stub_feature_flags(approval_rules: false)
-    end
-
     def execute!
       service.execute(merge_request)
     end

ee/spec/services/merge_requests/update_service_spec.rb

@@ -60,32 +60,6 @@ describe MergeRequests::UpdateService, :mailer do
     context 'when approvals_before_merge changes' do
       using RSpec::Parameterized::TableSyntax
 
-      context 'when approval_rules is disabled' do
-        before do
-          stub_feature_flags(approval_rules: false)
-        end
-
-        where(:project_value, :mr_before_value, :mr_after_value, :result) do
-          3 | 4   | 5   | 5
-          3 | 4   | nil | 3
-          3 | nil | 5   | 5
-        end
-
-        with_them do
-          let(:project) { create(:project, :repository, approvals_before_merge: project_value) }
-
-          it "updates approval_rules' approvals_required" do
-            merge_request.update(approvals_before_merge: mr_before_value)
-            rule = create(:approval_merge_request_rule, merge_request: merge_request)
-
-            update_merge_request(approvals_before_merge: mr_after_value)
-
-            expect(rule.reload.approvals_required).to eq(result)
-          end
-        end
-      end
-
-      context 'when approval_rules is enabled' do
       where(:project_value, :mr_before_value, :mr_after_value, :result) do
         3 | 4   | 5   | 5
         3 | 4   | nil | 3
@@ -105,13 +79,8 @@ describe MergeRequests::UpdateService, :mailer do
         end
       end
     end
-    end
 
     context 'merge' do
-      before do
-        stub_feature_flags(approval_rules: false)
-      end
-
       let(:opts) { { merge: merge_request.diff_head_sha } }
 
       context 'when not approved' do
@@ -150,8 +119,6 @@ describe MergeRequests::UpdateService, :mailer do
       let(:new_approver) { create(:user) }
 
       before do
-        stub_feature_flags(approval_rules: false)
-
         project.add_developer(existing_approver)
         project.add_developer(removed_approver)
         project.add_developer(new_approver)

ee/spec/services/projects/update_service_spec.rb

@@ -177,40 +177,6 @@ describe Projects::UpdateService, '#execute' do
     end
   end
 
-  context 'with approval_rules' do
-    context 'when approval_rules is disabled' do
-      it "updates approval_rules' approvals_required" do
-        stub_feature_flags(approval_rules: false)
-
-        rule = create(:approval_project_rule, project: project)
-
-        update_project(project, user, approvals_before_merge: 42)
-
-        expect(rule.reload.approvals_required).to eq(42)
-      end
-    end
-
-    context 'when approval_rules is enabled' do
-      it 'does not update' do
-        rule = create(:approval_project_rule, project: project)
-
-        update_project(project, user, approvals_before_merge: 42)
-
-        expect(rule.reload.approvals_required).to eq(0)
-      end
-    end
-
-    context 'when approval_rule feature is enabled' do
-      it "does not update approval_rules' approvals_required" do
-        rule = create(:approval_project_rule, project: project)
-
-        expect do
-          update_project(project, user, approvals_before_merge: 42)
-        end.not_to change { rule.reload.approvals_required }
-      end
-    end
-  end
-
   describe 'repository_storage' do
     let(:admin_user) { create(:user, admin: true) }
     let(:user) { create(:user) }

ee/spec/support/helpers/feature_approval_helper.rb

+# frozen_string_literal: true
+
+module FeatureApprovalHelper
+  def open_modal
+    page.execute_script "document.querySelector('#{config_selector}').scrollIntoView()"
+    within(config_selector) do
+      click_on('Edit')
+    end
+  end
+
+  def open_approver_select
+    within(modal_selector) do
+      find('.select2-input').click
+    end
+    wait_for_requests
+  end
+
+  def close_approver_select
+    within(modal_selector) do
+      find('.select2-input').send_keys :escape
+    end
+  end
+
+  def remove_approver(name)
+    el = page.find("#{modal_selector} .content-list li", text: /#{name}/i)
+    el.find('button').click
+  end
+
+  def expect_avatar(container, users)
+    users = Array(users)
+
+    members = container.all('.js-members img.avatar').map do |member|
+      member['alt']
+    end
+
+    users.each do |user|
+      expect(members).to include(user.name)
+    end
+
+    expect(members.size).to eq(users.size)
+  end
+end

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

@@ -17,15 +17,10 @@ describe 'shared/issuable/_approvals.html.haml' do
     allow(form).to receive(:number_field)
     allow(MergeRequestApproverPresenter).to receive(:new).and_return(approver_presenter)
     assign(:project, project)
+    assign(:target_project, project)
   end
 
   context 'has no approvers' do
-    it 'shows empty approvers list' do
-      render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
-
-      expect(rendered).to have_text('There are no approvers')
-    end
-
     context 'can override approvers' do
       before do
         render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
@@ -34,14 +29,6 @@ describe 'shared/issuable/_approvals.html.haml' do
       it 'shows suggested approvers' do
         expect(rendered).to have_css('.suggested-approvers')
       end
-
-      it 'shows select approvers field' do
-        expect(rendered).to have_css('#merge_request_approver_ids', visible: false)
-      end
-
-      it 'shows select approver groups field' do
-        expect(rendered).to have_css('#merge_request_approver_group_ids', visible: false)
-      end
     end
 
     context 'can not override approvers' do
@@ -82,14 +69,6 @@ describe 'shared/issuable/_approvals.html.haml' do
       expect(rendered).to have_text(approver_group[:name])
     end
 
-    context 'can override approvers' do
-      it 'shows remove button for approver' do
-        render 'shared/issuable/approvals', form: form, issuable: merge_request, presenter: presenter
-
-        expect(rendered).to have_css('.btn-remove')
-      end
-    end
-
     context 'can not override approvers' do
       it 'hides remove button' do
         allow(view).to receive(:can?).with(user, :update_approvers, merge_request).and_return(false)

locale/gitlab.pot

@@ -693,9 +693,6 @@ msgstr ""
 msgid "Add an SSH key"
 msgstr ""
 
-msgid "Add approver(s)"
-msgstr ""
-
 msgid "Add approvers"
 msgstr ""
 
@@ -750,9 +747,6 @@ msgstr ""
 msgid "Add user(s) to the group:"
 msgstr ""
 
-msgid "Add users or groups who are allowed to approve every merge request"
-msgstr ""
-
 msgid "Add users to group"
 msgstr ""
 
@@ -1082,9 +1076,6 @@ msgstr ""
 msgid "An error occurred when updating the issue weight"
 msgstr ""
 
-msgid "An error occurred while adding approver"
-msgstr ""
-
 msgid "An error occurred while deleting the approvers group"
 msgstr ""
 
@@ -1193,9 +1184,6 @@ msgstr ""
 msgid "An error occurred while parsing recent searches"
 msgstr ""
 
-msgid "An error occurred while removing approver"
-msgstr ""
-
 msgid "An error occurred while removing epics."
 msgstr ""
 
@@ -1414,12 +1402,6 @@ msgstr ""
 msgid "Approvals"
 msgstr ""
 
-msgid "Approvals required"
-msgstr ""
-
-msgid "Approvers"
-msgstr ""
-
 msgid "Apr"
 msgstr ""
 
@@ -1483,18 +1465,6 @@ msgstr ""
 msgid "Are you sure you want to remove %{group_name}?"
 msgstr ""
 
-msgid "Are you sure you want to remove approver %{name}"
-msgstr ""
-
-msgid "Are you sure you want to remove approver %{name}?"
-msgstr ""
-
-msgid "Are you sure you want to remove group %{name}"
-msgstr ""
-
-msgid "Are you sure you want to remove group %{name}?"
-msgstr ""
-
 msgid "Are you sure you want to remove the attachment?"
 msgstr ""
 
@@ -10760,9 +10730,6 @@ msgstr ""
 msgid "Remove all or specific label(s)"
 msgstr ""
 
-msgid "Remove approver"
-msgstr ""
-
 msgid "Remove approvers"
 msgstr ""
 
@@ -11699,9 +11666,6 @@ msgstr ""
 msgid "Set notification email for abuse reports."
 msgstr ""
 
-msgid "Set number of approvers required before open merge requests can be merged"
-msgstr ""
-
 msgid "Set requirements for a user to sign-in. Enable mandatory two-factor authentication."
 msgstr ""
 
@@ -13083,9 +13047,6 @@ msgstr ""
 msgid "There are no SSH keys with access to your account."
 msgstr ""
 
-msgid "There are no approvers"
-msgstr ""
-
 msgid "There are no archived projects yet"
 msgstr ""
 
@@ -13377,12 +13338,6 @@ msgstr ""
 msgid "This merge request is locked."
 msgstr ""
 
-msgid "This merge request must be approved by members of these groups. You can override the project settings by setting your own list of approvers."
-msgstr ""
-
-msgid "This merge request must be approved by these users. You can override the project settings by setting your own list of approvers."
-msgstr ""
-
 msgid "This namespace has already been taken! Please choose another one."
 msgstr ""
 
@@ -15922,9 +15877,6 @@ msgstr ""
 msgid "mrWidget|%{metricsLinkStart} Memory %{metricsLinkEnd} usage is %{emphasisStart} unchanged %{emphasisEnd} at %{memoryFrom}MB"
 msgstr ""
 
-msgid "mrWidget|Add approval"
-msgstr ""
-
 msgid "mrWidget|Allows commits from members who can merge to the target branch"
 msgstr ""
 
@@ -16018,15 +15970,9 @@ msgstr ""
 msgid "mrWidget|Merge locally"
 msgstr ""
 
-msgid "mrWidget|Merge request approved"
-msgstr ""
-
 msgid "mrWidget|Merge request approved."
 msgstr ""
 
-msgid "mrWidget|Merge request approved; you can approve additionally"
-msgstr ""
-
 msgid "mrWidget|Merged by"
 msgstr ""
 
@@ -16057,22 +16003,9 @@ msgstr ""
 msgid "mrWidget|Refreshing now"
 msgstr ""
 
-msgid "mrWidget|Remove your approval"
-msgstr ""
-
 msgid "mrWidget|Request to merge"
 msgstr ""
 
-msgid "mrWidget|Requires 1 more approval"
-msgid_plural "mrWidget|Requires %d more approvals"
-msgstr[0] ""
-msgstr[1] ""
-
-msgid "mrWidget|Requires 1 more approval by"
-msgid_plural "mrWidget|Requires %d more approvals by"
-msgstr[0] ""
-msgstr[1] ""
-
 msgid "mrWidget|Resolve conflicts"
 msgstr ""
 

qa/qa/ee/page/merge_request/show.rb

@@ -32,10 +32,6 @@ module QA
                 element :review_preview_toggle
               end
 
-              view 'ee/app/views/shared/issuable/_approvals_single_rule.html.haml' do
-                element :approver_list
-              end
-
               def start_review
                 click_element :start_review
               end

spec/features/projects_spec.rb

@@ -6,7 +6,6 @@ describe 'Project' do
 
   before do
     stub_feature_flags(vue_file_list: false)
-    stub_feature_flags(approval_rules: false)
   end
 
   describe 'creating from template' do