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