diff --git a/app/assets/javascripts/vue_merge_request_widget/ee/components/approvals/mr_widget_approvals.js b/app/assets/javascripts/vue_merge_request_widget/ee/components/approvals/mr_widget_approvals.js
index fa6d73268971641339b74f51642c3e835aafd932..2f6f9e16d336e2f73f7abb4528b7dc805607578b 100644
--- a/app/assets/javascripts/vue_merge_request_widget/ee/components/approvals/mr_widget_approvals.js
+++ b/app/assets/javascripts/vue_merge_request_widget/ee/components/approvals/mr_widget_approvals.js
@@ -15,39 +15,15 @@ export default {
required: true,
},
},
- data() {
- return {
- fetchingApprovals: true,
- };
- },
components: {
'approvals-body': ApprovalsBody,
'approvals-footer': ApprovalsFooter,
},
- created() {
- const flashErrorMessage = 'An error occured while retrieving approval data for this merge request.';
-
- this.service.fetchApprovals()
- .then((data) => {
- this.mr.setApprovals(data);
- this.fetchingApprovals = false;
- })
- .catch(() => new Flash(flashErrorMessage));
- },
template: `
-
- Checking approval status for this merge request.
-
-
-
-
`,
};
-
diff --git a/app/assets/javascripts/vue_merge_request_widget/ee/services/mr_widget_service.js b/app/assets/javascripts/vue_merge_request_widget/ee/services/mr_widget_service.js
index 544b7fc221ff72364784622072cd404f1fe89195..600a56d3c21c8645d4d5ef73cfb0b00b2aaceb7c 100644
--- a/app/assets/javascripts/vue_merge_request_widget/ee/services/mr_widget_service.js
+++ b/app/assets/javascripts/vue_merge_request_widget/ee/services/mr_widget_service.js
@@ -10,11 +10,6 @@ export default class MRWidgetService extends CEWidgetService {
this.rebaseResource = Vue.resource(mr.rebasePath);
}
- fetchApprovals() {
- return this.approvalsResource.get()
- .then(res => res.json());
- }
-
approveMergeRequest() {
return this.approvalsResource.save()
.then(res => res.json());
diff --git a/app/assets/javascripts/vue_merge_request_widget/ee/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/ee/stores/mr_widget_store.js
index cbc80c5910d165c8dc98d53d8d847a5cc84053c0..00b23972f8c17d99bf0ec3234f4c06dfd18b9141 100644
--- a/app/assets/javascripts/vue_merge_request_widget/ee/stores/mr_widget_store.js
+++ b/app/assets/javascripts/vue_merge_request_widget/ee/stores/mr_widget_store.js
@@ -37,7 +37,7 @@ export default class MergeRequestStore extends CEMergeRequestStore {
initApprovals(data) {
this.isApproved = this.isApproved || false;
- this.approvals = this.approvals || null;
+ this.approvals = data.approvals || null;
this.approvalsPath = data.approvals_path || this.approvalsPath;
this.approvalsRequired = Boolean(this.approvalsPath);
}
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9da909f0214a4484c719103511f29bb0fa5f8911..f52695227200cbe49465adf7271f318ccd3dfd86 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -13,7 +13,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
:pipeline_status, :ci_environments_status, :toggle_subscription, :cancel_merge_when_pipeline_succeeds,
:remove_wip, :resolve_conflicts, :assign_related_issues, :commit_change_content,
# EE
- :approve, :approvals, :unapprove, :rebase
+ :approve, :unapprove, :rebase
]
before_action :validates_merge_request, only: [:show, :diffs, :commits, :pipelines]
before_action :define_show_vars, only: [:diffs, :commits, :conflicts, :conflict_for_path, :builds, :pipelines]
@@ -466,10 +466,6 @@ def approve
render_approvals_json
end
- def approvals
- render_approvals_json
- end
-
def unapprove
if @merge_request.has_approved?(current_user)
::MergeRequests::RemoveApprovalService
diff --git a/app/serializers/approver_entity.rb b/app/serializers/approver_entity.rb
new file mode 100644
index 0000000000000000000000000000000000000000..13acc9f702eb3422f858c6c7c23d833982f3941f
--- /dev/null
+++ b/app/serializers/approver_entity.rb
@@ -0,0 +1,3 @@
+class ApproverEntity < Grape::Entity
+ expose :user, using: UserEntity
+end
diff --git a/app/serializers/merge_request_approvals_entity.rb b/app/serializers/merge_request_approvals_entity.rb
new file mode 100644
index 0000000000000000000000000000000000000000..5cf9f1ba9efb54ca656857346fd07a065abe7d97
--- /dev/null
+++ b/app/serializers/merge_request_approvals_entity.rb
@@ -0,0 +1,18 @@
+class MergeRequestApprovalsEntity < Grape::Entity
+ include RequestAwareEntity
+
+ expose :approvals_required
+ expose :approvals_left
+ expose :approvals, as: :approved_by, using: ApproverEntity
+ expose :approvers_left, as: :suggested_approvers, using: UserEntity
+
+ expose :user_can_approve do |merge_request, options|
+ merge_request.can_approve?(current_user)
+ end
+
+ expose :user_has_approved do |merge_request, options|
+ merge_request.has_approved?(current_user)
+ end
+
+ delegate :current_user, to: :request
+end
diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb
index 2ec506eb55eecde0e57cdcc93fdd687a3e16bec2..29de6dae7499c971877ad32878e3cfe4a6b5d0e3 100644
--- a/app/serializers/merge_request_basic_entity.rb
+++ b/app/serializers/merge_request_basic_entity.rb
@@ -9,4 +9,9 @@ class MergeRequestBasicEntity < Grape::Entity
expose :human_time_estimate
expose :human_total_time_spent
expose :rebase_in_progress?, as: :rebase_in_progress
+
+ # EE-specific
+ expose :approvals do |merge_request, options|
+ MergeRequestApprovalsEntity.represent merge_request, options
+ end
end
diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb
index 365080ba5b7a66fbc0db0ed8dd26cb442686062c..0382041b91252d5b2e930a213ee4f3fa5f64af08 100644
--- a/app/serializers/merge_request_entity.rb
+++ b/app/serializers/merge_request_entity.rb
@@ -214,6 +214,11 @@ class MergeRequestEntity < IssuableEntity
end
end
+ # EE-specific
+ expose :approvals do |merge_request, options|
+ MergeRequestApprovalsEntity.represent merge_request, options
+ end
+
private
delegate :current_user, to: :request
diff --git a/config/routes/project.rb b/config/routes/project.rb
index f3a98f45fe5893d222527da8abc94ddc39891708..ae0ea55c0df74d0ce39371cc97a84605eb8fbaf2 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -98,7 +98,6 @@
post :toggle_subscription
## EE-specific
- get :approvals
post :approvals, action: :approve
delete :approvals, action: :unapprove
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index e8c7680482ad113126999e647bdd64a91d30614e..5c6079bc1607faa8d40e909b73db32953dd618ae 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -190,28 +190,6 @@ def json_response
end
end
- describe 'approvals' do
- before do
- merge_request.approvals.create(user: approver)
- get :approvals,
- namespace_id: project.namespace.to_param,
- project_id: project.to_param,
- id: merge_request.iid,
- format: :json
- end
-
- it 'shows approval information' do
- expect(response).to be_success
- expect(json_response['approvals_left']).to eq 1
- expect(json_response['approved_by'].size).to eq 1
- expect(json_response['approved_by'][0]['user']['username']).to eq approver.username
- expect(json_response['user_has_approved']).to be false
- expect(json_response['user_can_approve']).to be true
- expect(json_response['suggested_approvers'].size).to eq 1
- expect(json_response['suggested_approvers'][0]['username']).to eq user.username
- end
- end
-
describe 'unapprove' do
before do
merge_request.approvals.create(user: user)
@@ -301,7 +279,7 @@ def go(extra_params = {})
recorded = ActiveRecord::QueryRecorder.new { go(format: :json) }
- expect(recorded.count).to be_within(5).of(30)
+ expect(recorded.count).to be_within(5).of(45)
expect(recorded.cached_count).to eq(0)
end
end
diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json
index c18675a3395f9508377a9c72e80d6b3960ef3fd6..6fcf02fefbd2ad0ae11439da3d4803c244c8bffe 100644
--- a/spec/fixtures/api/schemas/entities/merge_request.json
+++ b/spec/fixtures/api/schemas/entities/merge_request.json
@@ -108,7 +108,8 @@
"codeclimate": {
"head_path": { "type": "string" },
"base_path": { "type": "string" }
- }
+ },
+ "approvals": { "type": "hash" }
},
"additionalProperties": false
}
diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json
index 0f4b676d08cc0c637161cf399b8321bfcb8148f2..4657ee551af533d73cf12673673c5b919378fa94 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_basic.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json
@@ -10,7 +10,8 @@
"human_total_time_spent": { "type": ["string", "null"] },
"merge_error": { "type": ["string", "null"] },
"rebase_in_progress": { "type": "boolean" },
- "assignee_id": { "type": ["integer", "null"] }
+ "assignee_id": { "type": ["integer", "null"] },
+ "approvals": { "type": "hash" }
},
"additionalProperties": false
}
diff --git a/spec/javascripts/vue_mr_widget/ee/components/approvals/mr_widget_approvals_spec.js b/spec/javascripts/vue_mr_widget/ee/components/approvals/mr_widget_approvals_spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..61ef6b8a703cbe02e575a1c947b32e237385c60a
--- /dev/null
+++ b/spec/javascripts/vue_mr_widget/ee/components/approvals/mr_widget_approvals_spec.js
@@ -0,0 +1,126 @@
+import Vue from 'vue';
+import mrWidgetApprovals from '~/vue_merge_request_widget/ee/components/approvals/mr_widget_approvals';
+import Store from '~/vue_merge_request_widget/ee/stores/mr_widget_store';
+import Service from '~/vue_merge_request_widget/ee/services/mr_widget_service';
+import { approvalsData } from '../../../mock_data';
+
+function queryTestableElements($el) {
+ return {
+ requiredText: $el.querySelector('.approvals-required-text').textContent,
+ button: $el.querySelector('.approve-btn'),
+ footer: $el.querySelector('.approvals-footer'),
+ suggested: [...$el.querySelectorAll('.approvals-body img.avatar')],
+ approvers: [...$el.querySelectorAll('.approvals-footer img.avatar')],
+ };
+}
+
+describe('mrWidgetApprovals', () => {
+ let MRWidgetApprovalsComponent;
+ let store;
+ let service;
+ let mountComponent;
+ let vm;
+
+ beforeEach(() => {
+ MRWidgetApprovalsComponent = Vue.extend(mrWidgetApprovals);
+ store = new Store(approvalsData);
+ service = new Service('');
+
+ mountComponent = propsData => new MRWidgetApprovalsComponent({
+ propsData,
+ }).$mount();
+ });
+
+ describe('with no approvers', () => {
+ beforeEach(() => {
+ const newData = Object.assign({}, approvalsData.approvals);
+ newData.approvals_left = 2;
+ newData.approved_by = [];
+ newData.user_has_approved = false;
+
+ store.setApprovals(newData);
+
+ vm = mountComponent({
+ mr: store,
+ service,
+ });
+ });
+
+ it('requires 2 more approvals', () => {
+ const { requiredText, button, footer, suggested } = queryTestableElements(vm.$el);
+
+ expect(requiredText).toMatch('Requires 2 more approvals');
+ expect(button).toBeDefined();
+ expect(footer).toBeNull();
+ expect(suggested[0].src).toMatch('/suggested_avatar_url');
+ expect(suggested[1].src).toMatch('/suggested_avatar_url');
+ });
+ });
+
+ describe('with 1 approver and 1 left', () => {
+ beforeEach(() => {
+ const newData = Object.assign({}, approvalsData.approvals);
+ newData.approvals_left = 1;
+ newData.approved_by = [newData.approved_by[0]];
+ newData.user_has_approved = false;
+
+ store.setApprovals(newData);
+
+ vm = mountComponent({
+ mr: store,
+ service,
+ });
+ });
+
+ it('requires 1 more approval', () => {
+ const { requiredText, button, approvers, suggested } = queryTestableElements(vm.$el);
+
+ expect(requiredText).toMatch('Requires 1 more approval');
+ expect(button).toBeDefined();
+ expect(suggested[0].src).toMatch('/suggested_avatar_url');
+ expect(suggested[1].src).toMatch('/suggested_avatar_url');
+ expect(approvers[0].src).toMatch('/approver_avatar_url');
+ expect(approvers.length).toEqual(1);
+ });
+ });
+
+ describe('with 2 approvers, none left', () => {
+ beforeEach(() => {
+ vm = mountComponent({
+ mr: store,
+ service,
+ });
+ });
+
+ it('renders an approved state', () => {
+ const { requiredText, button, approvers, suggested } = queryTestableElements(vm.$el);
+
+ expect(requiredText).toMatch('Requires 0 more approvals');
+ expect(button).toBeNull();
+ expect(suggested[0].src).toMatch('/suggested_avatar_url');
+ expect(suggested[1].src).toMatch('/suggested_avatar_url');
+ expect(approvers[0].src).toMatch('/approver_avatar_url');
+ expect(approvers[1].src).toMatch('/approver_avatar_url');
+ });
+ });
+
+ describe('user cannot approve', () => {
+ beforeEach(() => {
+ const newData = Object.assign({}, approvalsData.approvals);
+ newData.user_can_approve = false;
+
+ store.setApprovals(newData);
+
+ vm = mountComponent({
+ mr: store,
+ service,
+ });
+ });
+
+ it('does not show approve button', () => {
+ const { button } = queryTestableElements(vm.$el);
+
+ expect(button).toBeNull();
+ });
+ });
+});
diff --git a/spec/javascripts/vue_mr_widget/ee/stores/mr_widget_store_spec.js b/spec/javascripts/vue_mr_widget/ee/stores/mr_widget_store_spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..3d53e8295878684cd76f11b0268b5ccbd24f2163
--- /dev/null
+++ b/spec/javascripts/vue_mr_widget/ee/stores/mr_widget_store_spec.js
@@ -0,0 +1,25 @@
+import MergeRequestStore from '~/vue_merge_request_widget/ee/stores/mr_widget_store';
+
+describe('MergeRequestStore', () => {
+ const store = new MergeRequestStore({ current_user: {} });
+
+ describe('initApprovals', () => {
+ let data = {};
+
+ it('sets approvals to approvals data array', () => {
+ data = { approvals: [{}, {}] };
+
+ store.initApprovals(data);
+
+ expect(store.approvals).toBe(data.approvals);
+ });
+
+ it('sets approvals to null if no approvals data', () => {
+ data = { approvals: undefined };
+
+ store.initApprovals(data);
+
+ expect(store.approvals).toBe(null);
+ });
+ });
+});
diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index 3b95971535556b618490b0ba30b4856a80d6a829..4486f0a5d57c913d4885ccb5c7aefb414c1c2b1b 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -275,3 +275,43 @@ export const baseIssues = [
"fingerprint": "ca2354534dee94ae60ba2f54e3857c50e5",
}
]
+
+// EE-specific
+export const approvalsData = {
+ current_user: {},
+ approvals_path: '/approvals',
+ approvals: {
+ approvals_required: 2,
+ approvals_left: 0,
+ approved_by: [
+ {
+ user: {
+ id: 1,
+ avatar_url: '/approver_avatar_url',
+ name: 'one',
+ web_url: '/approver_web_url',
+ },
+ },
+ {
+ user: {
+ id: 2,
+ avatar_url: '/approver_avatar_url',
+ name: 'two',
+ web_url: '/approver_web_url',
+ },
+ },
+ ],
+ suggested_approvers: [
+ {
+ id: 1,
+ avatar_url: '/suggested_avatar_url',
+ },
+ {
+ id: 2,
+ avatar_url: '/suggested_avatar_url',
+ },
+ ],
+ user_can_approve: true,
+ user_has_approved: true,
+ },
+};
diff --git a/spec/serializers/approver_entity_spec.rb b/spec/serializers/approver_entity_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..71dff3b1d19f768499abced3ac162a787e09b253
--- /dev/null
+++ b/spec/serializers/approver_entity_spec.rb
@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe ApproverEntity do
+ let(:user) { create(:user) }
+ let(:resource) { double('approver', user: user) }
+
+ subject(:entity) { described_class.new(resource).as_json }
+
+ it { is_expected.to include(user: UserEntity.represent(user).as_json) }
+end
diff --git a/spec/serializers/merge_request_approvals_entity_spec.rb b/spec/serializers/merge_request_approvals_entity_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..bd903303835607bee8db3bdff1bd83f2213ec91b
--- /dev/null
+++ b/spec/serializers/merge_request_approvals_entity_spec.rb
@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+describe MergeRequestApprovalsEntity do
+ let(:project) { create(:empty_project, approvals_before_merge: 2) }
+ let(:user) { create(:user) }
+ let(:other_user) { create(:user) }
+ let(:resource) { create(:merge_request, source_project: project) }
+ let(:request) { double('request', current_user: user) }
+
+ subject(:entity) { described_class.new(resource, request: request).as_json }
+
+ before do
+ project.add_master(user)
+
+ resource.approvals.create!(user: user)
+ resource.approvers.create!(user: other_user)
+ end
+
+ it 'exposes approvals properties' do
+ is_expected.to eq(
+ approvals_required: 2,
+ approvals_left: 1,
+ approved_by: [{ user: UserEntity.represent(user).as_json }],
+ suggested_approvers: [UserEntity.represent(other_user).as_json],
+ user_can_approve: false,
+ user_has_approved: true
+ )
+ end
+end
diff --git a/spec/serializers/merge_request_basic_entity_spec.rb b/spec/serializers/merge_request_basic_entity_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..6a922dab21bf613f1004f757870b5d13af09a8f2
--- /dev/null
+++ b/spec/serializers/merge_request_basic_entity_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe MergeRequestBasicEntity do
+ let(:project) { create :empty_project }
+ let(:resource) { create(:merge_request, source_project: project) }
+ let(:user) { create(:user) }
+
+ let(:request) { double('request', current_user: user) }
+
+ subject(:entity) { described_class.new(resource, request: request).as_json }
+
+ it 'exposes approvals' do
+ approvals = MergeRequestApprovalsEntity
+ .represent(resource, request: request)
+ .as_json
+
+ expect(entity[:approvals]).to eq(approvals)
+ end
+end
diff --git a/spec/serializers/merge_request_basic_serializer_spec.rb b/spec/serializers/merge_request_basic_serializer_spec.rb
index 4daf5a59d0ccf9498a6e49cd3ca608d50f6c8628..6ddf4340bd4f26c19e2fac4f6fb4b5bb06926752 100644
--- a/spec/serializers/merge_request_basic_serializer_spec.rb
+++ b/spec/serializers/merge_request_basic_serializer_spec.rb
@@ -4,9 +4,7 @@
let(:resource) { create(:merge_request) }
let(:user) { create(:user) }
- subject { described_class.new.represent(resource) }
+ subject(:entity) { described_class.new(current_user: user).represent(resource) }
- it 'has important MergeRequest attributes' do
- expect(subject).to include(:merge_status)
- end
+ it { is_expected.to include(:merge_status) }
end
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb
index bb8ed4fb40485441f82a6dc62945be4a2b1d0b01..c7795b6d96011b4bd2579b615dc860206e25ab78 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/spec/serializers/merge_request_entity_spec.rb
@@ -162,4 +162,13 @@
expect(entity[:rebase_path]).to be_nil
end
end
+
+ # EE-specific
+ it 'exposes approvals' do
+ approvals = MergeRequestApprovalsEntity
+ .represent(resource, request: request)
+ .as_json
+
+ expect(subject[:approvals]).to eq(approvals)
+ end
end