Merge branch '4134-approve-additionally' into 'master'

Approve additionally

Closes #4134

See merge request gitlab-org/gitlab-ee!4033

changelogs/unreleased-ee/4134-approve-additionally.yml

+---
+title: Approve merge requests additionally
+merge_request: 4134
+author:
+type: changed

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_body.js

+import { n__, s__, sprintf } from '~/locale';
 import Flash from '~/flash';
 import MRWidgetAuthor from '~/vue_merge_request_widget/components/mr_widget_author.vue';
 import eventHub from '~/vue_merge_request_widget/event_hub';
@@ -44,8 +45,24 @@ export default {
   },
   computed: {
     approvalsRequiredStringified() {
-      const baseString = `${this.approvalsLeft} more approval`;
-      return this.approvalsLeft === 1 ? baseString : `${baseString}s`;
+      let approvedString = s__('MergeRequest|Approved');
+      if (this.approvalsLeft >= 1) {
+        approvedString = sprintf(n__('mrWidget|Requires 1 more approval by',
+          'MergeRequest|Requires %d more approvals by', this.approvalsLeft));
+      }
+      return approvedString;
+    },
+    approveButtonText() {
+      let approveButtonText = s__('mrWidget|Approve');
+      if (this.approvalsLeft <= 0) {
+        approveButtonText = s__('mrWidget|Approve additionally');
+      }
+      return approveButtonText;
+    },
+    approveButtonClass() {
+      return {
+        'btn-inverted': this.showApproveButton && this.approvalsLeft <= 0,
+      };
     },
     showApproveButton() {
       return this.userCanApprove && !this.userHasApproved;
@@ -65,7 +82,7 @@ export default {
         })
         .catch(() => {
           this.approving = false;
-          new Flash('An error occured while submitting your approval.'); // eslint-disable-line
+          Flash(s__('mrWidget|An error occured while submitting your approval.'));
         });
     },
   },
@@ -75,18 +92,18 @@ export default {
         <button
           :disabled="approving"
           @click="approveMergeRequest"
-          class="btn btn-primary btn-sm approve-btn">
+          class="btn btn-primary btn-sm approve-btn"
+          :class="approveButtonClass">
           <i
             v-if="approving"
             class="fa fa-spinner fa-spin"
             aria-hidden="true" />
-          Approve
+          {{approveButtonText}}
         </button>
       </span>
       <span class="approvals-required-text bold">
-        Requires {{approvalsRequiredStringified}}
+        {{approvalsRequiredStringified}}
         <span v-if="showSuggestedApprovers">
-          <span class="dash">&mdash;</span>
           <mr-widget-author
             v-for="approver in suggestedApprovers"
             :key="approver.username"

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.js

 import Flash from '~/flash';
 import LinkToMemberAvatar from '~/vue_shared/components/link_to_member_avatar';
+import { s__ } from '~/locale';
 import eventHub from '~/vue_merge_request_widget/event_hub';
 
 export default {
@@ -47,6 +48,12 @@ export default {
       const isMerged = this.mr.state === 'merged';
       return this.userHasApproved && !this.userCanApprove && !isMerged;
     },
+    approvedByText() {
+      return s__('mrWidget|Approved by');
+    },
+    removeApprovalText() {
+      return s__('mrWidget|Remove your approval');
+    },
   },
   methods: {
     unapproveMergeRequest() {
@@ -59,7 +66,7 @@ export default {
         })
         .catch(() => {
           this.unapproving = false;
-          Flash('An error occured while removing your approval.');
+          Flash(s__('mrWidget|An error occured while removing your approval.'));
         });
     },
   },
@@ -68,14 +75,14 @@ export default {
       v-if="approvedBy.length"
       class="approved-by-users approvals-footer clearfix mr-info-list">
       <div class="approvers-prefix">
-        <p>Approved by</p>
+        <p>{{approvedByText}}</p>
         <div class="approvers-list">
           <link-to-member-avatar
             v-for="(approver, index) in approvedBy"
             :key="index"
             :avatar-size="20"
             :avatar-url="approver.user.avatar_url"
-            extra-link-class="approver-avatar"
+            extra-link-class="approver-avatar js-approver-list-member"
             :display-name="approver.user.name"
             :profile-url="approver.user.web_url"
             :show-tooltip="true"
@@ -99,7 +106,7 @@ export default {
             class="fa fa-spinner fa-spin"
             aria-hidden="true">
           </i>
-          Remove your approval
+          {{removeApprovalText}}
         </button>
       </div>
     </div>

ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/mr_widget_approvals.js

 import Flash from '~/flash';
 import statusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue';
+import { s__ } from '~/locale';
 import ApprovalsBody from './approvals_body';
 import ApprovalsFooter from './approvals_footer';
 
@@ -34,7 +35,7 @@ export default {
     },
   },
   created() {
-    const flashErrorMessage = 'An error occured while retrieving approval data for this merge request.';
+    const flashErrorMessage = s__('mrWidget|An error occured while retrieving approval data for this merge request.');
 
     this.service.fetchApprovals()
       .then((data) => {

ee/app/models/concerns/approvable.rb

@@ -134,11 +134,14 @@ module Approvable
     approved_by_users.include?(user)
   end
 
-  # Once there are fewer approvers left in the list than approvals required, allow other
-  # project members to approve the MR.
+  # Once there are fewer approvers left in the list than approvals required or
+  # there are no more approvals required
+  # allow other project members to approve the MR.
   #
   def any_approver_allowed?
-    approvals_left > approvers_left.count
+    remaining_approvals = approvals_left
+
+    remaining_approvals.zero? || remaining_approvals > approvers_left.count
   end
 
   def approved_by_users

spec/features/projects/merge_requests/user_approves_merge_request_spec.rb

@@ -5,6 +5,7 @@ describe 'User approves a merge request', :js do
   let(:project) { create(:project, :repository, approvals_before_merge: 1) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
 
   before do
     project.add_developer(user)
@@ -27,6 +28,34 @@ describe 'User approves a merge request', :js do
     end
   end
 
+  context 'when a merge request is approved additionally' do
+    before do
+      project.add_developer(user2)
+      project.add_developer(user3)
+      visit(merge_request_path(merge_request))
+    end
+
+    it 'shows multiple approvers beyond the needed count' do
+      click_button('Approve')
+      wait_for_requests
+
+      sign_out(user)
+
+      sign_in_visit_merge_request(user2)
+      sign_in_visit_merge_request(user3)
+
+      expect(all('.js-approver-list-member').count).to eq(3)
+    end
+
+    def sign_in_visit_merge_request(user)
+      sign_in(user)
+      visit(merge_request_path(merge_request))
+      click_button('Approve')
+      wait_for_requests
+      sign_out(user)
+    end
+  end
+
   context 'when user cannot approve' do
     before do
       merge_request.approvers.create(user_id: user2.id)

spec/javascripts/approvals/approvals_body_spec.js

@@ -56,7 +56,7 @@ import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/appr
     describe('Computed properties', function () {
       describe('approvalsRequiredStringified', function () {
         it('should display the correct string for 1 possible approver', function () {
-          const correctText = '1 more approval';
+          const correctText = 'Requires 1 more approval by';
           expect(this.approvalsBody.approvalsRequiredStringified).toBe(correctText);
         });
 
@@ -65,11 +65,20 @@ import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/appr
           this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
 
           Vue.nextTick(() => {
-            const correctText = '2 more approvals';
+            const correctText = 'Requires 2 more approvals by';
             expect(this.approvalsBody.approvalsRequiredStringified).toBe(correctText);
             done();
           });
         });
+
+        it('shows the "Approved" text message when there is enough approvals in place', function (done) {
+          this.approvalsBody.approvalsLeft = 0;
+
+          Vue.nextTick(() => {
+            expect(this.approvalsBody.approvalsRequiredStringified).toBe('Approved');
+            done();
+          });
+        });
       });
 
       describe('showApproveButton', function () {
@@ -91,6 +100,30 @@ import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/appr
           });
         });
       });
+
+      describe('approveButtonText', function () {
+        it('The approve button should have the "Approve" text', function (done) {
+          this.approvalsBody.approvalsLeft = 1;
+          this.approvalsBody.userHasApproved = false;
+          this.approvalsBody.userCanApprove = true;
+
+          Vue.nextTick(() => {
+            expect(this.approvalsBody.approveButtonText).toBe('Approve');
+            done();
+          });
+        });
+
+        it('The approve button should have the "Approve additionally" text', function (done) {
+          this.approvalsBody.approvalsLeft = 0;
+          this.approvalsBody.userHasApproved = false;
+          this.approvalsBody.userCanApprove = true;
+
+          Vue.nextTick(() => {
+            expect(this.approvalsBody.approveButtonText).toBe('Approve additionally');
+            done();
+          });
+        });
+      });
     });
   });
 })(window.gl || (window.gl = {}));

spec/models/merge_request_spec.rb

@@ -1885,6 +1885,7 @@ describe MergeRequest do
     context 'on a project with several members' do
       let(:approver_2) { create(:user) }
       let(:developer) { create(:user) }
+      let(:other_developer) { create(:user) }
       let(:reporter) { create(:user) }
       let(:stranger) { create(:user) }
 
@@ -1893,6 +1894,7 @@ describe MergeRequest do
         project.add_developer(approver)
         project.add_developer(approver_2)
         project.add_developer(developer)
+        project.add_developer(other_developer)
         project.add_reporter(reporter)
       end
 
@@ -2041,6 +2043,36 @@ describe MergeRequest do
             expect(merge_request.can_approve?(stranger)).to be_falsey
             expect(merge_request.can_approve?(nil)).to be_falsey
           end
+
+          context 'when only 1 approval approved' do
+            it 'only allows the approvers to approve the MR' do
+              create(:approval, user: approver, merge_request: merge_request)
+
+              expect(merge_request.can_approve?(developer)).to be_truthy
+              expect(merge_request.can_approve?(approver)).to be_falsey
+              expect(merge_request.can_approve?(approver_2)).to be_truthy
+
+              expect(merge_request.can_approve?(author)).to be_falsey
+              expect(merge_request.can_approve?(reporter)).to be_falsey
+              expect(merge_request.can_approve?(other_developer)).to be_falsey
+              expect(merge_request.can_approve?(stranger)).to be_falsey
+              expect(merge_request.can_approve?(nil)).to be_falsey
+            end
+          end
+
+          context 'when all approvals received' do
+            it 'allows anyone with write access except for author to approve the MR' do
+              create(:approval, user: approver, merge_request: merge_request)
+              create(:approval, user: approver_2, merge_request: merge_request)
+              create(:approval, user: developer, merge_request: merge_request)
+
+              expect(merge_request.can_approve?(author)).to be_falsey
+              expect(merge_request.can_approve?(reporter)).to be_falsey
+              expect(merge_request.can_approve?(other_developer)).to be_truthy
+              expect(merge_request.can_approve?(stranger)).to be_falsey
+              expect(merge_request.can_approve?(nil)).to be_falsey
+            end
+          end
         end
       end
     end