diff --git a/changelogs/unreleased-ee/4134-approve-additionally.yml b/changelogs/unreleased-ee/4134-approve-additionally.yml
new file mode 100644
index 0000000000000000000000000000000000000000..1af99d39d0a4f97e4ecd788df4f8c7d4225dcbc9
--- /dev/null
+++ b/changelogs/unreleased-ee/4134-approve-additionally.yml
@@ -0,0 +1,5 @@
+---
+title: Approve merge requests additionally
+merge_request: 4134
+author:
+type: changed
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_body.js b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_body.js
index 6899e62e0ae70bafac534be1c643b83630f26147..42bd0d615eecae826279dafaedf8ca71335615d9 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_body.js
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_body.js
@@ -1,3 +1,4 @@
+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">—</span>
<mr-widget-author
v-for="approver in suggestedApprovers"
:key="approver.username"
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.js b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.js
index bfb9da7aff511f94d9f8a5130269bce7b1d62395..c8f13f2352d49fed803c8def9a040175b651372b 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.js
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/approvals_footer.js
@@ -1,5 +1,6 @@
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>
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/mr_widget_approvals.js b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/mr_widget_approvals.js
index ca2e08db8feca72104faef83638ba0f529bf767d..9db2aec34c4faea2e5ab0e88aea4ab31002f5718 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/mr_widget_approvals.js
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/mr_widget_approvals.js
@@ -1,5 +1,6 @@
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) => {
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index 0cefa4277bf2759afb805792cd19927db80e9e84..00928f43392a092d329aa400c854608d0dce5046 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -134,11 +134,14 @@ def has_approved?(user)
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
diff --git a/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb b/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb
index d7f452f7bdf8a588e773793fb1e98ee07ff90620..32f08e04c1ec21a963aeec976610d2d1bbf488df 100644
--- a/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb
+++ b/spec/features/projects/merge_requests/user_approves_merge_request_spec.rb
@@ -5,6 +5,7 @@
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 @@
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)
diff --git a/spec/javascripts/approvals/approvals_body_spec.js b/spec/javascripts/approvals/approvals_body_spec.js
index ca93179994779936f219e413993d0e7ca582b143..4ffb656facd5154deddb06651cda36ab4bdd2b4a 100644
--- a/spec/javascripts/approvals/approvals_body_spec.js
+++ b/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 = {}));
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 22f08ad6193b89c9ff0c68c4279c5a164a80c2c8..1dc583479ce3a5e21e3a5aef7243ad0d7e9b56f1 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1885,6 +1885,7 @@ def set_compare(merge_request)
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 @@ def set_compare(merge_request)
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 @@ def set_compare(merge_request)
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