Change to map MR rule to approved approvers directly

This is more direct in loggin who approved a rule.

db/schema.rb

@@ -236,11 +236,11 @@ ActiveRecord::Schema.define(version: 20190103140724) do
     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 @@ ActiveRecord::Schema.define(version: 20190103140724) do
   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

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] }

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 @@ class ApprovalMergeRequestRule < ApplicationRecord
     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

ee/app/services/approval_rules/finalize_service.rb

@@ -17,7 +17,7 @@ module ApprovalRules
         copy_project_approval_rules
       end
 
-      merge_request.approval_rules.each(&:sync_approvals)
+      merge_request.approval_rules.each(&:sync_approved_approvers)
     end
 
     private

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 @@ class CreateApprovalRulesApprovals < ActiveRecord::Migration[5.0]
         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

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe ApprovalMergeRequestRule, type: :model do
     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 @@ RSpec.describe ApprovalMergeRequestRule, type: :model do
 
     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 @@ RSpec.describe ApprovalMergeRequestRule, type: :model do
       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

ee/spec/services/approval_rules/finalize_service_spec.rb

@@ -35,11 +35,6 @@ describe ApprovalRules::FinalizeService 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
 
@@ -64,10 +59,8 @@ describe ApprovalRules::FinalizeService do
           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 @@ describe ApprovalRules::FinalizeService do
           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