From 9668791a2f55f35c5f207ccaae622e80fa4c028e Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Thu, 11 Jan 2018 13:29:53 -0600
Subject: [PATCH 1/9] Initial changes to approve additionally
---
.../components/approvals/approvals_body.js | 17 ++++++++++++++++-
ee/app/models/concerns/approvable.rb | 2 +-
2 files changed, 17 insertions(+), 2 deletions(-)
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 6899e62e0ae70baf..2d338c517a596cc7 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
@@ -48,7 +48,10 @@ export default {
return this.approvalsLeft === 1 ? baseString : `${baseString}s`;
},
showApproveButton() {
- return this.userCanApprove && !this.userHasApproved;
+ return this.userCanApprove && !this.userHasApproved && this.approvalsLeft > 0;
+ },
+ showApproveAdditionallyButton() {
+ return this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0;
},
showSuggestedApprovers() {
return this.suggestedApprovers && this.suggestedApprovers.length;
@@ -95,6 +98,18 @@ export default {
:show-author-tooltip="true" />
</span>
</span>
+ <span v-if="showApproveAdditionallyButton" class="approvals-approve-button-wrap">
+ <button
+ :disabled="approving"
+ @click="approveMergeRequest"
+ class="btn btn-primary btn-sm approve-btn">
+ <i
+ v-if="approving"
+ class="fa fa-spinner fa-spin"
+ aria-hidden="true" />
+ Approve additionally
+ </button>
+ </span>
</div>
`,
};
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index 0cefa4277bf2759a..41734a646c1a3412 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -125,7 +125,7 @@ def can_approve?(user)
return false if user == author
return false unless user.can?(:update_merge_request, self)
- any_approver_allowed? && approvals.where(user: user).empty?
+ approvals.where(user: user).empty?
end
def has_approved?(user)
--
GitLab
From 7965c4d89007a0466b640d7d671459eda5d7c7c6 Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Tue, 16 Jan 2018 17:35:59 -0600
Subject: [PATCH 2/9] Corrected integration specs and added some javascript
improvements
---
.../components/approvals/approvals_body.js | 47 +++++++++++--------
ee/app/models/concerns/approvable.rb | 9 +---
.../user_approves_merge_request_spec.rb | 26 ++++++++++
3 files changed, 54 insertions(+), 28 deletions(-)
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 2d338c517a596cc7..acb8b1acc6d057b7 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
@@ -45,13 +45,31 @@ export default {
computed: {
approvalsRequiredStringified() {
const baseString = `${this.approvalsLeft} more approval`;
- return this.approvalsLeft === 1 ? baseString : `${baseString}s`;
+ let approvedString = '';
+ if (this.approvalsLeft >= 1) {
+ approvedString = this.approvalsLeft === 1 ? baseString : `${baseString}s`;
+ approvedString = `Requires ${approvedString}`;
+ } else {
+ approvedString = 'Approved';
+ }
+ return approvedString;
},
- showApproveButton() {
- return this.userCanApprove && !this.userHasApproved && this.approvalsLeft > 0;
+ approveButtonText() {
+ let approveButtonText = 'Approve';
+ if (this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0) {
+ approveButtonText = `${approveButtonText} additionally`;
+ }
+ return approveButtonText;
+ },
+ approveButtonClass() {
+ let approveButtonClass = '';
+ if (this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0) {
+ approveButtonClass = 'btn-inverted';
+ }
+ return approveButtonClass;
},
- showApproveAdditionallyButton() {
- return this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0;
+ showApproveButton() {
+ return this.userCanApprove && !this.userHasApproved;
},
showSuggestedApprovers() {
return this.suggestedApprovers && this.suggestedApprovers.length;
@@ -78,16 +96,17 @@ 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
@@ -98,18 +117,6 @@ export default {
:show-author-tooltip="true" />
</span>
</span>
- <span v-if="showApproveAdditionallyButton" class="approvals-approve-button-wrap">
- <button
- :disabled="approving"
- @click="approveMergeRequest"
- class="btn btn-primary btn-sm approve-btn">
- <i
- v-if="approving"
- class="fa fa-spinner fa-spin"
- aria-hidden="true" />
- Approve additionally
- </button>
- </span>
</div>
`,
};
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index 41734a646c1a3412..6cff3c685cbeb8ec 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -125,7 +125,7 @@ def can_approve?(user)
return false if user == author
return false unless user.can?(:update_merge_request, self)
- approvals.where(user: user).empty?
+ approvers_left.count == 0 && approvals.where(user: user).empty?
end
def has_approved?(user)
@@ -134,13 +134,6 @@ 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.
- #
- def any_approver_allowed?
- approvals_left > approvers_left.count
- end
-
def approved_by_users
approvals.map(&:user)
end
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 d7f452f7bdf8a588..1922df39a3b18482 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,31 @@
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('.approvers-list div').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)
--
GitLab
From 8b38f93b6a6daa681c6c8f2400488adf47460935 Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Wed, 17 Jan 2018 09:47:09 -0600
Subject: [PATCH 3/9] Corrected and added karma specs
---
.../approvals/approvals_body_spec.js | 37 ++++++++++++++++++-
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/spec/javascripts/approvals/approvals_body_spec.js b/spec/javascripts/approvals/approvals_body_spec.js
index ca93179994779936..fcff6da7c09d0208 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';
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';
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 = {}));
--
GitLab
From ece457539da0c7b7d57b243f9c8f8e3c086e409f Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Wed, 17 Jan 2018 09:47:45 -0600
Subject: [PATCH 4/9] Add CHANGELOG
---
changelogs/unreleased-ee/4134-approve-additionally.yml | 5 +++++
1 file changed, 5 insertions(+)
create mode 100644 changelogs/unreleased-ee/4134-approve-additionally.yml
diff --git a/changelogs/unreleased-ee/4134-approve-additionally.yml b/changelogs/unreleased-ee/4134-approve-additionally.yml
new file mode 100644
index 0000000000000000..1af99d39d0a4f97e
--- /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
--
GitLab
From d5f8bc596b3e7bc30ba1bfc706b2fea90335a1de Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Thu, 18 Jan 2018 12:10:01 -0600
Subject: [PATCH 5/9] Solved mr observations
---
.../components/approvals/approvals_body.js | 10 +++++-----
.../components/approvals/approvals_footer.js | 2 +-
.../merge_requests/user_approves_merge_request_spec.rb | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
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 acb8b1acc6d057b7..d8ba1c99c128db65 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__ } 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,19 +45,18 @@ export default {
},
computed: {
approvalsRequiredStringified() {
- const baseString = `${this.approvalsLeft} more approval`;
let approvedString = '';
if (this.approvalsLeft >= 1) {
- approvedString = this.approvalsLeft === 1 ? baseString : `${baseString}s`;
- approvedString = `Requires ${approvedString}`;
+ approvedString = n__('MergeRequest|Requires 1 more approval',
+ 'MergeRequest|Requires %d more approvals', this.approvalsLeft);
} else {
- approvedString = 'Approved';
+ approvedString = s__('MergeRequest|Approved');
}
return approvedString;
},
approveButtonText() {
let approveButtonText = 'Approve';
- if (this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0) {
+ if (this.approvalsLeft <= 0) {
approveButtonText = `${approveButtonText} additionally`;
}
return approveButtonText;
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 bfb9da7aff511f94..2145dc180d5d5cd9 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
@@ -69,7 +69,7 @@ export default {
class="approved-by-users approvals-footer clearfix mr-info-list">
<div class="approvers-prefix">
<p>Approved by</p>
- <div class="approvers-list">
+ <div class="approvers-list js-approvers-list">
<link-to-member-avatar
v-for="(approver, index) in approvedBy"
:key="index"
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 1922df39a3b18482..30054c65a7707ccd 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
@@ -41,7 +41,7 @@
sign_out(user)
sign_in_visit_merge_request(user2)
sign_in_visit_merge_request(user3)
- expect(all('.approvers-list div').count).to eq(3)
+ expect(all('.js-approvers-list div').count).to eq(3)
end
def sign_in_visit_merge_request(user)
--
GitLab
From 4cd1ef4dbd0d79f3b6cd187ebf0fca4dc8151d34 Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Tue, 23 Jan 2018 15:49:11 -0600
Subject: [PATCH 6/9] Changed approver logic
---
ee/app/models/concerns/approvable.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index 6cff3c685cbeb8ec..c0e9006f5b1f49c8 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -125,7 +125,7 @@ def can_approve?(user)
return false if user == author
return false unless user.can?(:update_merge_request, self)
- approvers_left.count == 0 && approvals.where(user: user).empty?
+ approvals.where(user: user).empty?
end
def has_approved?(user)
--
GitLab
From 7f83c5fb6c7eb2759237168d43e1b5ab1ce53dac Mon Sep 17 00:00:00 2001
From: Jose Ivan Vargas <jvargas@gitlab.com>
Date: Fri, 26 Jan 2018 12:40:28 -0600
Subject: [PATCH 7/9] Modified approver logic to be less strict
---
ee/app/models/concerns/approvable.rb | 6 +++++-
spec/models/merge_request_spec.rb | 17 +++++++++++++++++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index c0e9006f5b1f49c8..9076c429bffd8140 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -125,7 +125,11 @@ def can_approve?(user)
return false if user == author
return false unless user.can?(:update_merge_request, self)
- approvals.where(user: user).empty?
+ any_approver_allowed? && approvals.where(user: user).empty?
+ end
+
+ def any_approver_allowed?
+ approvers_left.count < approvals_required
end
def has_approved?(user)
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 22f08ad6193b89c9..728987a657c7691a 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,21 @@ def set_compare(merge_request)
expect(merge_request.can_approve?(stranger)).to be_falsey
expect(merge_request.can_approve?(nil)).to be_falsey
end
+
+ it 'only allows the approvers to approve the MR when only 1 approval approved' do
+ create(:approval, user: approver, merge_request: merge_request)
+ project.update_attributes(approvals_before_merge: 2)
+
+ 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
end
end
--
GitLab
From 1937b52939f77d08cacfb7eedae50351b5f10d20 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jarka=20Kadlecova=CC=81?= <jarka@gitlab.com>
Date: Tue, 30 Jan 2018 15:39:51 +0100
Subject: [PATCH 8/9] Allow approve MR to anyone when no required approval left
---
.../components/approvals/approvals_body.js | 25 +++++--------
.../components/approvals/approvals_footer.js | 17 ++++++---
.../approvals/mr_widget_approvals.js | 3 +-
ee/app/models/concerns/approvable.rb | 12 ++++--
.../user_approves_merge_request_spec.rb | 5 ++-
.../approvals/approvals_body_spec.js | 4 +-
spec/models/merge_request_spec.rb | 37 +++++++++++++------
7 files changed, 64 insertions(+), 39 deletions(-)
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 d8ba1c99c128db65..42bd0d615eecae82 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,4 +1,4 @@
-import { n__, s__ } from '~/locale';
+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';
@@ -45,28 +45,24 @@ export default {
},
computed: {
approvalsRequiredStringified() {
- let approvedString = '';
+ let approvedString = s__('MergeRequest|Approved');
if (this.approvalsLeft >= 1) {
- approvedString = n__('MergeRequest|Requires 1 more approval',
- 'MergeRequest|Requires %d more approvals', this.approvalsLeft);
- } else {
- approvedString = s__('MergeRequest|Approved');
+ approvedString = sprintf(n__('mrWidget|Requires 1 more approval by',
+ 'MergeRequest|Requires %d more approvals by', this.approvalsLeft));
}
return approvedString;
},
approveButtonText() {
- let approveButtonText = 'Approve';
+ let approveButtonText = s__('mrWidget|Approve');
if (this.approvalsLeft <= 0) {
- approveButtonText = `${approveButtonText} additionally`;
+ approveButtonText = s__('mrWidget|Approve additionally');
}
return approveButtonText;
},
approveButtonClass() {
- let approveButtonClass = '';
- if (this.userCanApprove && !this.userHasApproved && this.approvalsLeft <= 0) {
- approveButtonClass = 'btn-inverted';
- }
- return approveButtonClass;
+ return {
+ 'btn-inverted': this.showApproveButton && this.approvalsLeft <= 0,
+ };
},
showApproveButton() {
return this.userCanApprove && !this.userHasApproved;
@@ -86,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.'));
});
},
},
@@ -108,7 +104,6 @@ export default {
<span class="approvals-required-text bold">
{{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 2145dc180d5d5cd9..c8f13f2352d49fed 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>
- <div class="approvers-list js-approvers-list">
+ <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 ca2e08db8feca721..9db2aec34c4faea2 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 9076c429bffd8140..7c796b196564b273 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -128,16 +128,20 @@ def can_approve?(user)
any_approver_allowed? && approvals.where(user: user).empty?
end
- def any_approver_allowed?
- approvers_left.count < approvals_required
- end
-
def has_approved?(user)
return false unless user
approved_by_users.include?(user)
end
+ # 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 || approvals_left.zero?
+ end
+
def approved_by_users
approvals.map(&:user)
end
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 30054c65a7707ccd..32f08e04c1ec21a9 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
@@ -38,10 +38,13 @@
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-approvers-list div').count).to eq(3)
+
+ expect(all('.js-approver-list-member').count).to eq(3)
end
def sign_in_visit_merge_request(user)
diff --git a/spec/javascripts/approvals/approvals_body_spec.js b/spec/javascripts/approvals/approvals_body_spec.js
index fcff6da7c09d0208..4ffb656facd5154d 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 = 'Requires 1 more approval';
+ const correctText = 'Requires 1 more approval by';
expect(this.approvalsBody.approvalsRequiredStringified).toBe(correctText);
});
@@ -65,7 +65,7 @@ import ApprovalsBody from 'ee/vue_merge_request_widget/components/approvals/appr
this.approvalsBody.suggestedApprovers.push({ name: 'Approver 2' });
Vue.nextTick(() => {
- const correctText = 'Requires 2 more approvals';
+ const correctText = 'Requires 2 more approvals by';
expect(this.approvalsBody.approvalsRequiredStringified).toBe(correctText);
done();
});
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 728987a657c7691a..1dc583479ce3a5e2 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2044,19 +2044,34 @@ def set_compare(merge_request)
expect(merge_request.can_approve?(nil)).to be_falsey
end
- it 'only allows the approvers to approve the MR when only 1 approval approved' do
- create(:approval, user: approver, merge_request: merge_request)
- project.update_attributes(approvals_before_merge: 2)
+ 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?(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
+ 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
--
GitLab
From 0970041e6b7cc3c8d4f3ab7eaa8c6c8b94fcf284 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 7 Feb 2018 12:47:20 +0000
Subject: [PATCH 9/9] Reduce queries needed for computing approvers
---
ee/app/models/concerns/approvable.rb | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ee/app/models/concerns/approvable.rb b/ee/app/models/concerns/approvable.rb
index 7c796b196564b273..00928f43392a092d 100644
--- a/ee/app/models/concerns/approvable.rb
+++ b/ee/app/models/concerns/approvable.rb
@@ -139,7 +139,9 @@ def has_approved?(user)
# allow other project members to approve the MR.
#
def any_approver_allowed?
- approvals_left > approvers_left.count || approvals_left.zero?
+ remaining_approvals = approvals_left
+
+ remaining_approvals.zero? || remaining_approvals > approvers_left.count
end
def approved_by_users
--
GitLab