Merge branch '199795-overridden-approval-rules-be' into 'master'

Return overridden property in API response

See merge request gitlab-org/gitlab!28521

doc/api/merge_request_approvals.md

@@ -676,7 +676,8 @@ This includes additional information about the users who have already approved
         }
       ],
       "source_rule": null,
-      "approved": true
+      "approved": true,
+      "overridden": false
     }
   ]
 }
@@ -753,7 +754,8 @@ GET /projects/:id/merge_requests/:merge_request_iid/approval_rules
         "ldap_access": null
       }
     ],
-    "contains_hidden_groups": false
+    "contains_hidden_groups": false,
+    "overridden": false
   }
 ]
 ```
@@ -837,7 +839,8 @@ will be used.
       "ldap_access": null
     }
   ],
-  "contains_hidden_groups": false
+  "contains_hidden_groups": false,
+  "overridden": false
 }
 ```
 
@@ -921,7 +924,8 @@ These are system generated rules.
       "ldap_access": null
     }
   ],
-  "contains_hidden_groups": false
+  "contains_hidden_groups": false,
+  "overridden": false
 }
 ```
 

ee/app/models/approval_merge_request_rule.rb

@@ -75,8 +75,9 @@ class ApprovalMergeRequestRule < ApplicationRecord
   end
 
   def self.applicable_to_branch(branch)
-    includes(approval_project_rule: :protected_branches).select do |rule|
+    includes(:users, :groups, approval_project_rule: [:users, :groups, :protected_branches]).select do |rule|
       next true unless rule.approval_project_rule.present?
+      next true if rule.overridden?
 
       rule.approval_project_rule.applies_to_branch?(branch)
     end

ee/app/models/approval_wrapped_rule.rb

@@ -10,8 +10,8 @@ class ApprovalWrappedRule
 
   def_delegators(:@approval_rule,
                  :regular?, :any_approver?, :code_owner?, :report_approver?,
-                 :id, :name, :users, :groups, :code_owner, :source_rule,
-                 :rule_type, :approvals_required)
+                 :overridden?, :id, :name, :users, :groups, :code_owner,
+                 :source_rule, :rule_type, :approvals_required)
 
   def self.wrap(merge_request, rule)
     if rule.any_approver?

ee/app/models/concerns/approval_rule_like.rb

@@ -54,4 +54,13 @@ module ApprovalRuleLike
   def user_defined?
     regular? || any_approver?
   end
+
+  def overridden?
+    return false unless source_rule.present?
+
+    source_rule.name != name ||
+      source_rule.approvals_required != approvals_required ||
+      source_rule.user_ids.to_set != user_ids.to_set ||
+      source_rule.group_ids.to_set != group_ids.to_set
+  end
 end

ee/changelogs/unreleased/199795-overridden-approval-rules-be.yml

+---
+title: Return overridden property in API response
+merge_request: 28521
+author:
+type: fixed

ee/lib/ee/api/entities/merge_request_approval_rule.rb

@@ -9,6 +9,7 @@ module EE
         end
 
         expose :source_rule, using: MergeRequestApprovalRule::SourceRule
+        expose :overridden?, as: :overridden
       end
     end
   end

ee/spec/fixtures/api/schemas/public_api/v4/merge_request_approval_rule.json

@@ -30,7 +30,8 @@
         "type": "object",
         "properties": {}
       }
-    }
+    },
+    "overridden": { "type": "boolean" }
   },
   "additionalProperties": false
 }

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -189,10 +189,14 @@ describe ApprovalMergeRequestRule do
 
     subject { described_class.applicable_to_branch(branch) }
 
-    context 'when there are no associated source rules' do
+    shared_examples_for 'with applicable rules to specified branch' do
       it { is_expected.to eq([rule]) }
     end
 
+    context 'when there are no associated source rules' do
+      it_behaves_like 'with applicable rules to specified branch'
+    end
+
     context 'when there are associated source rules' do
       let(:source_rule) { create(:approval_project_rule, project: merge_request.target_project) }
 
@@ -200,27 +204,46 @@ describe ApprovalMergeRequestRule do
         rule.update!(approval_project_rule: source_rule)
       end
 
-      context 'and there are no associated protected branches to source rule' do
-        it { is_expected.to eq([rule]) }
-      end
-
-      context 'and there are associated protected branches to source rule' do
+      context 'and rule is not overridden' do
         before do
-          source_rule.update!(protected_branches: protected_branches)
+          rule.update!(
+            name: source_rule.name,
+            approvals_required: source_rule.approvals_required,
+            users: source_rule.users,
+            groups: source_rule.groups
+          )
         end
 
-        context 'and branch matches' do
-          let(:protected_branches) { [create(:protected_branch, name: branch)] }
-
-          it { is_expected.to eq([rule]) }
+        context 'and there are no associated protected branches to source rule' do
+          it_behaves_like 'with applicable rules to specified branch'
         end
 
-        context 'but branch does not match anything' do
-          let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+        context 'and there are associated protected branches to source rule' do
+          before do
+            source_rule.update!(protected_branches: protected_branches)
+          end
+
+          context 'and branch matches' do
+            let(:protected_branches) { [create(:protected_branch, name: branch)] }
 
-          it { is_expected.to be_empty }
+            it_behaves_like 'with applicable rules to specified branch'
+          end
+
+          context 'but branch does not match anything' do
+            let(:protected_branches) { [create(:protected_branch, name: branch.reverse)] }
+
+            it { is_expected.to be_empty }
+          end
         end
       end
+
+      context 'but rule is overridden' do
+        before do
+          rule.update!(name: 'Overridden Rule')
+        end
+
+        it_behaves_like 'with applicable rules to specified branch'
+      end
     end
   end
 

ee/spec/models/approval_state_spec.rb

@@ -1585,8 +1585,22 @@ describe ApprovalState do
             merge_request.update!(target_branch: 'stable-1')
             source_rule.update!(protected_branches: [protected_branch])
             another_source_rule.update!(protected_branches: [another_protected_branch])
-            mr_rule.update!(approval_project_rule: another_source_rule)
-            another_mr_rule.update!(approval_project_rule: source_rule)
+
+            mr_rule.update!(
+              approval_project_rule: another_source_rule,
+              name: another_source_rule.name,
+              approvals_required: another_source_rule.approvals_required,
+              users: another_source_rule.users,
+              groups: another_source_rule.groups
+            )
+
+            another_mr_rule.update!(
+              approval_project_rule: source_rule,
+              name: source_rule.name,
+              approvals_required: source_rule.approvals_required,
+              users: source_rule.users,
+              groups: source_rule.groups
+            )
           end
 
           context 'and scoped_approval_rules feature is enabled' do

ee/spec/models/concerns/approval_rule_like_spec.rb

@@ -84,12 +84,96 @@ describe ApprovalRuleLike do
     subject { create(:approval_merge_request_rule, merge_request: merge_request) }
 
     it_behaves_like 'approval rule like'
+
+    describe '#overridden?' do
+      it 'returns false' do
+        expect(subject.overridden?).to be_falsy
+      end
+
+      context 'when rule has source rule' do
+        let(:source_rule) do
+          create(
+            :approval_project_rule,
+            project: merge_request.target_project,
+            name: 'Source Rule',
+            approvals_required: 2,
+            users: [user1, user2],
+            groups: [group1, group2]
+          )
+        end
+
+        before do
+          subject.update!(approval_project_rule: source_rule)
+        end
+
+        context 'and any attributes differ from source rule' do
+          shared_examples_for 'overridden rule' do
+            it 'returns true' do
+              expect(subject.overridden?).to be_truthy
+            end
+          end
+
+          context 'name' do
+            before do
+              subject.update!(name: 'Overridden Rule')
+            end
+
+            it_behaves_like 'overridden rule'
+          end
+
+          context 'approvals_required' do
+            before do
+              subject.update!(approvals_required: 1)
+            end
+
+            it_behaves_like 'overridden rule'
+          end
+
+          context 'users' do
+            before do
+              subject.update!(users: [user1])
+            end
+
+            it_behaves_like 'overridden rule'
+          end
+
+          context 'groups' do
+            before do
+              subject.update!(groups: [group1])
+            end
+
+            it_behaves_like 'overridden rule'
+          end
+        end
+
+        context 'and no changes made to attributes' do
+          before do
+            subject.update!(
+              name: source_rule.name,
+              approvals_required: source_rule.approvals_required,
+              users: source_rule.users,
+              groups: source_rule.groups
+            )
+          end
+
+          it 'returns false' do
+            expect(subject.overridden?).to be_falsy
+          end
+        end
+      end
+    end
   end
 
   context 'Project' do
     subject { create(:approval_project_rule) }
 
     it_behaves_like 'approval rule like'
+
+    describe '#overridden?' do
+      it 'returns false' do
+        expect(subject.overridden?).to be_falsy
+      end
+    end
   end
 
   describe '.group_users' do

ee/spec/requests/projects/merge_requests_controller_spec.rb

@@ -28,8 +28,8 @@ describe Projects::MergeRequestsController do
 
         create(:code_owner_rule, merge_request: merge_request)
 
-        # Threshold of 2 because we load the users & group users for all rules
-        expect { get_edit }.not_to exceed_query_limit(control).with_threshold(2)
+        # Threshold of 3 because we load the source_rule, users & group users for all rules
+        expect { get_edit }.not_to exceed_query_limit(control).with_threshold(3)
       end
 
       it 'does not cause extra queries when multiple code owner rules are present' do