Merge branch '2566-namespace-license-mr-approvers' into 'master'

Introduce namespace license checks for merge request approvers (EES)

Closes #2566

See merge request !2324

app/models/concerns/approvable.rb

 module Approvable
-  extend ActiveSupport::Concern
+  def requires_approve?
+    approvals_required.nonzero?
+  end
 
-  included do
-    def requires_approve?
-      approvals_required.nonzero?
-    end
+  def approved?
+    approvals_left < 1
+  end
 
-    def approved?
-      approvals_left < 1
-    end
+  # Number of approvals remaining (excluding existing approvals) before the MR is
+  # considered approved. If there are fewer potential approvers than approvals left,
+  # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
+  #
+  def approvals_left
+    [
+      [approvals_required - approvals.count, number_of_potential_approvers].min,
+      0
+    ].max
+  end
 
-    # Number of approvals remaining (excluding existing approvals) before the MR is
-    # considered approved. If there are fewer potential approvers than approvals left,
-    # choose the lower so the MR doesn't get 'stuck' in a state where it can't be approved.
-    #
-    def approvals_left
-      [
-        [approvals_required - approvals.count, number_of_potential_approvers].min,
-        0
-      ].max
-    end
+  def approvals_required
+    approvals_before_merge || target_project.approvals_before_merge
+  end
 
-    def approvals_required
-      approvals_before_merge || target_project.approvals_before_merge
-    end
+  def approvals_before_merge
+    return 0 unless project&.feature_available?(:merge_request_approvers)
 
-    # An MR can potentially be approved by:
-    # - anyone in the approvers list
-    # - any other project member with developer access or higher (if there are no approvers
-    #   left)
-    #
-    # It cannot be approved by:
-    # - a user who has already approved the MR
-    # - the MR author
-    #
-    def number_of_potential_approvers
-      has_access = ['access_level > ?', Member::REPORTER]
-      users_with_access = { id: project.project_authorizations.where(has_access).select(:user_id) }
-      all_approvers = all_approvers_including_groups
-
-      users_relation = User.active.where.not(id: approvals.select(:user_id))
-      users_relation = users_relation.where.not(id: author.id) if author
-
-      # This is an optimisation for large instances. Instead of getting the
-      # count of all users who meet the conditions in a single query, which
-      # produces a slow query plan, we get the union of all users with access
-      # and all users in the approvers list, and count them.
-      if all_approvers.any?
-        specific_approvers = { id: all_approvers.map(&:id) }
-
-        union = Gitlab::SQL::Union.new([
-          users_relation.where(users_with_access).select(:id),
-          users_relation.where(specific_approvers).select(:id)
-        ])
-
-        User.from("(#{union.to_sql}) subquery").count
-      else
-        users_relation.where(users_with_access).count
-      end
-    end
+    super
+  end
 
-    # Users in the list of approvers who have not already approved this MR.
-    #
-    def approvers_left
-      User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id))
+  # An MR can potentially be approved by:
+  # - anyone in the approvers list
+  # - any other project member with developer access or higher (if there are no approvers
+  #   left)
+  #
+  # It cannot be approved by:
+  # - a user who has already approved the MR
+  # - the MR author
+  #
+  def number_of_potential_approvers
+    has_access = ['access_level > ?', Member::REPORTER]
+    users_with_access = { id: project.project_authorizations.where(has_access).select(:user_id) }
+    all_approvers = all_approvers_including_groups
+
+    users_relation = User.active.where.not(id: approvals.select(:user_id))
+    users_relation = users_relation.where.not(id: author.id) if author
+
+    # This is an optimisation for large instances. Instead of getting the
+    # count of all users who meet the conditions in a single query, which
+    # produces a slow query plan, we get the union of all users with access
+    # and all users in the approvers list, and count them.
+    if all_approvers.any?
+      specific_approvers = { id: all_approvers.map(&:id) }
+
+      union = Gitlab::SQL::Union.new([
+        users_relation.where(users_with_access).select(:id),
+        users_relation.where(specific_approvers).select(:id)
+      ])
+
+      User.from("(#{union.to_sql}) subquery").count
+    else
+      users_relation.where(users_with_access).count
     end
+  end
 
-    # The list of approvers from either this MR (if they've been set on the MR) or the
-    # target project. Excludes the author by default.
-    #
-    # Before a merge request has been created, author will be nil, so pass the current user
-    # on the MR create page.
-    #
-    def overall_approvers
-      approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
-      approvers_relation = approvers_relation.where.not(user_id: author.id) if author
-
-      approvers_relation
-    end
+  # Users in the list of approvers who have not already approved this MR.
+  #
+  def approvers_left
+    User.where(id: all_approvers_including_groups.map(&:id)).where.not(id: approvals.select(:user_id))
+  end
 
-    def overall_approver_groups
-      approvers_overwritten? ? approver_groups : target_project.approver_groups
-    end
+  # The list of approvers from either this MR (if they've been set on the MR) or the
+  # target project. Excludes the author by default.
+  #
+  # Before a merge request has been created, author will be nil, so pass the current user
+  # on the MR create page.
+  #
+  def overall_approvers
+    approvers_relation = approvers_overwritten? ? approvers : target_project.approvers
+    approvers_relation = approvers_relation.where.not(user_id: author.id) if author
+
+    approvers_relation
+  end
 
-    def all_approvers_including_groups
-      approvers = []
+  def overall_approver_groups
+    approvers_overwritten? ? approver_groups : target_project.approver_groups
+  end
 
-      # Approvers from direct assignment
-      approvers << approvers_from_users
+  def all_approvers_including_groups
+    approvers = []
 
-      approvers << approvers_from_groups
+    # Approvers from direct assignment
+    approvers << approvers_from_users
 
-      approvers.flatten
-    end
+    approvers << approvers_from_groups
 
-    def approvers_from_users
-      overall_approvers.map(&:user)
-    end
+    approvers.flatten
+  end
 
-    def approvers_from_groups
-      group_approvers = []
+  def approvers_from_users
+    overall_approvers.map(&:user)
+  end
 
-      overall_approver_groups.each do |approver_group|
-        group_approvers << approver_group.users
-      end
+  def approvers_from_groups
+    group_approvers = []
 
-      group_approvers.flatten!
+    overall_approver_groups.each do |approver_group|
+      group_approvers << approver_group.users
+    end
 
-      group_approvers.delete(author)
+    group_approvers.flatten!
 
-      group_approvers
-    end
+    group_approvers.delete(author)
 
-    def approvers_overwritten?
-      approvers.to_a.any? || approver_groups.to_a.any?
-    end
+    group_approvers
+  end
 
-    def can_approve?(user)
-      return false unless user
-      return true if approvers_left.include?(user)
-      return false if user == author
-      return false unless user.can?(:update_merge_request, self)
+  def approvers_overwritten?
+    approvers.to_a.any? || approver_groups.to_a.any?
+  end
 
-      any_approver_allowed? && approvals.where(user: user).empty?
-    end
+  def can_approve?(user)
+    return false unless user
+    return true if approvers_left.include?(user)
+    return false if user == author
+    return false unless user.can?(:update_merge_request, self)
 
-    def has_approved?(user)
-      return false unless user
+    any_approver_allowed? && approvals.where(user: user).empty?
+  end
 
-      approved_by_users.include?(user)
-    end
+  def has_approved?(user)
+    return false unless user
 
-    # Once there are fewer approvers left in the list than approvals required, allow other
-    # project members to approve the MR.
-    #
-    def any_approver_allowed?
-      approvals_left > approvers_left.count
-    end
+    approved_by_users.include?(user)
+  end
 
-    def approved_by_users
-      approvals.map(&:user)
-    end
+  # Once there are fewer approvers left in the list than approvals required, allow other
+  # project members to approve the MR.
+  #
+  def any_approver_allowed?
+    approvals_left > approvers_left.count
+  end
+
+  def approved_by_users
+    approvals.map(&:user)
+  end
 
-    def approver_ids=(value)
-      value.split(",").map(&:strip).each do |user_id|
-        next if author && user_id == author.id
+  def approver_ids=(value)
+    value.split(",").map(&:strip).each do |user_id|
+      next if author && user_id == author.id
 
-        approvers.find_or_initialize_by(user_id: user_id, target_id: id)
-      end
+      approvers.find_or_initialize_by(user_id: user_id, target_id: id)
     end
+  end
 
-    def approver_group_ids=(value)
-      value.split(",").map(&:strip).each do |group_id|
-        approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
-      end
+  def approver_group_ids=(value)
+    value.split(",").map(&:strip).each do |group_id|
+      approver_groups.find_or_initialize_by(group_id: group_id, target_id: id)
     end
   end
 end

app/models/ee/merge_request.rb

 module EE
   module MergeRequest
+    include ::Approvable
+
     def ff_merge_possible?
       project.repository.is_ancestor?(target_branch_sha, diff_head_sha)
     end

app/models/ee/project.rb

@@ -246,6 +246,17 @@ module EE
       default_issues_tracker? || jira_tracker_active?
     end
 
+    def approvals_before_merge
+      return 0 unless feature_available?(:merge_request_approvers)
+
+      super
+    end
+
+    def reset_approvals_on_push
+      super && feature_available?(:merge_request_approvers)
+    end
+    alias_method :reset_approvals_on_push?, :reset_approvals_on_push
+
     def approver_ids=(value)
       value.split(",").map(&:strip).each do |user_id|
         approvers.find_or_create_by(user_id: user_id, target_id: id)

app/models/license.rb

@@ -12,6 +12,7 @@ class License < ActiveRecord::Base
   GEO_FEATURE = 'GitLab_Geo'.freeze
   ISSUE_BOARDS_FOCUS_MODE_FEATURE = 'IssueBoardsFocusMode'.freeze
   ISSUE_WEIGHTS_FEATURE = 'GitLab_IssueWeights'.freeze
+  MERGE_REQUEST_APPROVERS_FEATURE = 'GitLab_MergeRequestApprovers'.freeze
   MERGE_REQUEST_REBASE_FEATURE = 'GitLab_MergeRequestRebase'.freeze
   MERGE_REQUEST_SQUASH_FEATURE = 'GitLab_MergeRequestSquash'.freeze
   OBJECT_STORAGE_FEATURE = 'GitLab_ObjectStorage'.freeze
@@ -35,6 +36,7 @@ class License < ActiveRecord::Base
     file_lock: FILE_LOCK_FEATURE,
     issue_board_focus_mode: ISSUE_BOARDS_FOCUS_MODE_FEATURE,
     issue_weights: ISSUE_WEIGHTS_FEATURE,
+    merge_request_approvers: MERGE_REQUEST_APPROVERS_FEATURE,
     merge_request_rebase: MERGE_REQUEST_REBASE_FEATURE,
     merge_request_squash: MERGE_REQUEST_SQUASH_FEATURE
   }.freeze
@@ -52,6 +54,7 @@ class License < ActiveRecord::Base
     { FAST_FORWARD_MERGE_FEATURE => 1 },
     { ISSUE_BOARDS_FOCUS_MODE_FEATURE => 1 },
     { ISSUE_WEIGHTS_FEATURE => 1 },
+    { MERGE_REQUEST_APPROVERS_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
     { MERGE_REQUEST_SQUASH_FEATURE => 1 },
     { RELATED_ISSUES_FEATURE => 1 }
@@ -89,6 +92,7 @@ class License < ActiveRecord::Base
     { GEO_FEATURE => 1 },
     { ISSUE_BOARDS_FOCUS_MODE_FEATURE => 1 },
     { ISSUE_WEIGHTS_FEATURE => 1 },
+    { MERGE_REQUEST_APPROVERS_FEATURE => 1 },
     { MERGE_REQUEST_REBASE_FEATURE => 1 },
     { MERGE_REQUEST_SQUASH_FEATURE => 1 },
     { OBJECT_STORAGE_FEATURE => 1 },

app/models/merge_request.rb

@@ -5,7 +5,6 @@ class MergeRequest < ActiveRecord::Base
   include Referable
   include Sortable
   include Elastic::MergeRequestsSearch
-  include Approvable
   include IgnorableColumn
 
   ignore_column :position

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

+- return unless project.feature_available?(:merge_request_approvers)
+
+.form-group.reset-approvals-on-push
+  .checkbox
+    = label_tag :require_approvals do
+      = check_box_tag :require_approvals, nil, project.approvals_before_merge.nonzero?, class: 'js-require-approvals-toggle'
+      %strong Activate merge request approvals
+      = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
+      .descr Merge request approvals allow you to set the number of necessary approvals and predefine a list of approvers that you will need to approve every merge request in a project.
+
+.nested-settings{ class: project.approvals_before_merge.nonzero? ? '' : 'hidden' }
+  .form-group
+    = form.label :approver_ids, class: 'label-light' do
+      Approvers
+    = hidden_field_tag "project[approver_ids]"
+    = hidden_field_tag "project[approver_group_ids]"
+    .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' }
+      %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' }
+        Add
+    .help-block
+      Add an approver or group suggestion for each merge request
+
+    .panel.panel-default.prepend-top-10.js-current-approvers
+      .panel-heading
+        Approvers
+        %span.badge
+          - 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.well-list.approver-list
+        .load-wrapper.hidden
+          = icon('spinner spin', class: 'approver-list-loader')
+        - project.approvers.each do |approver|
+          %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
+            = link_to approver.user.name, approver.user
+            .pull-right
+              %button{ href: namespace_project_approver_path(project.namespace, project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, 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
+                = approver_group.group.members.count
+            .pull-right
+              %button{ href: namespace_project_approver_group_path(project.namespace, project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, 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-light' do
+      Approvals required
+    = form.number_field :approvals_before_merge, class: "form-control", min: 0
+    .help-block
+
+  .form-group.reset-approvals-on-push
+    .checkbox
+      = form.label :reset_approvals_on_push do
+        = form.check_box :reset_approvals_on_push
+        %strong Reset approvals on push
+        .descr Approvals are reset when new data is pushed to the merge request
+

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

@@ -47,74 +47,7 @@
   .hint
     Description parsed with #{link_to "GitLab Flavored Markdown", help_page_path('user/markdown'), target: '_blank'}.
 
-.form-group.reset-approvals-on-push
-  .checkbox
-    = label_tag :require_approvals do
-      = check_box_tag :require_approvals, nil, project.approvals_before_merge.nonzero?, class: 'js-require-approvals-toggle'
-      %strong Activate merge request approvals
-      = link_to icon('question-circle'), help_page_path("user/project/merge_requests/merge_request_approvals"), target: '_blank'
-      .descr Merge request approvals allow you to set the number of necessary approvals and predefine a list of approvers that you will need to approve every merge request in a project.
-
-.nested-settings{ class: project.approvals_before_merge.nonzero? ? '' : 'hidden' }
-  .form-group
-    = form.label :approver_ids, class: 'label-light' do
-      Approvers
-    = hidden_field_tag "project[approver_ids]"
-    = hidden_field_tag "project[approver_group_ids]"
-    .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' }
-      %button.btn.btn-success.js-add-approvers{ type: 'button', title: 'Add approvers(s)' }
-        Add
-    .help-block
-      Add an approver or group suggestion for each merge request
-
-    .panel.panel-default.prepend-top-10.js-current-approvers
-      .panel-heading
-        Approvers
-        %span.badge
-          - 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.well-list.approver-list
-        .load-wrapper.hidden
-          = icon('spinner spin', class: 'approver-list-loader')
-        - project.approvers.each do |approver|
-          %li.approver.settings-flex-row.js-approver{ data: { id: approver.user_id } }
-            = link_to approver.user.name, approver.user
-            .pull-right
-              %button{ href: namespace_project_approver_path(project.namespace, project, approver), data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, 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
-                = approver_group.group.members.count
-            .pull-right
-              %button{ href: namespace_project_approver_group_path(project.namespace, project, approver_group), data: { confirm: "Are you sure you want to remove group #{approver_group.group.name}" }, 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-light' do
-      Approvals required
-    = form.number_field :approvals_before_merge, class: "form-control", min: 0
-    .help-block
-
-  .form-group.reset-approvals-on-push
-    .checkbox
-      = form.label :reset_approvals_on_push do
-        = form.check_box :reset_approvals_on_push
-        %strong Reset approvals on push
-        .descr Approvals are reset when new data is pushed to the merge request
+= render 'projects/ee/merge_request_approvals_settings', project: project, form: form
 
 :javascript
   new GroupsSelect();

changelogs/unreleased-ee/2566-namespace-license-mr-approvers.yml

+---
+title: Introduce namespace license checks for merge request approvers (EES)
+merge_request: 2324
+author:

lib/api/entities.rb

@@ -132,7 +132,7 @@ module API
       expose :printing_merge_request_link_enabled
 
       # EE only
-      expose :approvals_before_merge
+      expose :approvals_before_merge, if: ->(project, _) { project.feature_available?(:merge_request_approvers) }
 
       expose :statistics, using: 'API::Entities::ProjectStatistics', if: :statistics
     end

lib/api/v3/entities.rb

@@ -116,7 +116,7 @@ module API
         expose :repository_storage, if: lambda { |_project, options| options[:current_user].try(:admin?) }
         expose :request_access_enabled
         expose :only_allow_merge_if_all_discussions_are_resolved
-        expose :approvals_before_merge
+        expose :approvals_before_merge, if: ->(project, _) { project.feature_available?(:merge_request_approvers) }
 
         expose :statistics, using: '::API::V3::Entities::ProjectStatistics', if: :statistics
       end

spec/models/ee/merge_request_spec.rb

@@ -127,4 +127,26 @@ describe MergeRequest, models: true do
       end
     end
   end
+
+  describe '#approvals_before_merge' do
+    [
+      { license: true,  database: 5,  expected: 5 },
+      { license: true,  database: 0,  expected: 0 },
+      { license: false, database: 5,  expected: 0 },
+      { license: false, database: 0,  expected: 0 }
+    ].each do |spec|
+      context spec.inspect do
+        let(:spec) { spec }
+        let(:merge_request) { build(:merge_request, approvals_before_merge: spec[:database]) }
+
+        subject { merge_request.approvals_before_merge }
+
+        before do
+          stub_licensed_features(merge_request_approvers: spec[:license])
+        end
+
+        it { is_expected.to eq(spec[:expected]) }
+      end
+    end
+  end
 end

spec/models/ee/project_spec.rb

@@ -295,6 +295,72 @@ describe Project, models: true do
     end
   end
 
+  describe '#approvals_before_merge' do
+    [
+      { license: true,  database: 5,  expected: 5 },
+      { license: true,  database: 0,  expected: 0 },
+      { license: false, database: 5,  expected: 0 },
+      { license: false, database: 0,  expected: 0 }
+    ].each do |spec|
+      context spec.inspect do
+        let(:spec) { spec }
+        let(:project) { build(:project, approvals_before_merge: spec[:database]) }
+
+        subject { project.approvals_before_merge }
+
+        before do
+          stub_licensed_features(merge_request_approvers: spec[:license])
+        end
+
+        it { is_expected.to eq(spec[:expected]) }
+      end
+    end
+  end
+
+  describe "#reset_approvals_on_push?" do
+    [
+      { license: true,  database: true,  expected: true },
+      { license: true,  database: false, expected: false },
+      { license: false, database: true,  expected: false },
+      { license: false, database: false, expected: false }
+    ].each do |spec|
+      context spec.inspect do
+        let(:spec) { spec }
+        let(:project) { build(:project, reset_approvals_on_push: spec[:database]) }
+
+        subject { project.reset_approvals_on_push? }
+
+        before do
+          stub_licensed_features(merge_request_approvers: spec[:license])
+        end
+
+        it { is_expected.to eq(spec[:expected]) }
+      end
+    end
+  end
+
+  describe '#approvals_before_merge' do
+    [
+      { license: true,  database: 5,  expected: 5 },
+      { license: true,  database: 0,  expected: 0 },
+      { license: false, database: 5,  expected: 0 },
+      { license: false, database: 0,  expected: 0 }
+    ].each do |spec|
+      context spec.inspect do
+        let(:spec) { spec }
+        let(:project) { build(:project, approvals_before_merge: spec[:database]) }
+
+        subject { project.approvals_before_merge }
+
+        before do
+          stub_licensed_features(merge_request_approvers: spec[:license])
+        end
+
+        it { is_expected.to eq(spec[:expected]) }
+      end
+    end
+  end
+
   describe '#merge_method' do
     [
       { ff: true,  rebase: true,  ff_licensed: true,  rebase_licensed: true,  method: :ff },

spec/support/ee/license_helpers.rb

@@ -9,6 +9,9 @@ module EE
     # This enables `geo` and disables `deploy_board` features for a spec.
     # Other features are still enabled/disabled as defined in the licence.
     def stub_licensed_features(features)
+      unknown_features = features.keys - License::FEATURE_CODES.keys
+      raise "Unknown features: #{unknown_features.inspect}" unless unknown_features.empty?
+
       allow(License).to receive(:feature_available?).and_call_original
 
       features.each do |feature, enabled|