Merge branch 'fix-mr-approval-sentence' into 'master'

Fix MR approvals sentence

Closes #2272

See merge request !1730

app/assets/javascripts/merge_request_widget/approvals/components/approvals_body.js

@@ -44,12 +44,15 @@ Vue.component('approvals-body', {
         return '';
       }
 
+      const separator = this.approvalsLeft === approvers.length ? 'and' : 'or';
+      const serialComma = approvers.length > 2 ? ',' : '';
+
       return approvers.length === 1 ? approvers[0].name :
         approvers.reduce((memo, curr, index) => {
           const nextMemo = `${memo}${curr.name}`;
 
           if (index === approvers.length - 2) { // second to last index
-            return `${nextMemo} or `;
+            return `${nextMemo}${serialComma} ${separator} `;
           } else if (index === approvers.length - 1) { // last index
             return nextMemo;
           }

app/helpers/merge_requests_helper.rb

@@ -99,31 +99,6 @@ module MergeRequestsHelper
     end
   end
 
-  # This may be able to be removed with associated specs
-  def render_require_section(merge_request)
-    str = if merge_request.approvals_left == 1
-            "Requires one more approval"
-          else
-            "Requires #{merge_request.approvals_left} more approvals"
-          end
-
-    if merge_request.approvers_left.any?
-      more_approvals = merge_request.approvals_left - merge_request.approvers_left.count
-      approvers_names = merge_request.approvers_left.map(&:name)
-
-      str <<
-        if more_approvals > 0
-          " (from #{render_items_list(approvers_names + ["#{more_approvals} more"])})"
-        elsif more_approvals < 0
-          " (from #{render_items_list(approvers_names, "or")})"
-        else
-          " (from #{render_items_list(approvers_names)})"
-        end
-    end
-
-    str
-  end
-
   def mr_assign_issues_link
     issues = MergeRequests::AssignIssuesService.new(@project,
                                                     current_user,

changelogs/unreleased-ee/fix-mr-approval-sentence.yml

+---
+title: Fix MR approvals sentence when all approvers need to approve the MR
+merge_request:
+author:

spec/helpers/merge_request_helper_spec.rb

@@ -82,49 +82,4 @@ describe MergeRequestsHelper do
       expect(render_items_list(%w(user user1 user2))).to eq("user, user1 and user2")
     end
   end
-
-  describe 'render_require_section' do
-    let(:merge_request) { create(:merge_request, target_project: project, source_project: project) }
-    let(:approver) { create(:user) }
-
-    before do
-      5.times { project.team << [create(:user), :developer] }
-      project.team << [approver, :developer]
-    end
-
-    it "returns correct value in case - one approval" do
-      project.update(approvals_before_merge: 1)
-
-      expect(render_require_section(merge_request)).to eq("Requires one more approval")
-    end
-
-    it "returns correct value in case - two approval" do
-      project.update(approvals_before_merge: 2)
-
-      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals")
-    end
-
-    it "returns correct value in case - one approver" do
-      project.update(approvals_before_merge: 1)
-      create(:approver, user: approver, target: merge_request)
-
-      expect(render_require_section(merge_request)).to eq("Requires one more approval (from #{approver.name})")
-    end
-
-    it "returns correct value in case - one approver and one more" do
-      project.update(approvals_before_merge: 2)
-      create(:approver, user: approver, target: merge_request)
-
-      expect(render_require_section(merge_request)).to eq("Requires 2 more approvals (from #{approver.name} and 1 more)")
-    end
-
-    it "returns correct value in case - two approver and one more" do
-      project.update(approvals_before_merge: 3)
-      approver2 = create(:user)
-      create(:approver, user: approver, target: merge_request)
-      create(:approver, user: approver2, target: merge_request)
-
-      expect(render_require_section(merge_request)).to eq("Requires 3 more approvals (from #{approver2.name}, #{approver.name} and 1 more)")
-    end
-  end
 end

spec/javascripts/approvals/approvals_body_spec.js

@@ -58,7 +58,7 @@ require('~/merge_request_widget/approvals/components/approvals_body');
           expect(this.approvalsBody.approvalsRequiredStringified).toBe(correctText);
         });
 
-        it('should display the correct string for 2 possible approver', function (done) {
+        it('should display the correct string for 2 possible approvers', function (done) {
           this.approvalsBody.approvalsLeft = 2;
           this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
 
@@ -81,7 +81,6 @@ require('~/merge_request_widget/approvals/components/approvals_body');
         });
 
         it('should display the correct string for 2 possible approver names', function (done) {
-          this.approvalsBody.approvalsLeft = 2;
           this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
 
           Vue.nextTick(() => {
@@ -92,12 +91,23 @@ require('~/merge_request_widget/approvals/components/approvals_body');
         });
 
         it('should display the correct string for 3 possible approver names', function (done) {
+          this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
+          this.approvalsBody.suggestedApprovers.push({ name: 'Approver 3' });
+
+          Vue.nextTick(() => {
+            const correctText = 'Approver 1, Approver 2, or Approver 3';
+            expect(this.approvalsBody.approverNamesStringified).toBe(correctText);
+            done();
+          });
+        });
+
+        it('should join the names with "and" when all of the remaining approvers have to approve the MR', function (done) {
           this.approvalsBody.approvalsLeft = 3;
           this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
           this.approvalsBody.suggestedApprovers.push({ name: 'Approver 3' });
 
           Vue.nextTick(() => {
-            const correctText = 'Approver 1, Approver 2 or Approver 3';
+            const correctText = 'Approver 1, Approver 2, and Approver 3';
             expect(this.approvalsBody.approverNamesStringified).toBe(correctText);
             done();
           });