Merge branch 'ph/32285/fixApprovalsSummary' into 'master'

Fixes approval rules summary text

See merge request gitlab-org/gitlab!70928

app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_summary.vue

@@ -35,13 +35,17 @@ export default {
       }
 
       if (!this.rulesLeft.length) {
-        return n__('Requires approval.', 'Requires %d more approvals.', this.approvalsLeft);
+        return n__(
+          'Requires %d approval from eligible users.',
+          'Requires %d approvals from eligible users.',
+          this.approvalsLeft,
+        );
       }
 
       return sprintf(
         n__(
-          'Requires approval from %{names}.',
-          'Requires %{count} more approvals from %{names}.',
+          'Requires %{count} approval from %{names}.',
+          'Requires %{count} approvals from %{names}.',
           this.approvalsLeft,
         ),
         {

ee/app/assets/javascripts/vue_merge_request_widget/mappers.js

@@ -4,6 +4,7 @@ import {
   RULE_TYPE_FALLBACK,
   RULE_TYPE_CODE_OWNER,
   RULE_TYPE_REPORT_APPROVER,
+  RULE_TYPE_ANY_APPROVER,
 } from 'ee/approvals/constants';
 import { __ } from '~/locale';
 
@@ -33,6 +34,8 @@ function getApprovalRuleNamesLeft(data) {
 
   const rulesLeft = groupBy(data.approval_rules_left, (x) => x.rule_type);
 
+  const anyApprover = rulesLeft[RULE_TYPE_ANY_APPROVER] ? [__('eligible users')] : [];
+
   // Filter out empty names (fallback rule has no name) because the empties would look weird.
   const regularRules = (rulesLeft[RULE_TYPE_REGULAR] || []).map((x) => x.name).filter((x) => x);
 
@@ -43,7 +46,7 @@ function getApprovalRuleNamesLeft(data) {
   // As the names of code owner rules are patterns that don't mean much out of context.
   const codeOwnerRules = rulesLeft[RULE_TYPE_CODE_OWNER] ? [__('Code Owners')] : [];
 
-  return [...regularRules, ...reportApprovalRules, ...codeOwnerRules];
+  return [...anyApprover, ...regularRules, ...reportApprovalRules, ...codeOwnerRules];
 }
 
 /**

ee/app/models/approval_state.rb

@@ -77,12 +77,42 @@ class ApprovalState
   # considered approved.
   def approvals_left
     strong_memoize(:approvals_left) do
-      wrapped_approval_rules.sum(&:approvals_left)
+      [regular_rules_left + code_owner_rules_left + report_approver_rules_left, any_approver_rules_left].max
+    end
+  end
+
+  def code_owner_rules_left
+    strong_memoize(:code_owner_rules_left) do
+      code_owner_rules.sum(&:approvals_left)
+    end
+  end
+
+  def report_approver_rules_left
+    strong_memoize(:report_approver_rules_left) do
+      report_approver_rules.sum(&:approvals_left)
+    end
+  end
+
+  def regular_rules_left
+    strong_memoize(:regular_rules_left) do
+      regular_approval_rules.sum(&:approvals_left)
+    end
+  end
+
+  def any_approver_rules_left
+    strong_memoize(:any_approver_rules_left) do
+      any_approver_approval_rules.sum(&:approvals_left)
     end
   end
 
   def approval_rules_left
-    wrapped_approval_rules.reject(&:approved?)
+    rules = if any_approver_rules_left <= regular_rules_left + code_owner_rules_left + report_approver_rules_left
+              wrapped_approval_rules.reject(&:any_approver?)
+            else
+              wrapped_approval_rules
+            end
+
+    rules.reject(&:approved?)
   end
 
   def approvers
@@ -203,6 +233,18 @@ class ApprovalState
     end
   end
 
+  def regular_approval_rules
+    strong_memoize(:regular_approval_rules) do
+      wrapped_approval_rules.select(&:regular?)
+    end
+  end
+
+  def any_approver_approval_rules
+    strong_memoize(:any_approver_approval_rules) do
+      wrapped_approval_rules.select(&:any_approver?)
+    end
+  end
+
   def wrapped_rules
     strong_memoize(:wrapped_rules) do
       merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch)

ee/spec/features/merge_request/user_sees_approval_widget_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe 'Merge request > User sees approval widget', :js do
       it 'the renders the number of required approvals' do
         wait_for_requests
 
-        expect(page).to have_content('Requires 3 more approvals.')
+        expect(page).to have_content('Requires 3 approvals from eligible users.')
       end
     end
 
@@ -66,7 +66,7 @@ RSpec.describe 'Merge request > User sees approval widget', :js do
         visit project_merge_request_path(project, merge_request)
 
         wait_for_requests
-        expect(page).to have_content("Requires approval from #{rule.name}")
+        expect(page).to have_content("Requires 1 approval from #{rule.name}")
 
         click_on 'View eligible approvers'
         wait_for_requests
@@ -94,7 +94,7 @@ RSpec.describe 'Merge request > User sees approval widget', :js do
           visit project_merge_request_path(project, merge_request)
 
           wait_for_requests
-          expect(page).to have_content("Requires approval from #{rule.name}.")
+          expect(page).to have_content("Requires 1 approval from #{rule.name}.")
 
           click_on 'View eligible approvers'
           wait_for_requests
@@ -118,7 +118,7 @@ RSpec.describe 'Merge request > User sees approval widget', :js do
             visit project_merge_request_path(project, merge_request)
             wait_for_requests
 
-            expect(page).to have_content("Requires 2 more approvals from #{rule.name} and Code Owners")
+            expect(page).to have_content("Requires 2 approvals from #{rule.name} and Code Owners")
 
             click_on 'View eligible approvers'
             wait_for_requests

ee/spec/features/merge_request/user_sets_approvers_spec.rb

@@ -92,7 +92,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         click_on("Create merge request")
         wait_for_all_requests
 
-        expect(page).to have_content("Requires approval.")
+        expect(page).to have_content("Requires 1 approval from eligible users.")
         expect(page).to have_selector("img[alt='#{other_user.name}']")
       end
 
@@ -165,7 +165,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
           click_on("Save changes")
           wait_for_all_requests
 
-          expect(page).to have_content("Requires approval.")
+          expect(page).to have_content("Requires 1 approval from eligible users.")
           expect(page).to have_selector("img[alt='#{other_user.name}']")
         end
       end
@@ -193,7 +193,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         click_on("Save changes")
         wait_for_all_requests
 
-        expect(page).to have_content("Requires approval.")
+        expect(page).to have_content("Requires 1 approval from eligible users.")
         expect(page).to have_selector("img[alt='#{user.name}']")
         expect(page).to have_selector("img[alt='#{other_user.name}']")
       end
@@ -225,7 +225,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
 
         expect(page).not_to have_selector(".js-approvers img[alt='#{other_user.name}']")
         expect(page).to have_selector(".js-approvers img[alt='#{approver.name}']")
-        expect(page).to have_content("Requires approval.")
+        expect(page).to have_content("Requires 1 approval from eligible users.")
       end
 
       it 'allows changing approvals number' do
@@ -237,7 +237,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         wait_for_requests
 
         # project setting in the beginning on the show MR page
-        expect(page).to have_content("Requires 2 more approvals")
+        expect(page).to have_content("Requires 2 approvals from eligible users")
 
         find('.merge-request').click_on 'Edit'
         open_modal
@@ -253,7 +253,7 @@ RSpec.describe 'Merge request > User sets approvers', :js do
         wait_for_all_requests
 
         # new MR setting on the show MR page
-        expect(page).to have_content("Requires 3 more approvals")
+        expect(page).to have_content("Requires 3 approvals from eligible users")
 
         # new MR setting on the edit MR page
         find('.merge-request').click_on 'Edit'

ee/spec/features/merge_requests/user_resets_approvers_spec.rb

@@ -46,6 +46,6 @@ RSpec.describe 'Merge Requests > User resets approvers', :js do
 
     wait_for_requests
 
-    expect(page).to have_content 'Requires approval'
+    expect(page).to have_content 'Requires 1 approval'
   end
 end

ee/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb

@@ -87,7 +87,7 @@ RSpec.describe 'User approves a merge request', :js do
       page.within('.mr-state-widget') do
         expect(page).to have_button('Merge', disabled: true)
         expect(page).not_to have_button('Approve')
-        expect(page).to have_content('Requires approval')
+        expect(page).to have_content('Requires 1 approval from eligible users.')
       end
     end
   end

ee/spec/models/approval_state_spec.rb

@@ -316,6 +316,22 @@ RSpec.describe ApprovalState do
         expect(subject.approvals_left).to eq(12)
       end
 
+      context 'with any approval rule' do
+        it 'sums approvals_left from regular rules' do
+          create_rule(rule_type: :any_approver, approvals_required: 20)
+
+          expect(subject.approvals_left).to eq(20)
+        end
+      end
+
+      context 'with report approver rule' do
+        it 'sums code_owner_rules_left from report approver rules' do
+          create_rule(rule_type: :report_approver, approvals_required: 20)
+
+          expect(subject.approvals_left).to eq(32)
+        end
+      end
+
       context 'when approval feature is unavailable' do
         it 'returns 0' do
           disable_feature
@@ -605,7 +621,7 @@ RSpec.describe ApprovalState do
           end
 
           it 'is not approved' do
-            expect(subject.approvals_left).to eq(2)
+            expect(subject.approvals_left).to eq(1)
             expect(subject.approved?).to eq(false)
           end
         end

locale/gitlab.pot

@@ -28973,13 +28973,13 @@ msgstr ""
 msgid "Requirements can be based on users, stakeholders, system, software, or anything else you find important to capture."
 msgstr ""
 
-msgid "Requires approval from %{names}."
-msgid_plural "Requires %{count} more approvals from %{names}."
+msgid "Requires %d approval from eligible users."
+msgid_plural "Requires %d approvals from eligible users."
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "Requires approval."
-msgid_plural "Requires %d more approvals."
+msgid "Requires %{count} approval from %{names}."
+msgid_plural "Requires %{count} approvals from %{names}."
 msgstr[0] ""
 msgstr[1] ""
 
@@ -40209,6 +40209,9 @@ msgstr ""
 msgid "element is not a hierarchy"
 msgstr ""
 
+msgid "eligible users"
+msgstr ""
+
 msgid "email '%{email}' is not a verified email."
 msgstr ""
 

spec/frontend/vue_mr_widget/components/approvals/approvals_summary_spec.js

@@ -61,9 +61,7 @@ describe('MRWidget approvals summary', () => {
     it('render message', () => {
       const names = toNounSeriesText(testRulesLeft());
 
-      expect(wrapper.text()).toContain(
-        `Requires ${TEST_APPROVALS_LEFT} more approvals from ${names}.`,
-      );
+      expect(wrapper.text()).toContain(`Requires ${TEST_APPROVALS_LEFT} approvals from ${names}.`);
     });
   });
 
@@ -75,7 +73,9 @@ describe('MRWidget approvals summary', () => {
     });
 
     it('renders message', () => {
-      expect(wrapper.text()).toContain(`Requires ${TEST_APPROVALS_LEFT} more approvals.`);
+      expect(wrapper.text()).toContain(
+        `Requires ${TEST_APPROVALS_LEFT} approvals from eligible users`,
+      );
     });
   });