From be31aff99157c1a11d33bec7edfabb6babdc0b49 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 19 Nov 2018 11:35:36 +0800
Subject: [PATCH 1/9] Add ApprovalMergeRequestRule and ApprovalProjectRule

These represents rules associated with MRs and projects.
Rules are further associated with User and Group as members.

ApprovalMergeRequestRule is associated with Approvals,
as a record of whom approved what when MR is merged.
This is because recomputation result can change in
the future when project approvers/group members changes.
---
 db/schema.rb                                  | 66 ++++++++++++++
 ee/app/models/approval.rb                     |  1 +
 ee/app/models/approval_merge_request_rule.rb  | 38 ++++++++
 ee/app/models/approval_project_rule.rb        | 16 ++++
 ee/app/models/concerns/approval_rule_like.rb  | 46 ++++++++++
 ee/app/models/ee/merge_request.rb             |  1 +
 ee/app/models/ee/project.rb                   |  1 +
 .../20181204031328_create_approval_rules.rb   | 24 +++++
 ...4031329_create_approval_rules_approvals.rb | 28 ++++++
 ...1204031330_create_approval_rule_members.rb | 35 ++++++++
 ee/spec/factories/approval_rules.rb           | 13 +++
 .../lib/gitlab/import_export/all_models.yml   |  2 +
 .../approval_merge_request_rule_spec.rb       | 63 +++++++++++++
 ee/spec/models/approval_project_rule_spec.rb  | 30 +++++++
 .../concerns/approval_rule_like_spec.rb       | 89 +++++++++++++++++++
 15 files changed, 453 insertions(+)
 create mode 100644 ee/app/models/approval_merge_request_rule.rb
 create mode 100644 ee/app/models/approval_project_rule.rb
 create mode 100644 ee/app/models/concerns/approval_rule_like.rb
 create mode 100644 ee/db/migrate/20181204031328_create_approval_rules.rb
 create mode 100644 ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
 create mode 100644 ee/db/migrate/20181204031330_create_approval_rule_members.rb
 create mode 100644 ee/spec/factories/approval_rules.rb
 create mode 100644 ee/spec/models/approval_merge_request_rule_spec.rb
 create mode 100644 ee/spec/models/approval_project_rule_spec.rb
 create mode 100644 ee/spec/models/concerns/approval_rule_like_spec.rb

diff --git a/db/schema.rb b/db/schema.rb
index 1809ac6d95740d..3de22e8aa3f2e6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -219,6 +219,60 @@
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
   end
 
+  create_table "approval_merge_request_rules", id: :bigserial, force: :cascade do |t|
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.integer "merge_request_id", null: false
+    t.integer "approvals_required", limit: 2, default: 0, null: false
+    t.boolean "code_owner", default: false, null: false
+    t.string "name", null: false
+    t.index ["merge_request_id"], name: "index_approval_merge_request_rules_on_merge_request_id", using: :btree
+  end
+
+  create_table "approval_merge_request_rules_approvals", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_merge_request_rule_id", null: false
+    t.integer "approval_id", null: false
+    t.index ["approval_id"], name: "index_approval_merge_request_rules_approvals_2", using: :btree
+    t.index ["approval_merge_request_rule_id", "approval_id"], name: "index_approval_merge_request_rules_approvals_1", unique: true, using: :btree
+  end
+
+  create_table "approval_merge_request_rules_groups", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_merge_request_rule_id", null: false
+    t.integer "group_id", null: false
+    t.index ["approval_merge_request_rule_id", "group_id"], name: "index_approval_merge_request_rules_groups_1", unique: true, using: :btree
+    t.index ["group_id"], name: "index_approval_merge_request_rules_groups_2", using: :btree
+  end
+
+  create_table "approval_merge_request_rules_users", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_merge_request_rule_id", null: false
+    t.integer "user_id", null: false
+    t.index ["approval_merge_request_rule_id", "user_id"], name: "index_approval_merge_request_rules_users_1", unique: true, using: :btree
+    t.index ["user_id"], name: "index_approval_merge_request_rules_users_2", using: :btree
+  end
+
+  create_table "approval_project_rules", id: :bigserial, force: :cascade do |t|
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.integer "project_id", null: false
+    t.integer "approvals_required", limit: 2, default: 0, null: false
+    t.string "name", null: false
+    t.index ["project_id"], name: "index_approval_project_rules_on_project_id", using: :btree
+  end
+
+  create_table "approval_project_rules_groups", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_project_rule_id", null: false
+    t.integer "group_id", null: false
+    t.index ["approval_project_rule_id", "group_id"], name: "index_approval_project_rules_groups_1", unique: true, using: :btree
+    t.index ["group_id"], name: "index_approval_project_rules_groups_2", using: :btree
+  end
+
+  create_table "approval_project_rules_users", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_project_rule_id", null: false
+    t.integer "user_id", null: false
+    t.index ["approval_project_rule_id", "user_id"], name: "index_approval_project_rules_users_1", unique: true, using: :btree
+    t.index ["user_id"], name: "index_approval_project_rules_users_2", using: :btree
+  end
+
   create_table "approvals", force: :cascade do |t|
     t.integer "merge_request_id", null: false
     t.integer "user_id", null: false
@@ -3163,6 +3217,18 @@
   add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify
   add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify
   add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify
+  add_foreign_key "approval_merge_request_rules", "merge_requests", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_approvals", "approval_merge_request_rules", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_approvals", "approvals", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_groups", "approval_merge_request_rules", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_users", "approval_merge_request_rules", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_users", "users", on_delete: :cascade
+  add_foreign_key "approval_project_rules", "projects", on_delete: :cascade
+  add_foreign_key "approval_project_rules_groups", "approval_project_rules", on_delete: :cascade
+  add_foreign_key "approval_project_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "approval_project_rules_users", "approval_project_rules", on_delete: :cascade
+  add_foreign_key "approval_project_rules_users", "users", on_delete: :cascade
   add_foreign_key "approvals", "merge_requests", name: "fk_310d714958", on_delete: :cascade
   add_foreign_key "approver_groups", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "badges", "namespaces", column: "group_id", on_delete: :cascade
diff --git a/ee/app/models/approval.rb b/ee/app/models/approval.rb
index 3bec2b04cc1348..ef9a139310d3f0 100644
--- a/ee/app/models/approval.rb
+++ b/ee/app/models/approval.rb
@@ -3,6 +3,7 @@
 class Approval < ActiveRecord::Base
   belongs_to :user
   belongs_to :merge_request
+  has_and_belongs_to_many :approval_rules, class_name: 'ApprovalMergeRequestRule', join_table: :approval_merge_request_rules_approvals
 
   validates :merge_request_id, presence: true
   validates :user_id, presence: true, uniqueness: { scope: [:merge_request_id] }
diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
new file mode 100644
index 00000000000000..f8aebea65f02db
--- /dev/null
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class ApprovalMergeRequestRule < ApplicationRecord
+  include ApprovalRuleLike
+
+  scope :regular, -> { where(code_owner: false) }
+  scope :code_owner, -> { where(code_owner: true) } # special code owner rules, updated internally when code changes
+
+  belongs_to :merge_request
+
+  has_and_belongs_to_many :approvals # This is only populated after merge request is merged
+  has_many :approved_approvers, through: :approvals, source: :user
+
+  def project
+    merge_request.target_project
+  end
+
+  # Users who are eligible to approve, including specified group members.
+  # Excludes the author if 'self-approval' isn't explicitly
+  # enabled on project settings.
+  # @return [Array<User>]
+  def approvers
+    scope = super
+
+    if merge_request.author && !project.merge_requests_author_approval?
+      scope = scope.where.not(id: merge_request.author)
+    end
+
+    scope
+  end
+
+  def sync_approvals
+    # Before being merged, approvals are dynamically calculated instead of being persisted.
+    return unless merge_request.merged?
+
+    self.approvals = merge_request.approvals.where(user_id: approvers.map(&:id))
+  end
+end
diff --git a/ee/app/models/approval_project_rule.rb b/ee/app/models/approval_project_rule.rb
new file mode 100644
index 00000000000000..0083d9a5747998
--- /dev/null
+++ b/ee/app/models/approval_project_rule.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class ApprovalProjectRule < ApplicationRecord
+  include ApprovalRuleLike
+
+  belongs_to :project
+
+  # To allow easier duck typing
+  scope :regular, -> { all }
+  scope :code_owner, -> { none }
+
+  def code_owner
+    false
+  end
+  alias_method :code_owner?, :code_owner
+end
diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
new file mode 100644
index 00000000000000..9e77aa4ddaabcf
--- /dev/null
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module ApprovalRuleLike
+  extend ActiveSupport::Concern
+  include Gitlab::Utils::StrongMemoize
+
+  DEFAULT_NAME = 'Default'
+
+  included do
+    has_and_belongs_to_many :users
+    has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
+
+    validates :name, presence: true
+  end
+
+  # Users who are eligible to approve, including specified group members.
+  # @return [Array<User>]
+  def approvers
+    strong_memoize(:approvers) do
+      User.from_union(
+        [
+          users,
+          User.joins(:group_members).where(members: { source_id: groups })
+        ]
+      )
+    end
+  end
+
+  def add_member(member)
+    case member
+    when User
+      users << member unless users.exists?(member.id)
+    when Group
+      groups << member unless groups.exists?(member.id)
+    end
+  end
+
+  def remove_member(member)
+    case member
+    when User
+      users.delete(member)
+    when Group
+      groups.delete(member)
+    end
+  end
+end
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index c16417f1c35c25..18ff8260629d4e 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -16,6 +16,7 @@ module MergeRequest
       has_many :approved_by_users, through: :approvals, source: :user
       has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approval_rules, class_name: 'ApprovalMergeRequestRule'
       has_many :draft_notes
 
       validate :validate_approvals_before_merge, unless: :importing?
diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb
index 2f1a836f56e2a2..1f253b7b803b9f 100644
--- a/ee/app/models/ee/project.rb
+++ b/ee/app/models/ee/project.rb
@@ -44,6 +44,7 @@ module Project
       has_many :reviews, inverse_of: :project
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
       has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+      has_many :approval_rules, class_name: 'ApprovalProjectRule'
       has_many :audit_events, as: :entity
       has_many :path_locks
       has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback'
diff --git a/ee/db/migrate/20181204031328_create_approval_rules.rb b/ee/db/migrate/20181204031328_create_approval_rules.rb
new file mode 100644
index 00000000000000..1257b1a1cc1272
--- /dev/null
+++ b/ee/db/migrate/20181204031328_create_approval_rules.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class CreateApprovalRules < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :approval_project_rules, id: :bigserial do |t|
+      t.timestamps_with_timezone
+      t.references :project, index: true, foreign_key: { on_delete: :cascade }, null: false
+      t.integer :approvals_required, limit: 2, default: 0, null: false
+      t.string :name, null: false
+    end
+
+    create_table :approval_merge_request_rules, id: :bigserial do |t|
+      t.timestamps_with_timezone
+      t.references :merge_request, index: true, foreign_key: { on_delete: :cascade }, null: false
+      t.integer :approvals_required, limit: 2, default: 0, null: false
+      t.boolean :code_owner, default: false, null: false
+      t.string :name, null: false
+    end
+  end
+end
diff --git a/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
new file mode 100644
index 00000000000000..da74f20918fce2
--- /dev/null
+++ b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+class CreateApprovalRulesApprovals < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table(:approval_merge_request_rules_approvals, id: :bigserial) do |t|
+      t.references(
+        :approval_merge_request_rule,
+        type: :bigint,
+        null: false,
+        foreign_key: { on_delete: :cascade },
+        index: false
+      )
+      t.references(
+        :approval,
+        type: :integer,
+        null: false,
+        foreign_key: { on_delete: :cascade },
+        index: { name: 'index_approval_merge_request_rules_approvals_2' }
+      )
+
+      t.index [:approval_merge_request_rule_id, :approval_id], unique: true, name: 'index_approval_merge_request_rules_approvals_1'
+    end
+  end
+end
diff --git a/ee/db/migrate/20181204031330_create_approval_rule_members.rb b/ee/db/migrate/20181204031330_create_approval_rule_members.rb
new file mode 100644
index 00000000000000..5ef159c7a630ae
--- /dev/null
+++ b/ee/db/migrate/20181204031330_create_approval_rule_members.rb
@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+class CreateApprovalRuleMembers < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  TABLES = [
+    { table: 'approval_merge_request_rules_users',  rule: 'approval_merge_request_rule', member: 'user',  member_table: 'users' },
+    { table: 'approval_merge_request_rules_groups', rule: 'approval_merge_request_rule', member: 'group', member_table: 'namespaces' },
+    { table: 'approval_project_rules_users',        rule: 'approval_project_rule',       member: 'user',  member_table: 'users' },
+    { table: 'approval_project_rules_groups',       rule: 'approval_project_rule',       member: 'group', member_table: 'namespaces' }
+  ].freeze
+
+  def up
+    TABLES.each do |params|
+      member_id = "#{params[:member]}_id"
+      rule_id = "#{params[:rule]}_id"
+
+      create_table(params[:table], id: :bigserial) do |t|
+        t.references params[:rule],   null: false, type: :bigint,  index: false, foreign_key: { on_delete: :cascade }
+        t.references params[:member], null: false, type: :integer, index: { name: "index_#{params[:table]}_2" }
+
+        # To accommodate Group being in the `namespaces` table
+        t.foreign_key params[:member_table], column: member_id, on_delete: :cascade
+
+        t.index [rule_id, member_id], unique: true, name: "index_#{params[:table]}_1"
+      end
+    end
+  end
+
+  def down
+    TABLES.each { |params| drop_table(params[:table]) }
+  end
+end
diff --git a/ee/spec/factories/approval_rules.rb b/ee/spec/factories/approval_rules.rb
new file mode 100644
index 00000000000000..7c6a1d80dd566f
--- /dev/null
+++ b/ee/spec/factories/approval_rules.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :approval_merge_request_rule do
+    merge_request
+    name ApprovalRuleLike::DEFAULT_NAME
+  end
+
+  factory :approval_project_rule do
+    project
+    name ApprovalRuleLike::DEFAULT_NAME
+  end
+end
diff --git a/ee/spec/lib/gitlab/import_export/all_models.yml b/ee/spec/lib/gitlab/import_export/all_models.yml
index b7f300bca8f2c5..b683afb892eed1 100644
--- a/ee/spec/lib/gitlab/import_export/all_models.yml
+++ b/ee/spec/lib/gitlab/import_export/all_models.yml
@@ -6,6 +6,7 @@ milestone:
 - boards
 merge_requests:
 - reviews
+- approval_rules
 - approvals
 - approvers
 - approver_groups
@@ -50,6 +51,7 @@ project:
 - jenkins_service
 - jenkins_deprecated_service
 - index_status
+- approval_rules
 - approvers
 - pages_domains
 - audit_events
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
new file mode 100644
index 00000000000000..d4b17f52794fc2
--- /dev/null
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -0,0 +1,63 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ApprovalMergeRequestRule, type: :model do
+  let(:merge_request) { create(:merge_request) }
+
+  subject { create(:approval_merge_request_rule, merge_request: merge_request) }
+
+  describe '#project' do
+    it 'returns project of MergeRequest' do
+      expect(subject.project).to eq(merge_request.project)
+    end
+  end
+
+  describe '#approvers' do
+    context 'when project setting includes author' do
+      before do
+        merge_request.target_project.update(merge_requests_author_approval: true)
+
+        create(:group) do |group|
+          group.add_guest(merge_request.author)
+          subject.groups << group
+        end
+      end
+
+      it 'contains author' do
+        expect(subject.approvers).to contain_exactly(merge_request.author)
+      end
+    end
+  end
+
+  describe '#sync_approvals' do
+    let(:member1) { create(:user) }
+    let(:member2) { create(:user) }
+    let(:member3) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: merge_request, user: member1) }
+    let!(:approval2) { create(:approval, merge_request: merge_request, user: member2) }
+    let!(:approval3) { create(:approval, merge_request: merge_request, user: member3) }
+
+    before do
+      subject.users = [member1, member2]
+    end
+
+    context 'when not merged' do
+      it 'does nothing' do
+        subject.sync_approvals
+
+        expect(subject.approvals).to be_empty
+      end
+    end
+
+    context 'when merged' do
+      let(:merge_request) { create(:merged_merge_request) }
+
+      it 'updates mapping' do
+        subject.sync_approvals
+
+        expect(subject.approvals.reload).to contain_exactly(approval1, approval2)
+      end
+    end
+  end
+end
diff --git a/ee/spec/models/approval_project_rule_spec.rb b/ee/spec/models/approval_project_rule_spec.rb
new file mode 100644
index 00000000000000..4410b18e00a851
--- /dev/null
+++ b/ee/spec/models/approval_project_rule_spec.rb
@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalProjectRule do
+  subject { create(:approval_project_rule) }
+
+  describe '.regular' do
+    it 'returns all records' do
+      rules = create_list(:approval_project_rule, 2)
+
+      expect(described_class.regular).to contain_exactly(*rules)
+    end
+  end
+
+  describe '.code_ownerscope' do
+    it 'returns nothing' do
+      create_list(:approval_project_rule, 2)
+
+      expect(described_class.code_owner).to be_empty
+    end
+  end
+
+  describe '#code_owner' do
+    it 'returns false' do
+      expect(subject.code_owner).to eq(false)
+      expect(subject.code_owner?).to eq(false)
+    end
+  end
+end
diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb
new file mode 100644
index 00000000000000..daab80e779a057
--- /dev/null
+++ b/ee/spec/models/concerns/approval_rule_like_spec.rb
@@ -0,0 +1,89 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ApprovalRuleLike, type: :model do
+  let(:member1) { create(:user) }
+  let(:member2) { create(:user) }
+  let(:member3) { create(:user) }
+  let(:group1) { create(:group) }
+  let(:group2) { create(:group) }
+
+  let(:merge_request) { create(:merge_request) }
+
+  shared_examples 'approval rule like' do
+    describe '#add_member' do
+      it 'adds as a member of the rule' do
+        expect do
+          subject.add_member(member1)
+        end.to change { subject.users.count }.by(1)
+      end
+
+      it 'does nothing if already a member' do
+        subject.add_member(member1)
+
+        expect do
+          subject.add_member(member1)
+        end.not_to change { subject.users.count }
+      end
+    end
+
+    describe '#remove_member' do
+      it 'removes a member from the rule' do
+        subject.add_member(group1)
+
+        expect do
+          subject.remove_member(group1)
+        end.to change { subject.groups.count }.by(-1)
+      end
+
+      it 'does nothing if not a member' do
+        expect do
+          subject.remove_member(group1)
+        end.not_to change { subject.groups.count }
+      end
+    end
+
+    describe '#approvers' do
+      let(:group1_member1) { create(:user) }
+      let(:group2_member1) { create(:user) }
+
+      before do
+        subject.users << member1
+        subject.users << member2
+        subject.groups << group1
+        subject.groups << group2
+
+        group1.add_guest(group1_member1)
+        group2.add_guest(group2_member1)
+      end
+
+      it 'contains users as direct members and group members' do
+        expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+      end
+
+      context 'when user is both a direct member and a group member' do
+        before do
+          group1.add_guest(member1)
+          group2.add_guest(member2)
+        end
+
+        it 'contains only unique users' do
+          expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+        end
+      end
+    end
+  end
+
+  context 'MergeRequest' do
+    subject { create(:approval_merge_request_rule, merge_request: merge_request) }
+
+    it_behaves_like 'approval rule like'
+  end
+
+  context 'Project' do
+    subject { create(:approval_project_rule) }
+
+    it_behaves_like 'approval rule like'
+  end
+end
-- 
GitLab


From 9f062ceb2e58d9a75fd9960266d39d64b7e162fb Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 5 Dec 2018 18:18:50 +0800
Subject: [PATCH 2/9] Allow MR to finalize the approval state when merged
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

First the project rule is cloned if not overwritten in the MR.
The MR’s rule to approval mapping is then persisted.
---
 ee/app/models/ee/merge_request.rb    | 18 +++++
 ee/spec/models/merge_request_spec.rb | 99 ++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index 18ff8260629d4e..d8448bf4f0cddb 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -81,5 +81,23 @@ def compare_license_management_reports
 
       compare_reports(::Ci::CompareLicenseManagementReportsService)
     end
+
+    def finalize_approvals
+      return unless merged?
+
+      copy_project_approval_rules unless approval_rules.regular.exists?
+
+      approval_rules.each(&:sync_approvals)
+    end
+
+    private
+
+    def copy_project_approval_rules
+      target_project.approval_rules.each do |project_rule|
+        rule = approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
+        rule.users = project_rule.users
+        rule.groups = project_rule.groups
+      end
+    end
   end
 end
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index 309ac585fd0d12..ab5337d8aba6f3 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -37,6 +37,105 @@
     end
   end
 
+  describe '#finalize_approvals' do
+    let!(:member1) { create(:user) }
+    let!(:member2) { create(:user) }
+    let!(:member3) { create(:user) }
+    let!(:group1) { create(:group) }
+    let!(:group2) { create(:group) }
+    let!(:group1_member) { create(:user) }
+    let!(:group2_member) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: subject, user: member1) }
+    let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
+    let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
+    let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
+
+    before do
+      group1.add_guest(group1_member)
+      group2.add_guest(group2_member)
+
+      rule = create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12)
+
+      rule.users = [member1, member2]
+      rule.groups << group1
+    end
+
+    shared_examples 'skipping when unmerged' do
+      it 'does nothing if unmerged' do
+        expect do
+          subject.finalize_approvals
+        end.not_to change { ApprovalMergeRequestRule.count }
+
+        expect(approval1.approval_rules).to be_empty
+        expect(approval2.approval_rules).to be_empty
+        expect(approval3.approval_rules).to be_empty
+        expect(approval4.approval_rules).to be_empty
+      end
+    end
+
+    context 'when project rule is not overwritten' do
+      it_behaves_like 'skipping when unmerged'
+
+      context 'when merged' do
+        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        before do
+          subject.approval_rules.code_owner.create(name: 'Code Owner')
+        end
+
+        it 'copies project rules to MR' do
+          expect do
+            subject.finalize_approvals
+          end.to change { ApprovalMergeRequestRule.count }.by(1)
+
+          rule = subject.approval_rules.regular.first
+
+          expect(rule.name).to eq('foo')
+          expect(rule.approvals_required).to eq(12)
+          expect(rule.users).to contain_exactly(member1, member2)
+          expect(rule.groups).to contain_exactly(group1)
+          expect(approval1.approval_rules).to contain_exactly(rule)
+          expect(approval2.approval_rules).to be_empty
+          expect(approval3.approval_rules).to contain_exactly(rule)
+          expect(approval4.approval_rules).to be_empty
+        end
+      end
+    end
+
+    context 'when project rule is overwritten' do
+      before do
+        rule = create(:approval_merge_request_rule, merge_request: subject, name: 'bar', approvals_required: 32)
+        rule.users = [member2, member3]
+        rule.groups << group2
+      end
+
+      it_behaves_like 'skipping when unmerged'
+
+      context 'when merged' do
+        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        it 'does not copy project rules, and updates approval mapping with MR rules' do
+          expect(subject).not_to receive(:copy_project_approval_rules)
+
+          expect do
+            subject.finalize_approvals
+          end.not_to change { ApprovalMergeRequestRule.count }
+
+          rule = subject.approval_rules.regular.first
+
+          expect(rule.name).to eq('bar')
+          expect(rule.approvals_required).to eq(32)
+          expect(rule.users).to contain_exactly(member2, member3)
+          expect(rule.groups).to contain_exactly(group2)
+          expect(approval1.approval_rules).to be_empty
+          expect(approval2.approval_rules).to contain_exactly(rule)
+          expect(approval3.approval_rules).to be_empty
+          expect(approval4.approval_rules).to contain_exactly(rule)
+        end
+      end
+    end
+  end
+
   describe '#code_owners' do
     subject(:merge_request) { build(:merge_request) }
     let(:owners) { [double(:owner)] }
-- 
GitLab


From 7abd6894e17daf5ce93ecc730b44e51d09cddbcc Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Tue, 18 Dec 2018 12:02:29 +0800
Subject: [PATCH 3/9] Allow MR rule to store its source project rule

This is to enable MR override rule's `approvals_required` is never
less than the project rule's.
---
 db/schema.rb                                  |  9 +++++++
 ee/app/models/approval_merge_request_rule.rb  |  2 ++
 .../approval_merge_request_rule_source.rb     |  7 +++++
 ...ge_request_rules_approval_project_rules.rb | 26 +++++++++++++++++++
 ee/spec/models/merge_request_spec.rb          |  7 +++--
 5 files changed, 47 insertions(+), 4 deletions(-)
 create mode 100644 ee/app/models/approval_merge_request_rule_source.rb
 create mode 100644 ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb

diff --git a/db/schema.rb b/db/schema.rb
index 3de22e8aa3f2e6..fc0f8d32e87e53 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -219,6 +219,13 @@
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree
   end
 
+  create_table "approval_merge_request_rule_sources", id: :bigserial, force: :cascade do |t|
+    t.bigint "approval_merge_request_rule_id", null: false
+    t.bigint "approval_project_rule_id", null: false
+    t.index ["approval_merge_request_rule_id"], name: "index_approval_merge_request_rule_sources_1", unique: true, using: :btree
+    t.index ["approval_project_rule_id"], name: "index_approval_merge_request_rule_sources_2", using: :btree
+  end
+
   create_table "approval_merge_request_rules", id: :bigserial, force: :cascade do |t|
     t.datetime_with_timezone "created_at", null: false
     t.datetime_with_timezone "updated_at", null: false
@@ -3217,6 +3224,8 @@
   add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify
   add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify
   add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify
+  add_foreign_key "approval_merge_request_rule_sources", "approval_merge_request_rules", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rule_sources", "approval_project_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules", "merge_requests", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules_approvals", "approval_merge_request_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules_approvals", "approvals", on_delete: :cascade
diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index f8aebea65f02db..7aaa7ee5930a97 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -10,6 +10,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   has_and_belongs_to_many :approvals # This is only populated after merge request is merged
   has_many :approved_approvers, through: :approvals, source: :user
+  has_one :approval_merge_request_rule_source
+  has_one :approval_project_rule, through: :approval_merge_request_rule_source
 
   def project
     merge_request.target_project
diff --git a/ee/app/models/approval_merge_request_rule_source.rb b/ee/app/models/approval_merge_request_rule_source.rb
new file mode 100644
index 00000000000000..bd4b05b655a759
--- /dev/null
+++ b/ee/app/models/approval_merge_request_rule_source.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+# Allow MR rule to lookup its project rule source
+class ApprovalMergeRequestRuleSource < ApplicationRecord
+  belongs_to :approval_merge_request_rule
+  belongs_to :approval_project_rule
+end
diff --git a/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb b/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb
new file mode 100644
index 00000000000000..2190fd65e13b4b
--- /dev/null
+++ b/ee/db/migrate/20181204031331_create_approval_merge_request_rules_approval_project_rules.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class CreateApprovalMergeRequestRulesApprovalProjectRules < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table(:approval_merge_request_rule_sources, id: :bigserial) do |t|
+      t.references(
+        :approval_merge_request_rule,
+        type: :bigint,
+        null: false,
+        foreign_key: { on_delete: :cascade },
+        index: { name: 'index_approval_merge_request_rule_sources_1', unique: true }
+      )
+      t.references(
+        :approval_project_rule,
+        type: :bigint,
+        null: false,
+        foreign_key: { on_delete: :cascade },
+        index: { name: 'index_approval_merge_request_rule_sources_2' }
+      )
+    end
+  end
+end
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index ab5337d8aba6f3..36ad72508ea4dc 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -49,15 +49,14 @@
     let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
     let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
     let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
+    let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
 
     before do
       group1.add_guest(group1_member)
       group2.add_guest(group2_member)
 
-      rule = create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12)
-
-      rule.users = [member1, member2]
-      rule.groups << group1
+      project_rule.users = [member1, member2]
+      project_rule.groups << group1
     end
 
     shared_examples 'skipping when unmerged' do
-- 
GitLab


From 66d97a471f203601c7af969dc137ce2b967763c5 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 3 Dec 2018 18:12:23 +0800
Subject: [PATCH 4/9] Allow duck-type access to member

---
 ee/app/models/approver.rb       | 4 ++++
 ee/app/models/approver_group.rb | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/ee/app/models/approver.rb b/ee/app/models/approver.rb
index 07d9dc5ed9883e..9bf17aca25fab1 100644
--- a/ee/app/models/approver.rb
+++ b/ee/app/models/approver.rb
@@ -9,4 +9,8 @@ class Approver < ActiveRecord::Base
   def find_by_user_id(user_id)
     find_by(user_id: user_id)
   end
+
+  def member
+    user
+  end
 end
diff --git a/ee/app/models/approver_group.rb b/ee/app/models/approver_group.rb
index 965a67391286a5..d1177657361d48 100644
--- a/ee/app/models/approver_group.rb
+++ b/ee/app/models/approver_group.rb
@@ -13,4 +13,8 @@ def self.filtered_approver_groups(approver_groups, user)
 
     approver_groups.joins(:group).merge(public_or_visible_groups)
   end
+
+  def member
+    group
+  end
 end
-- 
GitLab


From 3f51e05929be52a60381c833531cfced7f1db55c Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Thu, 27 Dec 2018 20:02:44 +0800
Subject: [PATCH 5/9] Index on code_owner

Code owner rules are queries on its own.
---
 db/schema.rb                                          | 2 +-
 ee/db/migrate/20181204031328_create_approval_rules.rb | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/db/schema.rb b/db/schema.rb
index fc0f8d32e87e53..3320dfd099b8df 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -233,7 +233,7 @@
     t.integer "approvals_required", limit: 2, default: 0, null: false
     t.boolean "code_owner", default: false, null: false
     t.string "name", null: false
-    t.index ["merge_request_id"], name: "index_approval_merge_request_rules_on_merge_request_id", using: :btree
+    t.index ["merge_request_id", "code_owner"], name: "index_approval_merge_request_rules_1", using: :btree
   end
 
   create_table "approval_merge_request_rules_approvals", id: :bigserial, force: :cascade do |t|
diff --git a/ee/db/migrate/20181204031328_create_approval_rules.rb b/ee/db/migrate/20181204031328_create_approval_rules.rb
index 1257b1a1cc1272..27df6a077360cb 100644
--- a/ee/db/migrate/20181204031328_create_approval_rules.rb
+++ b/ee/db/migrate/20181204031328_create_approval_rules.rb
@@ -15,10 +15,12 @@ def change
 
     create_table :approval_merge_request_rules, id: :bigserial do |t|
       t.timestamps_with_timezone
-      t.references :merge_request, index: true, foreign_key: { on_delete: :cascade }, null: false
+      t.references :merge_request, index: false, foreign_key: { on_delete: :cascade }, null: false
       t.integer :approvals_required, limit: 2, default: 0, null: false
       t.boolean :code_owner, default: false, null: false
       t.string :name, null: false
+
+      t.index [:merge_request_id, :code_owner], name: 'index_approval_merge_request_rules_1'
     end
   end
 end
-- 
GitLab


From 2c1c67a0e1478ad705d9394abd772a75fce4a4fb Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 2 Jan 2019 16:56:42 +0800
Subject: [PATCH 6/9] Extract finalize_approvals as service

Keep snapshot of group member by persisting as explicit approver,
because group member can change after MR is merged.

Filter out inaccessible groups from MR author.
---
 ee/app/models/concerns/approval_rule_like.rb  |   1 +
 ee/app/models/ee/merge_request.rb             |  18 ---
 .../approval_rules/finalize_service.rb        |  39 +++++++
 ee/spec/models/merge_request_spec.rb          |  98 ----------------
 .../approval_rules/finalize_service_spec.rb   | 108 ++++++++++++++++++
 5 files changed, 148 insertions(+), 116 deletions(-)
 create mode 100644 ee/app/services/approval_rules/finalize_service.rb
 create mode 100644 ee/spec/services/approval_rules/finalize_service_spec.rb

diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index 9e77aa4ddaabcf..c20d74085900e6 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -9,6 +9,7 @@ module ApprovalRuleLike
   included do
     has_and_belongs_to_many :users
     has_and_belongs_to_many :groups, class_name: 'Group', join_table: "#{self.table_name}_groups"
+    has_many :group_users, through: :groups, source: :users
 
     validates :name, presence: true
   end
diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index d8448bf4f0cddb..18ff8260629d4e 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -81,23 +81,5 @@ def compare_license_management_reports
 
       compare_reports(::Ci::CompareLicenseManagementReportsService)
     end
-
-    def finalize_approvals
-      return unless merged?
-
-      copy_project_approval_rules unless approval_rules.regular.exists?
-
-      approval_rules.each(&:sync_approvals)
-    end
-
-    private
-
-    def copy_project_approval_rules
-      target_project.approval_rules.each do |project_rule|
-        rule = approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
-        rule.users = project_rule.users
-        rule.groups = project_rule.groups
-      end
-    end
   end
 end
diff --git a/ee/app/services/approval_rules/finalize_service.rb b/ee/app/services/approval_rules/finalize_service.rb
new file mode 100644
index 00000000000000..58c70118088e06
--- /dev/null
+++ b/ee/app/services/approval_rules/finalize_service.rb
@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+module ApprovalRules
+  class FinalizeService
+    attr_reader :merge_request
+
+    def initialize(merge_request)
+      @merge_request = merge_request
+    end
+
+    def execute
+      return unless merge_request.merged?
+
+      if merge_request.approval_rules.regular.exists?
+        merge_group_members_into_users
+      else
+        copy_project_approval_rules
+      end
+
+      merge_request.approval_rules.each(&:sync_approvals)
+    end
+
+    private
+
+    def merge_group_members_into_users
+      merge_request.approval_rules.each do |rule|
+        rule.users += rule.group_users
+      end
+    end
+
+    def copy_project_approval_rules
+      merge_request.target_project.approval_rules.each do |project_rule|
+        rule = merge_request.approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
+        rule.users = project_rule.approvers
+        rule.groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
+      end
+    end
+  end
+end
diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb
index 36ad72508ea4dc..309ac585fd0d12 100644
--- a/ee/spec/models/merge_request_spec.rb
+++ b/ee/spec/models/merge_request_spec.rb
@@ -37,104 +37,6 @@
     end
   end
 
-  describe '#finalize_approvals' do
-    let!(:member1) { create(:user) }
-    let!(:member2) { create(:user) }
-    let!(:member3) { create(:user) }
-    let!(:group1) { create(:group) }
-    let!(:group2) { create(:group) }
-    let!(:group1_member) { create(:user) }
-    let!(:group2_member) { create(:user) }
-    let!(:approval1) { create(:approval, merge_request: subject, user: member1) }
-    let!(:approval2) { create(:approval, merge_request: subject, user: member3) }
-    let!(:approval3) { create(:approval, merge_request: subject, user: group1_member) }
-    let!(:approval4) { create(:approval, merge_request: subject, user: group2_member) }
-    let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
-
-    before do
-      group1.add_guest(group1_member)
-      group2.add_guest(group2_member)
-
-      project_rule.users = [member1, member2]
-      project_rule.groups << group1
-    end
-
-    shared_examples 'skipping when unmerged' do
-      it 'does nothing if unmerged' do
-        expect do
-          subject.finalize_approvals
-        end.not_to change { ApprovalMergeRequestRule.count }
-
-        expect(approval1.approval_rules).to be_empty
-        expect(approval2.approval_rules).to be_empty
-        expect(approval3.approval_rules).to be_empty
-        expect(approval4.approval_rules).to be_empty
-      end
-    end
-
-    context 'when project rule is not overwritten' do
-      it_behaves_like 'skipping when unmerged'
-
-      context 'when merged' do
-        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
-
-        before do
-          subject.approval_rules.code_owner.create(name: 'Code Owner')
-        end
-
-        it 'copies project rules to MR' do
-          expect do
-            subject.finalize_approvals
-          end.to change { ApprovalMergeRequestRule.count }.by(1)
-
-          rule = subject.approval_rules.regular.first
-
-          expect(rule.name).to eq('foo')
-          expect(rule.approvals_required).to eq(12)
-          expect(rule.users).to contain_exactly(member1, member2)
-          expect(rule.groups).to contain_exactly(group1)
-          expect(approval1.approval_rules).to contain_exactly(rule)
-          expect(approval2.approval_rules).to be_empty
-          expect(approval3.approval_rules).to contain_exactly(rule)
-          expect(approval4.approval_rules).to be_empty
-        end
-      end
-    end
-
-    context 'when project rule is overwritten' do
-      before do
-        rule = create(:approval_merge_request_rule, merge_request: subject, name: 'bar', approvals_required: 32)
-        rule.users = [member2, member3]
-        rule.groups << group2
-      end
-
-      it_behaves_like 'skipping when unmerged'
-
-      context 'when merged' do
-        subject(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
-
-        it 'does not copy project rules, and updates approval mapping with MR rules' do
-          expect(subject).not_to receive(:copy_project_approval_rules)
-
-          expect do
-            subject.finalize_approvals
-          end.not_to change { ApprovalMergeRequestRule.count }
-
-          rule = subject.approval_rules.regular.first
-
-          expect(rule.name).to eq('bar')
-          expect(rule.approvals_required).to eq(32)
-          expect(rule.users).to contain_exactly(member2, member3)
-          expect(rule.groups).to contain_exactly(group2)
-          expect(approval1.approval_rules).to be_empty
-          expect(approval2.approval_rules).to contain_exactly(rule)
-          expect(approval3.approval_rules).to be_empty
-          expect(approval4.approval_rules).to contain_exactly(rule)
-        end
-      end
-    end
-  end
-
   describe '#code_owners' do
     subject(:merge_request) { build(:merge_request) }
     let(:owners) { [double(:owner)] }
diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb
new file mode 100644
index 00000000000000..58de3f28685a34
--- /dev/null
+++ b/ee/spec/services/approval_rules/finalize_service_spec.rb
@@ -0,0 +1,108 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::FinalizeService do
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+  describe '#execute' do
+    let!(:member1) { create(:user) }
+    let!(:member2) { create(:user) }
+    let!(:member3) { create(:user) }
+    let!(:group1) { create(:group) }
+    let!(:group2) { create(:group) }
+    let!(:group1_member) { create(:user) }
+    let!(:group2_member) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: merge_request, user: member1) }
+    let!(:approval2) { create(:approval, merge_request: merge_request, user: member3) }
+    let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_member) }
+    let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_member) }
+    let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
+
+    subject { described_class.new(merge_request) }
+
+    before do
+      group1.add_guest(group1_member)
+      group2.add_guest(group2_member)
+
+      project_rule.users = [member1, member2]
+      project_rule.groups << group1
+    end
+
+    shared_examples 'skipping when unmerged' do
+      it 'does nothing if unmerged' do
+        expect do
+          subject.execute
+        end.not_to change { ApprovalMergeRequestRule.count }
+
+        expect(approval1.approval_rules).to be_empty
+        expect(approval2.approval_rules).to be_empty
+        expect(approval3.approval_rules).to be_empty
+        expect(approval4.approval_rules).to be_empty
+      end
+    end
+
+    context 'when project rule is not overwritten' do
+      it_behaves_like 'skipping when unmerged'
+
+      context 'when merged' do
+        let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        before do
+          merge_request.approval_rules.code_owner.create(name: 'Code Owner')
+        end
+
+        it 'copies project rules to MR, keep snapshot of group member by including it as part of users association' do
+          expect do
+            subject.execute
+          end.to change { ApprovalMergeRequestRule.count }.by(1)
+
+          rule = merge_request.approval_rules.regular.first
+
+          expect(rule.name).to eq('foo')
+          expect(rule.approvals_required).to eq(12)
+          expect(rule.users).to contain_exactly(member1, member2, group1_member)
+          expect(rule.groups).to contain_exactly(group1)
+          expect(approval1.approval_rules).to contain_exactly(rule)
+          expect(approval2.approval_rules).to be_empty
+          expect(approval3.approval_rules).to contain_exactly(rule)
+          expect(approval4.approval_rules).to be_empty
+        end
+      end
+    end
+
+    context 'when project rule is overwritten' do
+      before do
+        rule = create(:approval_merge_request_rule, merge_request: merge_request, name: 'bar', approvals_required: 32)
+        rule.users = [member2, member3]
+        rule.groups << group2
+      end
+
+      it_behaves_like 'skipping when unmerged'
+
+      context 'when merged' do
+        let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
+
+        it 'does not copy project rules, and updates approval mapping with MR rules' do
+          expect(merge_request).not_to receive(:copy_project_approval_rules)
+
+          expect do
+            subject.execute
+          end.not_to change { ApprovalMergeRequestRule.count }
+
+          rule = merge_request.approval_rules.regular.first
+
+          expect(rule.name).to eq('bar')
+          expect(rule.approvals_required).to eq(32)
+          expect(rule.users).to contain_exactly(member2, member3, group2_member)
+          expect(rule.groups).to contain_exactly(group2)
+          expect(approval1.approval_rules).to be_empty
+          expect(approval2.approval_rules).to contain_exactly(rule)
+          expect(approval3.approval_rules).to be_empty
+          expect(approval4.approval_rules).to contain_exactly(rule)
+        end
+      end
+    end
+  end
+end
-- 
GitLab


From 72c9b8a972a2a6033357ed13eb087c5328c3bf65 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Wed, 2 Jan 2019 19:14:13 +0800
Subject: [PATCH 7/9] Refactor approver query

Utilze the new group_users association
---
 ee/app/models/concerns/approval_rule_like.rb | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index c20d74085900e6..bb4826d622725e 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -2,7 +2,6 @@
 
 module ApprovalRuleLike
   extend ActiveSupport::Concern
-  include Gitlab::Utils::StrongMemoize
 
   DEFAULT_NAME = 'Default'
 
@@ -17,14 +16,7 @@ module ApprovalRuleLike
   # Users who are eligible to approve, including specified group members.
   # @return [Array<User>]
   def approvers
-    strong_memoize(:approvers) do
-      User.from_union(
-        [
-          users,
-          User.joins(:group_members).where(members: { source_id: groups })
-        ]
-      )
-    end
+    @approvers ||= User.from_union([users, group_users])
   end
 
   def add_member(member)
-- 
GitLab


From 7f80b6ed05275abcc5323d767131b6f357162cf9 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Sat, 5 Jan 2019 16:06:47 +0800
Subject: [PATCH 8/9] Change to map MR rule to approved approvers directly

This is more direct in loggin who approved a rule.
---
 db/schema.rb                                    | 12 ++++++------
 ee/app/models/approval.rb                       |  1 -
 ee/app/models/approval_merge_request_rule.rb    | 10 +++++-----
 .../services/approval_rules/finalize_service.rb |  2 +-
 ...204031329_create_approval_rules_approvals.rb |  8 ++++----
 .../models/approval_merge_request_rule_spec.rb  | 10 +++++-----
 .../approval_rules/finalize_service_spec.rb     | 17 ++++-------------
 7 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/db/schema.rb b/db/schema.rb
index 3320dfd099b8df..65ab91c98726b2 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -236,11 +236,11 @@
     t.index ["merge_request_id", "code_owner"], name: "index_approval_merge_request_rules_1", using: :btree
   end
 
-  create_table "approval_merge_request_rules_approvals", id: :bigserial, force: :cascade do |t|
+  create_table "approval_merge_request_rules_approved_approvers", id: :bigserial, force: :cascade do |t|
     t.bigint "approval_merge_request_rule_id", null: false
-    t.integer "approval_id", null: false
-    t.index ["approval_id"], name: "index_approval_merge_request_rules_approvals_2", using: :btree
-    t.index ["approval_merge_request_rule_id", "approval_id"], name: "index_approval_merge_request_rules_approvals_1", unique: true, using: :btree
+    t.integer "user_id", null: false
+    t.index ["approval_merge_request_rule_id", "user_id"], name: "index_approval_merge_request_rules_approved_approvers_1", unique: true, using: :btree
+    t.index ["user_id"], name: "index_approval_merge_request_rules_approved_approvers_2", using: :btree
   end
 
   create_table "approval_merge_request_rules_groups", id: :bigserial, force: :cascade do |t|
@@ -3227,8 +3227,8 @@
   add_foreign_key "approval_merge_request_rule_sources", "approval_merge_request_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rule_sources", "approval_project_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules", "merge_requests", on_delete: :cascade
-  add_foreign_key "approval_merge_request_rules_approvals", "approval_merge_request_rules", on_delete: :cascade
-  add_foreign_key "approval_merge_request_rules_approvals", "approvals", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_approved_approvers", "approval_merge_request_rules", on_delete: :cascade
+  add_foreign_key "approval_merge_request_rules_approved_approvers", "users", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules_groups", "approval_merge_request_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules_groups", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "approval_merge_request_rules_users", "approval_merge_request_rules", on_delete: :cascade
diff --git a/ee/app/models/approval.rb b/ee/app/models/approval.rb
index ef9a139310d3f0..3bec2b04cc1348 100644
--- a/ee/app/models/approval.rb
+++ b/ee/app/models/approval.rb
@@ -3,7 +3,6 @@
 class Approval < ActiveRecord::Base
   belongs_to :user
   belongs_to :merge_request
-  has_and_belongs_to_many :approval_rules, class_name: 'ApprovalMergeRequestRule', join_table: :approval_merge_request_rules_approvals
 
   validates :merge_request_id, presence: true
   validates :user_id, presence: true, uniqueness: { scope: [:merge_request_id] }
diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 7aaa7ee5930a97..ec9e69060dc3d6 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -8,8 +8,8 @@ class ApprovalMergeRequestRule < ApplicationRecord
 
   belongs_to :merge_request
 
-  has_and_belongs_to_many :approvals # This is only populated after merge request is merged
-  has_many :approved_approvers, through: :approvals, source: :user
+  # approved_approvers is only populated after MR is merged
+  has_and_belongs_to_many :approved_approvers, class_name: 'User', join_table: :approval_merge_request_rules_approved_approvers
   has_one :approval_merge_request_rule_source
   has_one :approval_project_rule, through: :approval_merge_request_rule_source
 
@@ -31,10 +31,10 @@ def approvers
     scope
   end
 
-  def sync_approvals
-    # Before being merged, approvals are dynamically calculated instead of being persisted.
+  def sync_approved_approvers
+    # Before being merged, approved_approvers are dynamically calculated in ApprovalWrappedRule instead of being persisted.
     return unless merge_request.merged?
 
-    self.approvals = merge_request.approvals.where(user_id: approvers.map(&:id))
+    self.approved_approver_ids = merge_request.approvals.map(&:user_id) & approvers.map(&:id)
   end
 end
diff --git a/ee/app/services/approval_rules/finalize_service.rb b/ee/app/services/approval_rules/finalize_service.rb
index 58c70118088e06..22031d64cc8cdf 100644
--- a/ee/app/services/approval_rules/finalize_service.rb
+++ b/ee/app/services/approval_rules/finalize_service.rb
@@ -17,7 +17,7 @@ def execute
         copy_project_approval_rules
       end
 
-      merge_request.approval_rules.each(&:sync_approvals)
+      merge_request.approval_rules.each(&:sync_approved_approvers)
     end
 
     private
diff --git a/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
index da74f20918fce2..ff18e58f278ed0 100644
--- a/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
+++ b/ee/db/migrate/20181204031329_create_approval_rules_approvals.rb
@@ -6,7 +6,7 @@ class CreateApprovalRulesApprovals < ActiveRecord::Migration[5.0]
   DOWNTIME = false
 
   def change
-    create_table(:approval_merge_request_rules_approvals, id: :bigserial) do |t|
+    create_table(:approval_merge_request_rules_approved_approvers, id: :bigserial) do |t|
       t.references(
         :approval_merge_request_rule,
         type: :bigint,
@@ -15,14 +15,14 @@ def change
         index: false
       )
       t.references(
-        :approval,
+        :user,
         type: :integer,
         null: false,
         foreign_key: { on_delete: :cascade },
-        index: { name: 'index_approval_merge_request_rules_approvals_2' }
+        index: { name: 'index_approval_merge_request_rules_approved_approvers_2' }
       )
 
-      t.index [:approval_merge_request_rule_id, :approval_id], unique: true, name: 'index_approval_merge_request_rules_approvals_1'
+      t.index [:approval_merge_request_rule_id, :user_id], unique: true, name: 'index_approval_merge_request_rules_approved_approvers_1'
     end
   end
 end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index d4b17f52794fc2..6253ae9159639e 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -30,7 +30,7 @@
     end
   end
 
-  describe '#sync_approvals' do
+  describe '#sync_approved_approvers' do
     let(:member1) { create(:user) }
     let(:member2) { create(:user) }
     let(:member3) { create(:user) }
@@ -44,9 +44,9 @@
 
     context 'when not merged' do
       it 'does nothing' do
-        subject.sync_approvals
+        subject.sync_approved_approvers
 
-        expect(subject.approvals).to be_empty
+        expect(subject.approved_approvers).to be_empty
       end
     end
 
@@ -54,9 +54,9 @@
       let(:merge_request) { create(:merged_merge_request) }
 
       it 'updates mapping' do
-        subject.sync_approvals
+        subject.sync_approved_approvers
 
-        expect(subject.approvals.reload).to contain_exactly(approval1, approval2)
+        expect(subject.approved_approvers.reload).to contain_exactly(member1, member2)
       end
     end
   end
diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb
index 58de3f28685a34..16ef13e02117d5 100644
--- a/ee/spec/services/approval_rules/finalize_service_spec.rb
+++ b/ee/spec/services/approval_rules/finalize_service_spec.rb
@@ -35,11 +35,6 @@
         expect do
           subject.execute
         end.not_to change { ApprovalMergeRequestRule.count }
-
-        expect(approval1.approval_rules).to be_empty
-        expect(approval2.approval_rules).to be_empty
-        expect(approval3.approval_rules).to be_empty
-        expect(approval4.approval_rules).to be_empty
       end
     end
 
@@ -64,10 +59,8 @@
           expect(rule.approvals_required).to eq(12)
           expect(rule.users).to contain_exactly(member1, member2, group1_member)
           expect(rule.groups).to contain_exactly(group1)
-          expect(approval1.approval_rules).to contain_exactly(rule)
-          expect(approval2.approval_rules).to be_empty
-          expect(approval3.approval_rules).to contain_exactly(rule)
-          expect(approval4.approval_rules).to be_empty
+
+          expect(rule.approved_approvers).to contain_exactly(member1, group1_member)
         end
       end
     end
@@ -97,10 +90,8 @@
           expect(rule.approvals_required).to eq(32)
           expect(rule.users).to contain_exactly(member2, member3, group2_member)
           expect(rule.groups).to contain_exactly(group2)
-          expect(approval1.approval_rules).to be_empty
-          expect(approval2.approval_rules).to contain_exactly(rule)
-          expect(approval3.approval_rules).to be_empty
-          expect(approval4.approval_rules).to contain_exactly(rule)
+
+          expect(rule.approved_approvers).to contain_exactly(member3, group2_member)
         end
       end
     end
-- 
GitLab


From 201a65113fe852e91fcf5243e42cce271ff674e9 Mon Sep 17 00:00:00 2001
From: Mark Chao <mchao@gitlab.com>
Date: Mon, 7 Jan 2019 22:14:31 +0800
Subject: [PATCH 9/9] Improve various specs

Add test case for not allow author to approve
---
 .../approval_rules/finalize_service.rb        | 23 +++++++----
 .../approval_merge_request_rule_spec.rb       | 33 +++++++++------
 .../concerns/approval_rule_like_spec.rb       | 41 ++++++++++---------
 .../approval_rules/finalize_service_spec.rb   | 41 ++++++++++---------
 4 files changed, 77 insertions(+), 61 deletions(-)

diff --git a/ee/app/services/approval_rules/finalize_service.rb b/ee/app/services/approval_rules/finalize_service.rb
index 22031d64cc8cdf..9d482e87d459fb 100644
--- a/ee/app/services/approval_rules/finalize_service.rb
+++ b/ee/app/services/approval_rules/finalize_service.rb
@@ -11,13 +11,15 @@ def initialize(merge_request)
     def execute
       return unless merge_request.merged?
 
-      if merge_request.approval_rules.regular.exists?
-        merge_group_members_into_users
-      else
-        copy_project_approval_rules
+      ActiveRecord::Base.transaction do
+        if merge_request.approval_rules.regular.exists?
+          merge_group_members_into_users
+        else
+          copy_project_approval_rules
+        end
+
+        merge_request.approval_rules.each(&:sync_approved_approvers)
       end
-
-      merge_request.approval_rules.each(&:sync_approved_approvers)
     end
 
     private
@@ -30,9 +32,12 @@ def merge_group_members_into_users
 
     def copy_project_approval_rules
       merge_request.target_project.approval_rules.each do |project_rule|
-        rule = merge_request.approval_rules.create!(project_rule.attributes.slice('approvals_required', 'name'))
-        rule.users = project_rule.approvers
-        rule.groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
+        users = project_rule.approvers
+        groups = project_rule.groups.public_or_visible_to_user(merge_request.author)
+
+        merge_request.approval_rules.create!(
+          project_rule.attributes.slice('approvals_required', 'name').merge(users: users, groups: groups)
+        )
       end
     end
   end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 6253ae9159639e..16bc819617a599 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -1,33 +1,42 @@
 # frozen_string_literal: true
 
-require 'rails_helper'
+require 'spec_helper'
 
-RSpec.describe ApprovalMergeRequestRule, type: :model do
+describe ApprovalMergeRequestRule do
   let(:merge_request) { create(:merge_request) }
 
   subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
   describe '#project' do
     it 'returns project of MergeRequest' do
+      expect(subject.project).to be_present
       expect(subject.project).to eq(merge_request.project)
     end
   end
 
   describe '#approvers' do
-    context 'when project setting includes author' do
-      before do
-        merge_request.target_project.update(merge_requests_author_approval: true)
-
-        create(:group) do |group|
-          group.add_guest(merge_request.author)
-          subject.groups << group
-        end
+    before do
+      create(:group) do |group|
+        group.add_guest(merge_request.author)
+        subject.groups << group
       end
+    end
 
+    context 'when project merge_requests_author_approval is true' do
       it 'contains author' do
+        merge_request.project.update(merge_requests_author_approval: true)
+
         expect(subject.approvers).to contain_exactly(merge_request.author)
       end
     end
+
+    context 'when project merge_requests_author_approval is false' do
+      it 'contains author' do
+        merge_request.project.update(merge_requests_author_approval: false)
+
+        expect(subject.approvers).to be_empty
+      end
+    end
   end
 
   describe '#sync_approved_approvers' do
@@ -46,14 +55,14 @@
       it 'does nothing' do
         subject.sync_approved_approvers
 
-        expect(subject.approved_approvers).to be_empty
+        expect(subject.approved_approvers.reload).to be_empty
       end
     end
 
     context 'when merged' do
       let(:merge_request) { create(:merged_merge_request) }
 
-      it 'updates mapping' do
+      it 'records approved approvers as approved_approvers association' do
         subject.sync_approved_approvers
 
         expect(subject.approved_approvers.reload).to contain_exactly(member1, member2)
diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb
index daab80e779a057..ee7e02bbb2d284 100644
--- a/ee/spec/models/concerns/approval_rule_like_spec.rb
+++ b/ee/spec/models/concerns/approval_rule_like_spec.rb
@@ -1,11 +1,11 @@
 # frozen_string_literal: true
 
-require 'rails_helper'
+require 'spec_helper'
 
-RSpec.describe ApprovalRuleLike, type: :model do
-  let(:member1) { create(:user) }
-  let(:member2) { create(:user) }
-  let(:member3) { create(:user) }
+describe ApprovalRuleLike do
+  let(:user1) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
   let(:group1) { create(:group) }
   let(:group2) { create(:group) }
 
@@ -15,16 +15,17 @@
     describe '#add_member' do
       it 'adds as a member of the rule' do
         expect do
-          subject.add_member(member1)
-        end.to change { subject.users.count }.by(1)
+          subject.add_member(user1)
+          subject.add_member(group1)
+        end.to change { subject.users.count }.by(1).and change { subject.groups.count }.by(1)
       end
 
       it 'does nothing if already a member' do
-        subject.add_member(member1)
+        subject.add_member(user1)
 
         expect do
-          subject.add_member(member1)
-        end.not_to change { subject.users.count }
+          subject.add_member(user1)
+        end.not_to change { subject.users.count + subject.groups.count }
       end
     end
 
@@ -45,31 +46,31 @@
     end
 
     describe '#approvers' do
-      let(:group1_member1) { create(:user) }
-      let(:group2_member1) { create(:user) }
+      let(:group1_user) { create(:user) }
+      let(:group2_user) { create(:user) }
 
       before do
-        subject.users << member1
-        subject.users << member2
+        subject.users << user1
+        subject.users << user2
         subject.groups << group1
         subject.groups << group2
 
-        group1.add_guest(group1_member1)
-        group2.add_guest(group2_member1)
+        group1.add_guest(group1_user)
+        group2.add_guest(group2_user)
       end
 
       it 'contains users as direct members and group members' do
-        expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+        expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
       end
 
       context 'when user is both a direct member and a group member' do
         before do
-          group1.add_guest(member1)
-          group2.add_guest(member2)
+          group1.add_guest(user1)
+          group2.add_guest(user2)
         end
 
         it 'contains only unique users' do
-          expect(subject.approvers).to contain_exactly(member1, member2, group1_member1, group2_member1)
+          expect(subject.approvers).to contain_exactly(user1, user2, group1_user, group2_user)
         end
       end
     end
diff --git a/ee/spec/services/approval_rules/finalize_service_spec.rb b/ee/spec/services/approval_rules/finalize_service_spec.rb
index 16ef13e02117d5..2ed4dec67d5390 100644
--- a/ee/spec/services/approval_rules/finalize_service_spec.rb
+++ b/ee/spec/services/approval_rules/finalize_service_spec.rb
@@ -7,26 +7,26 @@
   let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
 
   describe '#execute' do
-    let!(:member1) { create(:user) }
-    let!(:member2) { create(:user) }
-    let!(:member3) { create(:user) }
+    let!(:user1) { create(:user) }
+    let!(:user2) { create(:user) }
+    let!(:user3) { create(:user) }
     let!(:group1) { create(:group) }
     let!(:group2) { create(:group) }
-    let!(:group1_member) { create(:user) }
-    let!(:group2_member) { create(:user) }
-    let!(:approval1) { create(:approval, merge_request: merge_request, user: member1) }
-    let!(:approval2) { create(:approval, merge_request: merge_request, user: member3) }
-    let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_member) }
-    let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_member) }
+    let!(:group1_user) { create(:user) }
+    let!(:group2_user) { create(:user) }
+    let!(:approval1) { create(:approval, merge_request: merge_request, user: user1) }
+    let!(:approval2) { create(:approval, merge_request: merge_request, user: user3) }
+    let!(:approval3) { create(:approval, merge_request: merge_request, user: group1_user) }
+    let!(:approval4) { create(:approval, merge_request: merge_request, user: group2_user) }
     let!(:project_rule) { create(:approval_project_rule, project: project, name: 'foo', approvals_required: 12) }
 
     subject { described_class.new(merge_request) }
 
     before do
-      group1.add_guest(group1_member)
-      group2.add_guest(group2_member)
+      group1.add_guest(group1_user)
+      group2.add_guest(group2_user)
 
-      project_rule.users = [member1, member2]
+      project_rule.users = [user1, user2]
       project_rule.groups << group1
     end
 
@@ -38,7 +38,7 @@
       end
     end
 
-    context 'when project rule is not overwritten' do
+    context 'when there is no merge request rules' do
       it_behaves_like 'skipping when unmerged'
 
       context 'when merged' do
@@ -57,18 +57,18 @@
 
           expect(rule.name).to eq('foo')
           expect(rule.approvals_required).to eq(12)
-          expect(rule.users).to contain_exactly(member1, member2, group1_member)
+          expect(rule.users).to contain_exactly(user1, user2, group1_user)
           expect(rule.groups).to contain_exactly(group1)
 
-          expect(rule.approved_approvers).to contain_exactly(member1, group1_member)
+          expect(rule.approved_approvers).to contain_exactly(user1, group1_user)
         end
       end
     end
 
-    context 'when project rule is overwritten' do
+    context 'when there is a regular merge request rule' do
       before do
         rule = create(:approval_merge_request_rule, merge_request: merge_request, name: 'bar', approvals_required: 32)
-        rule.users = [member2, member3]
+        rule.users = [user2, user3]
         rule.groups << group2
       end
 
@@ -78,7 +78,7 @@
         let(:merge_request) { create(:merged_merge_request, source_project: project, target_project: project) }
 
         it 'does not copy project rules, and updates approval mapping with MR rules' do
-          expect(merge_request).not_to receive(:copy_project_approval_rules)
+          allow(subject).to receive(:copy_project_approval_rules)
 
           expect do
             subject.execute
@@ -88,10 +88,11 @@
 
           expect(rule.name).to eq('bar')
           expect(rule.approvals_required).to eq(32)
-          expect(rule.users).to contain_exactly(member2, member3, group2_member)
+          expect(rule.users).to contain_exactly(user2, user3, group2_user)
           expect(rule.groups).to contain_exactly(group2)
 
-          expect(rule.approved_approvers).to contain_exactly(member3, group2_member)
+          expect(rule.approved_approvers).to contain_exactly(user3, group2_user)
+          expect(subject).not_to have_received(:copy_project_approval_rules)
         end
       end
     end
-- 
GitLab