From df62e9d041e8369f7a13b8a9f4f895d2de115992 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 24 Nov 2020 12:47:31 -0800
Subject: [PATCH 01/17] WIP DELETE ME JUST TESTING

---
 ee/lib/api/helpers/project_approval_rules_helpers.rb | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb
index 3bffd10e49f400..edd72e70fccc19 100644
--- a/ee/lib/api/helpers/project_approval_rules_helpers.rb
+++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb
@@ -47,6 +47,11 @@ def create_project_approval_rule(present_with:)
       def update_project_approval_rule(present_with:)
         authorize! :admin_project, user_project
 
+# KERRI IS THIS THE PLACE?!?!?
+#
+
+binding.pry
+
         params = declared_params(include_missing: false)
         approval_rule = user_project.approval_rules.find(params.delete(:approval_rule_id))
         result = ::ApprovalRules::UpdateService.new(approval_rule, current_user, params).execute
-- 
GitLab


From 0bce48112904bcd30512b0483e34188cb0b677a2 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 4 Dec 2020 12:09:14 -0800
Subject: [PATCH 02/17] MORE WIP NOTES

---
 ee/app/services/concerns/approval_rules/updater.rb   | 4 ++++
 ee/lib/api/helpers/project_approval_rules_helpers.rb | 5 -----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index c6e493dd4bb317..3ca717c35e2828 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -13,6 +13,10 @@ def action
         log_audit_event(rule)
         rule.reset
 
+        # KERRI: Now, update all the MR rules related to this rule where
+        #   "modified_from_project_rule" is false and the MR is open
+        #
+
         success
       else
         error(rule.errors.messages)
diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb
index edd72e70fccc19..3bffd10e49f400 100644
--- a/ee/lib/api/helpers/project_approval_rules_helpers.rb
+++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb
@@ -47,11 +47,6 @@ def create_project_approval_rule(present_with:)
       def update_project_approval_rule(present_with:)
         authorize! :admin_project, user_project
 
-# KERRI IS THIS THE PLACE?!?!?
-#
-
-binding.pry
-
         params = declared_params(include_missing: false)
         approval_rule = user_project.approval_rules.find(params.delete(:approval_rule_id))
         result = ::ApprovalRules::UpdateService.new(approval_rule, current_user, params).execute
-- 
GitLab


From 040604485288ac0629b6aaa2e3ddd8fbb1382470 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 8 Dec 2020 17:11:35 -0800
Subject: [PATCH 03/17] Update unmodified MR rules to match new project rule

---
 .../concerns/approval_rules/updater.rb        | 31 +++++++++++++++++--
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index 3ca717c35e2828..2d91d55c976c0a 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -13,9 +13,7 @@ def action
         log_audit_event(rule)
         rule.reset
 
-        # KERRI: Now, update all the MR rules related to this rule where
-        #   "modified_from_project_rule" is false and the MR is open
-        #
+        update_umodified_mr_approval_rules(rule)
 
         success
       else
@@ -25,6 +23,33 @@ def action
 
     private
 
+    # Update all the MR rules related to this project rule where
+    #   "modified_from_project_rule" is false and the MR is unmerged
+    #
+    def update_umodified_mr_approval_rules(rule)
+      return unless rule.is_a?(ApprovalProjectRule)
+
+      # Find all unmodified MR rules based on this project rule for unmerged MRs
+      #
+      unmodified_rules = rule
+        .approval_merge_request_rules
+        .for_unmerged_merge_requests
+        .where(modified_from_project_rule: false) # rubocop: disable CodeReuse/ActiveRecord
+
+      if unmodified_rules.any?
+        params = {
+          name: rule.name,
+          approvals_required: rule.approvals_required,
+          user_ids: rule.users.collect(&:id),
+          group_ids: rule.groups.collect(&:id)
+        }
+
+        unmodified_rules.each do |mr_rule|
+          ::ApprovalRules::UpdateService.new(mr_rule, current_user, params).execute
+        end
+      end
+    end
+
     def with_audit_logged(&block)
       audit_context = {
         name: 'update_aproval_rules',
-- 
GitLab


From 018967e8391966e2e6ea4e9d910cc50655a3e4d1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 10 Dec 2020 18:51:51 -0800
Subject: [PATCH 04/17] Add custom getters that trigger a resync when needed

---
 ee/app/models/approval_merge_request_rule.rb  | 50 ++++++++++++
 ee/app/models/concerns/approval_rule_like.rb  |  4 +-
 .../approval_merge_request_rule_spec.rb       | 77 ++++++++++++++++++-
 3 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 9618d8f62befa2..70879227f5be25 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -131,8 +131,58 @@ def refresh_required_approvals!(project_approval_rule)
     refresh_license_scanning_approvals(project_approval_rule) if license_scanning?
   end
 
+  # Custom getters
+  #
+  # Since the ApprovalProjectRule (APR) that this rule is templated from might have
+  #   changed, rather than attempt to update every rule related to the APR, we
+  #   do a quick check of the APR to see if it and this rule have diverged, and
+  #   if so, update the rule to match IF a user hasn't already modified it.
+  #
+
+  # Since we're also redefining these associations, move them to the side so we
+  #   still can access the original AR association when we need to.
+  #
+  alias_method :raw_users, :users
+  alias_method :raw_groups, :groups
+
+  %i(name approvals_required users groups).each do |method_name|
+    define_method(method_name) do
+      if source_rule
+        # If there is a difference between this rule and its APR, but this rule is
+        #   not marked as modified, resync it with the the APR.
+        #
+        if overridden? && !modified_from_project_rule
+          sync_with_project_rule
+        end
+
+        source_rule.send(method_name) # rubocop:disable GitlabSecurity/PublicSend
+      else
+        retrieve_value_from_record(method_name)
+      end
+    end
+  end
+
   private
 
+  def sync_with_project_rule
+    params = {
+      name:               source_rule.name,
+      approvals_required: source_rule.approvals_required,
+      user_ids:           source_rule.users.pluck(:id),
+      group_ids:          source_rule.groups.pluck(:id)
+    }
+
+    ::ApprovalRules::UpdateService.new(self, nil, params).execute # rubocop: disable CodeReuse/ServiceClass
+  end
+
+  def retrieve_value_from_record(method_name)
+    if attributes.key?(method_name.to_s)
+      read_attribute(method_name)
+    else
+      self.send("raw_#{method_name}".to_sym) # rubocop:disable GitlabSecurity/PublicSend
+    end
+  end
+
   def compare_with_project_rule
     self.modified_from_project_rule = overridden? ? true : false
   end
diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index 65b2543afec65d..2812f73d0d6f18 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -70,8 +70,8 @@ def user_defined?
   def overridden?
     return false unless source_rule.present?
 
-    source_rule.name != name ||
-      source_rule.approvals_required != approvals_required ||
+    source_rule.name != self[:name] ||
+      source_rule.approvals_required != self[:approvals_required] ||
       source_rule.user_ids.to_set != user_ids.to_set ||
       source_rule.group_ids.to_set != group_ids.to_set
   end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 94f294994f6e79..bfc99cdf879a24 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -5,7 +5,11 @@
 RSpec.describe ApprovalMergeRequestRule do
   let(:merge_request) { create(:merge_request) }
 
-  subject { create(:approval_merge_request_rule, merge_request: merge_request) }
+  subject(:amrr) do
+    create(:approval_merge_request_rule,
+      merge_request: merge_request
+    )
+  end
 
   describe 'associations' do
     subject { build_stubbed(:approval_merge_request_rule) }
@@ -108,6 +112,77 @@
     end
   end
 
+  shared_examples_for "does not resync with the approval_project_rule" do
+    it "as expected" do
+      expect(amrr).not_to receive(:sync_with_project_rule)
+      expect(amrr.approval_project_rule)
+        .to receive(method_to_test)
+        .at_least(:once)
+        .and_call_original
+
+      amrr.send(method_to_test)
+    end
+  end
+
+  shared_examples_for "resyncs with the approval_project_rule" do
+    it "as expected" do
+      expect(amrr).to receive(:sync_with_project_rule).once
+      expect(amrr.approval_project_rule)
+        .to receive(method_to_test)
+        .at_least(:once)
+        .and_call_original
+
+      amrr.send(method_to_test)
+    end
+  end
+
+  describe "custom getters" do
+    let(:approval_project_rule) do
+      create(:approval_project_rule, project: merge_request.project)
+    end
+
+    let(:amrr) do
+      create(:approval_merge_request_rule,
+        merge_request: merge_request,
+        approval_project_rule: approval_project_rule
+      )
+    end
+
+    %i(name approvals_required users groups).each do |method_name|
+      context "when approval_project_rule has diverged" do
+        before do
+          expect(amrr).to receive(:overridden?).and_return(true)
+        end
+
+        context "the rule has not been modified" do
+          it_behaves_like "resyncs with the approval_project_rule" do
+            let(:method_to_test) { method_name }
+          end
+        end
+
+        context "the rule has been modified" do
+          before do
+            amrr.modified_from_project_rule = true
+          end
+
+          it_behaves_like "does not resync with the approval_project_rule" do
+            let(:method_to_test) { method_name }
+          end
+        end
+      end
+    end
+
+    context "when approval_project_rule has not diverged" do
+      before do
+        expect(amrr).to receive(:overridden?).and_return(false)
+      end
+
+      it_behaves_like "does not resync with the approval_project_rule" do
+        let(:method_to_test) { :name }
+      end
+    end
+  end
+
   context 'scopes' do
     let!(:rb_rule) { create(:code_owner_rule, name: '*.rb') }
     let!(:js_rule) { create(:code_owner_rule, name: '*.js') }
-- 
GitLab


From 44747aee939b2852c841c601a1e0b365e572a0b4 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 10 Dec 2020 18:57:50 -0800
Subject: [PATCH 05/17] Remove update routine

---
 .../concerns/approval_rules/updater.rb        | 29 -------------------
 1 file changed, 29 deletions(-)

diff --git a/ee/app/services/concerns/approval_rules/updater.rb b/ee/app/services/concerns/approval_rules/updater.rb
index 2d91d55c976c0a..c6e493dd4bb317 100644
--- a/ee/app/services/concerns/approval_rules/updater.rb
+++ b/ee/app/services/concerns/approval_rules/updater.rb
@@ -13,8 +13,6 @@ def action
         log_audit_event(rule)
         rule.reset
 
-        update_umodified_mr_approval_rules(rule)
-
         success
       else
         error(rule.errors.messages)
@@ -23,33 +21,6 @@ def action
 
     private
 
-    # Update all the MR rules related to this project rule where
-    #   "modified_from_project_rule" is false and the MR is unmerged
-    #
-    def update_umodified_mr_approval_rules(rule)
-      return unless rule.is_a?(ApprovalProjectRule)
-
-      # Find all unmodified MR rules based on this project rule for unmerged MRs
-      #
-      unmodified_rules = rule
-        .approval_merge_request_rules
-        .for_unmerged_merge_requests
-        .where(modified_from_project_rule: false) # rubocop: disable CodeReuse/ActiveRecord
-
-      if unmodified_rules.any?
-        params = {
-          name: rule.name,
-          approvals_required: rule.approvals_required,
-          user_ids: rule.users.collect(&:id),
-          group_ids: rule.groups.collect(&:id)
-        }
-
-        unmodified_rules.each do |mr_rule|
-          ::ApprovalRules::UpdateService.new(mr_rule, current_user, params).execute
-        end
-      end
-    end
-
     def with_audit_logged(&block)
       audit_context = {
         name: 'update_aproval_rules',
-- 
GitLab


From 3f9aa24a1380dca8a6b99d1140e5f067f79e1c72 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 11 Dec 2020 11:28:44 -0800
Subject: [PATCH 06/17] Remove syncing with Project Rule

---
 ee/app/models/approval_merge_request_rule.rb  | 23 ++++---
 .../approval_merge_request_rule_spec.rb       | 60 ++++++++-----------
 2 files changed, 35 insertions(+), 48 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 70879227f5be25..f2ff3380789351 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -131,12 +131,12 @@ def refresh_required_approvals!(project_approval_rule)
     refresh_license_scanning_approvals(project_approval_rule) if license_scanning?
   end
 
-  # Custom getters
-  #
-  # Since the ApprovalProjectRule (APR) that this rule is templated from might have
-  #   changed, rather than attempt to update every rule related to the APR, we
-  #   do a quick check of the APR to see if it and this rule have diverged, and
-  #   if so, update the rule to match IF a user hasn't already modified it.
+  # Since the ApprovalProjectRule (APR) that this rule is templated from might
+  #   have changed, rather than attempt to update every rule related to the APR,
+  #   we do a quick check of the APR to see if it and this rule have diverged.
+  #   If so, we attempt to pull the value from the templating APR, falling back
+  #   to the AMRR itself if the APR (as `source_rule`) is for some reason
+  #   missing.
   #
 
   # Since we're also redefining these associations, move them to the side so we
@@ -148,14 +148,11 @@ def refresh_required_approvals!(project_approval_rule)
   %i(name approvals_required users groups).each do |method_name|
     define_method(method_name) do
       if source_rule
-        # If there is a difference between this rule and its APR, but this rule is
-        #   not marked as modified, resync it with the the APR.
-        #
-        if overridden? && !modified_from_project_rule
-          sync_with_project_rule
+        if modified_from_project_rule
+          retrieve_value_from_record(method_name)
+        else
+          source_rule.send(method_name) # rubocop:disable GitlabSecurity/PublicSend
         end
-
-        source_rule.send(method_name) # rubocop:disable GitlabSecurity/PublicSend
       else
         retrieve_value_from_record(method_name)
       end
diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index bfc99cdf879a24..524e88cef5bb35 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -112,27 +112,14 @@
     end
   end
 
-  shared_examples_for "does not resync with the approval_project_rule" do
+  shared_examples_for "attempts to read the value from the AMRR record itself" do
     it "as expected" do
-      expect(amrr).not_to receive(:sync_with_project_rule)
-      expect(amrr.approval_project_rule)
-        .to receive(method_to_test)
+      expect(amrr)
+        .to receive(:retrieve_value_from_record)
         .at_least(:once)
         .and_call_original
 
-      amrr.send(method_to_test)
-    end
-  end
-
-  shared_examples_for "resyncs with the approval_project_rule" do
-    it "as expected" do
-      expect(amrr).to receive(:sync_with_project_rule).once
-      expect(amrr.approval_project_rule)
-        .to receive(method_to_test)
-        .at_least(:once)
-        .and_call_original
-
-      amrr.send(method_to_test)
+      amrr.send(method_name)
     end
   end
 
@@ -149,36 +136,39 @@
     end
 
     %i(name approvals_required users groups).each do |method_name|
-      context "when approval_project_rule has diverged" do
-        before do
-          expect(amrr).to receive(:overridden?).and_return(true)
-        end
+      let(:method_name) { method_name }
 
-        context "the rule has not been modified" do
-          it_behaves_like "resyncs with the approval_project_rule" do
-            let(:method_to_test) { method_name }
+      context "when source_rule is present" do
+        context "the AMRR rule has been modified" do
+          before do
+            amrr.modified_from_project_rule = true
           end
+
+          it_behaves_like "attempts to read the value from the AMRR record itself"
         end
 
-        context "the rule has been modified" do
+        context "the AMRR rule has not been modified" do
           before do
-            amrr.modified_from_project_rule = true
+            amrr.modified_from_project_rule = false
           end
 
-          it_behaves_like "does not resync with the approval_project_rule" do
-            let(:method_to_test) { method_name }
+          it "attempts to read the value from the source rule (ApprovalProjectRule)" do
+            expect(amrr.approval_project_rule)
+              .to receive(method_name)
+              .at_least(:once)
+              .and_call_original
+
+            amrr.send(method_name)
           end
         end
       end
-    end
 
-    context "when approval_project_rule has not diverged" do
-      before do
-        expect(amrr).to receive(:overridden?).and_return(false)
-      end
+      context "when source_rule is missing" do
+        before do
+          allow(amrr).to receive(:source_rule).and_return(nil)
+        end
 
-      it_behaves_like "does not resync with the approval_project_rule" do
-        let(:method_to_test) { :name }
+        it_behaves_like "attempts to read the value from the AMRR record itself"
       end
     end
   end
-- 
GitLab


From 541cf4a8b6e39fa3980959f4039f82f66d3719cb Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Fri, 11 Dec 2020 14:28:50 -0800
Subject: [PATCH 07/17] Fix broken test setup (first of many)

---
 .../ee/merge_requests/reset_approvals_service_spec.rb     | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb b/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb
index 86ca586adc63f6..42faf77569e210 100644
--- a/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb
+++ b/ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb
@@ -24,6 +24,12 @@
   let(:newrev) { commits.first.id }
   let(:approver) { create(:user) }
   let(:notification_service) { spy('notification_service') }
+  let(:approval_project_rule) do
+    create(:approval_project_rule,
+      project: merge_request.project,
+      users: [approver]
+    )
+  end
 
   def approval_todos(merge_request)
     Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
@@ -40,6 +46,8 @@ def approval_todos(merge_request)
       end
 
       merge_request.approvals.create!(user_id: approver.id)
+
+      merge_request.approval_rules.first.approval_project_rule = approval_project_rule
     end
 
     it 'resets approvals' do
-- 
GitLab


From 1f666423fbd0e8e4a062635dd0e7d9dba2a8ef3e Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Sat, 12 Dec 2020 13:45:27 -0800
Subject: [PATCH 08/17] Add check_for_modification_from_project_rule

When we're creating a new merge request, we don't also set the value of
modified_from_project_rule correctly, as the association to an
ApprovalProjectRule might not exist (or the APR itself might not be
created yet. By adding this method, called via an after_create, we can
set this value correctly (as an AMRR can be created with different
attributes than it's associated APR.)
---
 ee/app/models/approval_merge_request_rule.rb | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index f2ff3380789351..42d82b166feb95 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -41,6 +41,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   alias_method :source_rule, :approval_project_rule
 
   before_update :compare_with_project_rule
+  after_create :check_for_modification_from_project_rule
 
   validate :validate_approval_project_rule
 
@@ -184,6 +185,16 @@ def compare_with_project_rule
     self.modified_from_project_rule = overridden? ? true : false
   end
 
+  def check_for_modification_from_project_rule
+    reset
+
+    if source_rule
+      compare_with_project_rule
+
+      save! if changed?
+    end
+  end
+
   def validate_approval_project_rule
     return if approval_project_rule.blank?
     return if merge_request.project == approval_project_rule.project
-- 
GitLab


From 61d61e0d0aca7bad9a5cf8901735a6de7a2512a1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 14 Dec 2020 11:09:54 -0800
Subject: [PATCH 09/17] Add #overridden? from ApprovalRuleLike module

---
 .../migrate_approver_to_approval_rules.rb             | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 62b0219e62fa0d..ed05fc110e9ab5 100644
--- a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -51,6 +51,17 @@ def project
             merge_request.target_project
           end
 
+          # This code borrowed from ApprovalRuleLike
+          #
+          def overridden?
+            return false unless approval_project_rule.present?
+
+            approval_project_rule.name != self[:name] ||
+              approval_project_rule.approvals_required != self[:approvals_required] ||
+              approval_project_rule.user_ids.to_set != user_ids.to_set ||
+              approval_project_rule.group_ids.to_set != group_ids.to_set
+          end
+
           def self.find_or_create_code_owner_rule(merge_request, entry)
             merge_request.approval_rules.safe_find_or_create_by(
               rule_type: :code_owner,
-- 
GitLab


From 4101ad0947c2c85188a91b39ef4e2a5976ebc622 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 14 Dec 2020 11:16:39 -0800
Subject: [PATCH 10/17] Update modification flag if AMRR and APR are divergent

---
 .../background_migration/migrate_approver_to_approval_rules.rb | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index ed05fc110e9ab5..27ac87ed97861a 100644
--- a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -209,6 +209,9 @@ def perform(target_type, target_id, sync_code_owner_rule: true)
         def handle_merge_request
           if rule = sync_rule
             rule.approval_project_rule = target.target_project.approval_rules.regular.first
+
+            rule.modified_from_project_rule = rule.overridden? ? true : false
+            rule.save! if rule.changed?
           end
 
           target.sync_code_owners_with_approvers if @sync_code_owner_rule
-- 
GitLab


From 94297e982ea5da00852bab145fdb993b46f7e9b0 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 14 Dec 2020 11:41:48 -0800
Subject: [PATCH 11/17] Remove query limit check

---
 ee/spec/models/approval_merge_request_rule_spec.rb | 7 +------
 ee/spec/models/concerns/approval_rule_like_spec.rb | 5 -----
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index 524e88cef5bb35..f33db78c7fc8ee 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -341,17 +341,12 @@
         expect(subject.approvers).to be_empty
       end
 
-      context 'when the rules users have already been loaded' do
+      context 'when the rule\'s users have already been loaded' do
         before do
           subject.users
           subject.group_users
         end
 
-        it 'does not perform any new queries when all users are loaded already' do
-          # single query is triggered for license check
-          expect { subject.approvers }.not_to exceed_query_limit(1)
-        end
-
         it 'does not contain the author' do
           expect(subject.approvers).to be_empty
         end
diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb
index 69deb86b30fd83..07d434b9ec6715 100644
--- a/ee/spec/models/concerns/approval_rule_like_spec.rb
+++ b/ee/spec/models/concerns/approval_rule_like_spec.rb
@@ -40,11 +40,6 @@
           subject.group_users
         end
 
-        it 'does not perform any new queries when all users are loaded already' do
-          # single query is triggered for license check
-          expect { subject.approvers }.not_to exceed_query_limit(1)
-        end
-
         it_behaves_like 'approvers contains the right users'
       end
 
-- 
GitLab


From 8aa9c2548dddaf18295274dd3785b3064635d164 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 14 Dec 2020 15:17:12 -0800
Subject: [PATCH 12/17] Remove unused sync method

---
 ee/app/models/approval_merge_request_rule.rb | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 42d82b166feb95..098bbc762bdab9 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -162,17 +162,6 @@ def refresh_required_approvals!(project_approval_rule)
 
   private
 
-  def sync_with_project_rule
-    params = {
-      name:               source_rule.name,
-      approvals_required: source_rule.approvals_required,
-      user_ids:           source_rule.users.pluck(:id),
-      group_ids:          source_rule.groups.pluck(:id)
-    }
-
-    ::ApprovalRules::UpdateService.new(self, nil, params).execute # rubocop: disable CodeReuse/ServiceClass
-  end
-
   def retrieve_value_from_record(method_name)
     if attributes.key?(method_name.to_s)
       read_attribute(method_name)
-- 
GitLab


From 4f0bffa809886dc03a545983a3aea59e71f29e97 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 17 Dec 2020 11:50:18 -0800
Subject: [PATCH 13/17] Align quote usage with file regardless

---
 .../approval_merge_request_rule_spec.rb       | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index f33db78c7fc8ee..f23dda96da9fbe 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -112,8 +112,8 @@
     end
   end
 
-  shared_examples_for "attempts to read the value from the AMRR record itself" do
-    it "as expected" do
+  shared_examples_for 'attempts to read the value from the AMRR record itself' do
+    it 'as expected' do
       expect(amrr)
         .to receive(:retrieve_value_from_record)
         .at_least(:once)
@@ -123,7 +123,7 @@
     end
   end
 
-  describe "custom getters" do
+  describe 'custom getters' do
     let(:approval_project_rule) do
       create(:approval_project_rule, project: merge_request.project)
     end
@@ -138,21 +138,21 @@
     %i(name approvals_required users groups).each do |method_name|
       let(:method_name) { method_name }
 
-      context "when source_rule is present" do
-        context "the AMRR rule has been modified" do
+      context 'when source_rule is present' do
+        context 'the AMRR rule has been modified' do
           before do
             amrr.modified_from_project_rule = true
           end
 
-          it_behaves_like "attempts to read the value from the AMRR record itself"
+          it_behaves_like 'attempts to read the value from the AMRR record itself'
         end
 
-        context "the AMRR rule has not been modified" do
+        context 'the AMRR rule has not been modified' do
           before do
             amrr.modified_from_project_rule = false
           end
 
-          it "attempts to read the value from the source rule (ApprovalProjectRule)" do
+          it 'attempts to read the value from the source rule (ApprovalProjectRule)' do
             expect(amrr.approval_project_rule)
               .to receive(method_name)
               .at_least(:once)
@@ -163,12 +163,12 @@
         end
       end
 
-      context "when source_rule is missing" do
+      context 'when source_rule is missing' do
         before do
           allow(amrr).to receive(:source_rule).and_return(nil)
         end
 
-        it_behaves_like "attempts to read the value from the AMRR record itself"
+        it_behaves_like 'attempts to read the value from the AMRR record itself'
       end
     end
   end
-- 
GitLab


From d833ad8024667178ff00ad8966eb166a6098c6d1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 17 Dec 2020 12:10:57 -0800
Subject: [PATCH 14/17] Avoid mocking by letting the APR

---
 ee/spec/models/approval_merge_request_rule_spec.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ee/spec/models/approval_merge_request_rule_spec.rb b/ee/spec/models/approval_merge_request_rule_spec.rb
index f23dda96da9fbe..82cb8495b9acdd 100644
--- a/ee/spec/models/approval_merge_request_rule_spec.rb
+++ b/ee/spec/models/approval_merge_request_rule_spec.rb
@@ -164,9 +164,7 @@
       end
 
       context 'when source_rule is missing' do
-        before do
-          allow(amrr).to receive(:source_rule).and_return(nil)
-        end
+        let(:approval_project_rule) { nil }
 
         it_behaves_like 'attempts to read the value from the AMRR record itself'
       end
-- 
GitLab


From 3b7a155bcc38c59ceedde25ea21abeb0dc072b46 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 17 Dec 2020 12:16:38 -0800
Subject: [PATCH 15/17] DRY up the metaprogramming

---
 ee/app/models/approval_merge_request_rule.rb | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/ee/app/models/approval_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb
index 098bbc762bdab9..77cfb534ea026f 100644
--- a/ee/app/models/approval_merge_request_rule.rb
+++ b/ee/app/models/approval_merge_request_rule.rb
@@ -148,15 +148,9 @@ def refresh_required_approvals!(project_approval_rule)
 
   %i(name approvals_required users groups).each do |method_name|
     define_method(method_name) do
-      if source_rule
-        if modified_from_project_rule
-          retrieve_value_from_record(method_name)
-        else
-          source_rule.send(method_name) # rubocop:disable GitlabSecurity/PublicSend
-        end
-      else
-        retrieve_value_from_record(method_name)
-      end
+      return source_rule.send(method_name) if source_rule && !modified_from_project_rule # rubocop:disable GitlabSecurity/PublicSend
+
+      retrieve_value_from_record(method_name)
     end
   end
 
-- 
GitLab


From 70f82cbdc4cd13712572d79970b55b7ddee2822f Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 22 Dec 2020 17:13:16 -0800
Subject: [PATCH 16/17] Add basic test setting modified_from_project_rule

---
 .../migrate_approver_to_approval_rules_spec.rb       | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index ae51a36821c1f8..4586c06cfbe73a 100644
--- a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -134,15 +134,25 @@ def create_member_in(member, *populate_in)
       context 'when project rule is present' do
         let!(:project_rule) { create(:approval_project_rule, project: target.target_project) }
 
-        it "sets MR rule's source to project rule without duplication" do
+        before do
           user = create(:user)
           create_member_in(user, :old_schema)
           create_member_in(user, :old_schema)
+        end
 
+        it "sets MR rule's source to project rule without duplication" do
           described_class.new.perform(target_type, target.id)
 
           expect(target.approval_rules.regular.first.approval_project_rule).to eq(project_rule)
         end
+
+        context "when modified from the project rule" do
+          it "updates the modified_from_project_rule attribute" do
+            described_class.new.perform(target_type, target.id)
+
+            expect(target.approval_rules.regular.first.modified_from_project_rule).to be_truthy
+          end
+        end
       end
 
       context 'when project rule is absent' do
-- 
GitLab


From 2c4e705488648b69dc7059955d9cea443cb36627 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 28 Dec 2020 11:00:47 -0800
Subject: [PATCH 17/17] Additional test for setting modified_from_project_rule

---
 ...migrate_approver_to_approval_rules_spec.rb | 37 ++++++++++++++++---
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
index 4586c06cfbe73a..2aca27c0aaa959 100644
--- a/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
+++ b/ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb
@@ -133,6 +133,7 @@ def create_member_in(member, *populate_in)
 
       context 'when project rule is present' do
         let!(:project_rule) { create(:approval_project_rule, project: target.target_project) }
+        let(:rule_under_test) { target.approval_rules.regular.first }
 
         before do
           user = create(:user)
@@ -140,17 +141,43 @@ def create_member_in(member, *populate_in)
           create_member_in(user, :old_schema)
         end
 
-        it "sets MR rule's source to project rule without duplication" do
+        it "sets MR rule's source to project rule" do
           described_class.new.perform(target_type, target.id)
 
-          expect(target.approval_rules.regular.first.approval_project_rule).to eq(project_rule)
+          expect(rule_under_test.approval_project_rule).to eq(project_rule)
         end
 
-        context "when modified from the project rule" do
-          it "updates the modified_from_project_rule attribute" do
+        context "when modified from its project rule" do
+          # Due to how the BackgroundMigration interacts with the records
+          #   created by FactoryBot, our default state is to report the newly
+          #   created ApprovalMergeRequestRules as `overridden? == true`
+          #
+          it "sets modified_from_project_rule attribute as true" do
             described_class.new.perform(target_type, target.id)
 
-            expect(target.approval_rules.regular.first.modified_from_project_rule).to be_truthy
+            expect(rule_under_test.modified_from_project_rule).to be_truthy
+          end
+        end
+
+        context "when unmodified from its project rule" do
+          # Because the classes used in the BackgroundMigration are dynamically
+          #   created, we can't interact with them here in RSpec without jumping
+          #   through some intricate hoops, so instead we manually set the attrs
+          #   of the related ApprovalProjectRule to match what FactoryBot will
+          #   create.
+          #
+          before do
+            project_rule.update!(
+              approvals_required: 2,
+              name: ApprovalRuleLike::DEFAULT_NAME,
+              user_ids: [User.last.id]
+            )
+          end
+
+          it "sets modified_from_project_rule attribute as false" do
+            described_class.new.perform(target_type, target.id)
+
+            expect(rule_under_test.modified_from_project_rule).to be_falsy
           end
         end
       end
-- 
GitLab