From ad9dc26c100ae50405510f9ae49e35928f7f13a5 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Sun, 18 Jun 2017 02:26:58 +0100
Subject: [PATCH 1/8] Added merge_request_approvals_entity and approvers_entity
 and present them in merge_request_basic_entity and merge_request_entity.
 Removed call to approvals-only endpoint and correctly set the MR store
 approvals data.

---
 .../approvals/mr_widget_approvals.js          | 25 -------------------
 .../ee/stores/mr_widget_store.js              |  2 +-
 app/serializers/approvers_entity.rb           |  3 +++
 .../merge_request_approvals_entity.rb         | 18 +++++++++++++
 app/serializers/merge_request_basic_entity.rb |  5 ++++
 app/serializers/merge_request_entity.rb       |  5 ++++
 6 files changed, 32 insertions(+), 26 deletions(-)
 create mode 100644 app/serializers/approvers_entity.rb
 create mode 100644 app/serializers/merge_request_approvals_entity.rb

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 fa6d7326897164..2f6f9e16d336e2 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: `
     <div
       v-if="mr.approvalsRequired"
       class="mr-widget-approvals-container mr-widget-body">
       <div
-        v-show="fetchingApprovals"
-        class="mr-approvals-loading-state">
-        <span class="approvals-loading-text bold">
-          Checking approval status for this merge request.
-        </span>
-        <i class="fa fa-spinner fa-spin" />
-      </div>
-      <div
-        v-if="!fetchingApprovals"
         class="approvals-components">
         <approvals-body
           :mr="mr"
@@ -68,4 +44,3 @@ export default {
     </div>
     `,
 };
-
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 cbc80c5910d165..00b23972f8c17d 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/serializers/approvers_entity.rb b/app/serializers/approvers_entity.rb
new file mode 100644
index 00000000000000..0edeab3d43b607
--- /dev/null
+++ b/app/serializers/approvers_entity.rb
@@ -0,0 +1,3 @@
+class ApproversEntity < 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 00000000000000..3b17ec03d6c797
--- /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: ApproversEntity
+  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 2ec506eb55eecd..29de6dae7499c9 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 365080ba5b7a66..0382041b91252d 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
-- 
GitLab


From 9d7e3d6141f66a20e4192b3a1450ebb20ec29a77 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Tue, 20 Jun 2017 12:32:00 +0100
Subject: [PATCH 2/8] Added merge_request_basic_entity_spec.rb
 merge_request_approvals_entity_spec.rb approver_entity_spec.rb
 approver_entity.rb mr_widget_approvals_spec.js and mr_widget_store_spec.js as
 well as updating current specs

---
 .../ee/services/mr_widget_service.js          |   5 -
 .../projects/merge_requests_controller.rb     |   6 +-
 ...approvers_entity.rb => approver_entity.rb} |   2 +-
 .../merge_request_approvals_entity.rb         |   2 +-
 config/routes/project.rb                      |   1 -
 .../merge_requests_controller_spec.rb         |  22 ---
 .../approvals/mr_widget_approvals_spec.js     | 126 ++++++++++++++++++
 .../ee/stores/mr_widget_store_spec.js         |  25 ++++
 spec/javascripts/vue_mr_widget/mock_data.js   |  39 ++++++
 spec/serializers/approver_entity_spec.rb      |  16 +++
 .../merge_request_approvals_entity_spec.rb    |  37 +++++
 .../merge_request_basic_entity_spec.rb        |  21 +++
 spec/serializers/merge_request_entity_spec.rb |   8 ++
 13 files changed, 275 insertions(+), 35 deletions(-)
 rename app/serializers/{approvers_entity.rb => approver_entity.rb} (50%)
 create mode 100644 spec/javascripts/vue_mr_widget/ee/components/approvals/mr_widget_approvals_spec.js
 create mode 100644 spec/javascripts/vue_mr_widget/ee/stores/mr_widget_store_spec.js
 create mode 100644 spec/serializers/approver_entity_spec.rb
 create mode 100644 spec/serializers/merge_request_approvals_entity_spec.rb
 create mode 100644 spec/serializers/merge_request_basic_entity_spec.rb

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 544b7fc221ff72..600a56d3c21c86 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/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 9da909f0214a44..f52695227200cb 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/approvers_entity.rb b/app/serializers/approver_entity.rb
similarity index 50%
rename from app/serializers/approvers_entity.rb
rename to app/serializers/approver_entity.rb
index 0edeab3d43b607..13acc9f702eb34 100644
--- a/app/serializers/approvers_entity.rb
+++ b/app/serializers/approver_entity.rb
@@ -1,3 +1,3 @@
-class ApproversEntity < Grape::Entity
+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
index 3b17ec03d6c797..5cf9f1ba9efb54 100644
--- a/app/serializers/merge_request_approvals_entity.rb
+++ b/app/serializers/merge_request_approvals_entity.rb
@@ -3,7 +3,7 @@ class MergeRequestApprovalsEntity < Grape::Entity
 
   expose :approvals_required
   expose :approvals_left
-  expose :approvals, as: :approved_by, using: ApproversEntity
+  expose :approvals, as: :approved_by, using: ApproverEntity
   expose :approvers_left, as: :suggested_approvers, using: UserEntity
 
   expose :user_can_approve do |merge_request, options|
diff --git a/config/routes/project.rb b/config/routes/project.rb
index f3a98f45fe5893..ae0ea55c0df74d 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 e8c7680482ad11..ebf9e8ab8122c8 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)
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 00000000000000..61ef6b8a703cbe
--- /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 00000000000000..3d53e829587868
--- /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 3b95971535556b..a9fe68b7ab7e42 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -275,3 +275,42 @@ export const baseIssues = [
     "fingerprint": "ca2354534dee94ae60ba2f54e3857c50e5",
   }
 ]
+
+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 00000000000000..777acc82f7805c
--- /dev/null
+++ b/spec/serializers/approver_entity_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+describe ApproverEntity do
+  let(:user)     { create(:user) }
+  let(:resource) { double('approver', user: user) }
+
+  subject do
+    described_class.new(resource).as_json
+  end
+
+  it 'exposes user' do
+    user_payload = UserEntity.represent(user).as_json
+
+    expect(subject[:user]).to eq(user_payload)
+  end
+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 00000000000000..dc3e8d512ffe4c
--- /dev/null
+++ b/spec/serializers/merge_request_approvals_entity_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe MergeRequestApprovalsEntity do
+  let(:project)  { create :empty_project }
+  let(:resource) { create(:merge_request, source_project: project, target_project: project) }
+  let(:user)     { create(:user) }
+
+  let(:request) { double('request', current_user: user) }
+
+  subject do
+    described_class.new(resource, request: request).as_json
+  end
+
+  it 'exposes approvals_required' do
+    expect(subject[:approvals_required]).to be_a(Numeric)
+  end
+
+  it 'exposes approvals_left' do
+    expect(subject[:approvals_left]).to be_a(Numeric)
+  end
+
+  it 'exposes approved_by' do
+    expect(subject[:approved_by]).to be_a(Array)
+  end
+
+  it 'exposes suggested_approvers' do
+    expect(subject[:suggested_approvers]).to be_a(Array)
+  end
+
+  it 'exposes user_can_approve' do
+    expect(subject[:user_can_approve]).to be(resource.can_approve?(user))
+  end
+
+  it 'exposes user_has_approved' do
+    expect(subject[:user_has_approved]).to be(resource.has_approved?(user))
+  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 00000000000000..c417aef7880986
--- /dev/null
+++ b/spec/serializers/merge_request_basic_entity_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe MergeRequestBasicEntity do
+  let(:project)  { create :empty_project }
+  let(:resource) { create(:merge_request, source_project: project, target_project: project) }
+  let(:user)     { create(:user) }
+
+  let(:request) { double('request', current_user: user) }
+
+  subject do
+    described_class.new(resource, request: request).as_json
+  end
+
+  it 'exposes approvals' do
+    approvals = MergeRequestApprovalsEntity
+      .represent(resource, request: request)
+      .as_json
+
+    expect(subject[:approvals]).to eq(approvals)
+  end
+end
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb
index bb8ed4fb404854..28779dc77d49c8 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/spec/serializers/merge_request_entity_spec.rb
@@ -162,4 +162,12 @@
       expect(entity[:rebase_path]).to be_nil
     end
   end
+
+  it 'exposes approvals' do
+    approvals = MergeRequestApprovalsEntity
+      .represent(resource, request: request)
+      .as_json
+
+    expect(subject[:approvals]).to eq(approvals)
+  end
 end
-- 
GitLab


From 7d85fd5fb61a43f4f67a3f0d64063434f65058a9 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Tue, 20 Jun 2017 21:54:41 +0100
Subject: [PATCH 3/8] Tidy up approver_entity_spec.rb
 merge_request_approvals_entity_spec.rb merge_request_basic_entity_spec.rb

---
 spec/serializers/approver_entity_spec.rb      | 10 +----
 .../merge_request_approvals_entity_spec.rb    | 40 +++++++------------
 .../merge_request_basic_entity_spec.rb        |  8 ++--
 3 files changed, 20 insertions(+), 38 deletions(-)

diff --git a/spec/serializers/approver_entity_spec.rb b/spec/serializers/approver_entity_spec.rb
index 777acc82f7805c..71dff3b1d19f76 100644
--- a/spec/serializers/approver_entity_spec.rb
+++ b/spec/serializers/approver_entity_spec.rb
@@ -4,13 +4,7 @@
   let(:user)     { create(:user) }
   let(:resource) { double('approver', user: user) }
 
-  subject do
-    described_class.new(resource).as_json
-  end
+  subject(:entity) { described_class.new(resource).as_json }
 
-  it 'exposes user' do
-    user_payload = UserEntity.represent(user).as_json
-
-    expect(subject[:user]).to eq(user_payload)
-  end
+  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
index dc3e8d512ffe4c..1bb39ffc121736 100644
--- a/spec/serializers/merge_request_approvals_entity_spec.rb
+++ b/spec/serializers/merge_request_approvals_entity_spec.rb
@@ -1,37 +1,27 @@
 require 'spec_helper'
 
 describe MergeRequestApprovalsEntity do
-  let(:project)  { create :empty_project }
-  let(:resource) { create(:merge_request, source_project: project, target_project: project) }
+  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 do
-    described_class.new(resource, request: request).as_json
-  end
+  subject(:entity) { described_class.new(resource, request: request).as_json }
 
-  it 'exposes approvals_required' do
-    expect(subject[:approvals_required]).to be_a(Numeric)
-  end
-
-  it 'exposes approvals_left' do
-    expect(subject[:approvals_left]).to be_a(Numeric)
-  end
-
-  it 'exposes approved_by' do
-    expect(subject[:approved_by]).to be_a(Array)
-  end
-
-  it 'exposes suggested_approvers' do
-    expect(subject[:suggested_approvers]).to be_a(Array)
-  end
+  before do
+    project.add_master(user)
 
-  it 'exposes user_can_approve' do
-    expect(subject[:user_can_approve]).to be(resource.can_approve?(user))
+    resource.approvals.create(user: user)
+    resource.approvers.create(user: other_user)
   end
 
-  it 'exposes user_has_approved' do
-    expect(subject[:user_has_approved]).to be(resource.has_approved?(user))
+  it 'exposes approvals properties' do
+    expect(entity[:approvals_required]).to eq(2)
+    expect(entity[:approvals_left]).to eq(1)
+    expect(entity[:approved_by].to_json).to eq([user: UserEntity.represent(user)].to_json)
+    expect(entity[:suggested_approvers].to_json).to eq([UserEntity.represent(other_user)].to_json)
+    expect(entity[:user_can_approve]).to eq(resource.can_approve?(user))
+    expect(entity[:user_has_approved]).to eq(resource.has_approved?(user))
   end
 end
diff --git a/spec/serializers/merge_request_basic_entity_spec.rb b/spec/serializers/merge_request_basic_entity_spec.rb
index c417aef7880986..6a922dab21bf61 100644
--- a/spec/serializers/merge_request_basic_entity_spec.rb
+++ b/spec/serializers/merge_request_basic_entity_spec.rb
@@ -2,20 +2,18 @@
 
 describe MergeRequestBasicEntity do
   let(:project)  { create :empty_project }
-  let(:resource) { create(:merge_request, source_project: project, target_project: project) }
+  let(:resource) { create(:merge_request, source_project: project) }
   let(:user)     { create(:user) }
 
   let(:request) { double('request', current_user: user) }
 
-  subject do
-    described_class.new(resource, request: request).as_json
-  end
+  subject(:entity) { described_class.new(resource, request: request).as_json }
 
   it 'exposes approvals' do
     approvals = MergeRequestApprovalsEntity
       .represent(resource, request: request)
       .as_json
 
-    expect(subject[:approvals]).to eq(approvals)
+    expect(entity[:approvals]).to eq(approvals)
   end
 end
-- 
GitLab


From 7a50ae05f324b109330828430cb29abe74e77db1 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Wed, 21 Jun 2017 10:32:55 +0100
Subject: [PATCH 4/8] Pass current_user to serializer options in
 merge_request_basic_serializer_spec.rb

---
 spec/serializers/merge_request_basic_serializer_spec.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/spec/serializers/merge_request_basic_serializer_spec.rb b/spec/serializers/merge_request_basic_serializer_spec.rb
index 4daf5a59d0ccf9..6ddf4340bd4f26 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
-- 
GitLab


From af62eb581bd1e8932f0ad8192530d3e42147ea7b Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Wed, 21 Jun 2017 10:46:14 +0100
Subject: [PATCH 5/8] Updated spec fixture entity schemas for merge_request and
 merge_request_basic

---
 spec/fixtures/api/schemas/entities/merge_request.json       | 3 ++-
 spec/fixtures/api/schemas/entities/merge_request_basic.json | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request.json
index c18675a3395f95..6fcf02fefbd2ad 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 0f4b676d08cc0c..4657ee551af533 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
 }
-- 
GitLab


From 3ddda803840781284ad8ad37c5aed334bc6396e8 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Wed, 21 Jun 2017 10:46:37 +0100
Subject: [PATCH 6/8] Add EE-specific comments to merge_request_entity_spec.rb
 and mock_data.js

---
 spec/javascripts/vue_mr_widget/mock_data.js   | 1 +
 spec/serializers/merge_request_entity_spec.rb | 1 +
 2 files changed, 2 insertions(+)

diff --git a/spec/javascripts/vue_mr_widget/mock_data.js b/spec/javascripts/vue_mr_widget/mock_data.js
index a9fe68b7ab7e42..4486f0a5d57c91 100644
--- a/spec/javascripts/vue_mr_widget/mock_data.js
+++ b/spec/javascripts/vue_mr_widget/mock_data.js
@@ -276,6 +276,7 @@ export const baseIssues = [
   }
 ]
 
+// EE-specific
 export const approvalsData = {
   current_user: {},
   approvals_path: '/approvals',
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_entity_spec.rb
index 28779dc77d49c8..c7795b6d96011b 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/spec/serializers/merge_request_entity_spec.rb
@@ -163,6 +163,7 @@
     end
   end
 
+  # EE-specific
   it 'exposes approvals' do
     approvals = MergeRequestApprovalsEntity
       .represent(resource, request: request)
-- 
GitLab


From e127e191e84cadf1141def73a90d8ba9a282f026 Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Wed, 21 Jun 2017 10:49:19 +0100
Subject: [PATCH 7/8] Increase expected number of queries for
 merge_requests_controller GET show as json

---
 spec/controllers/projects/merge_requests_controller_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index ebf9e8ab8122c8..5c6079bc1607fa 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -279,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
-- 
GitLab


From 24a891b04a7a2f6ac87572056774c641c546bb0a Mon Sep 17 00:00:00 2001
From: "Luke \"Jared\" Bennett" <lbennett@gitlab.com>
Date: Wed, 21 Jun 2017 16:59:44 +0100
Subject: [PATCH 8/8] Tidy up merge_request_approvals_entity_spec.rb

---
 .../merge_request_approvals_entity_spec.rb    | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/spec/serializers/merge_request_approvals_entity_spec.rb b/spec/serializers/merge_request_approvals_entity_spec.rb
index 1bb39ffc121736..bd903303835607 100644
--- a/spec/serializers/merge_request_approvals_entity_spec.rb
+++ b/spec/serializers/merge_request_approvals_entity_spec.rb
@@ -3,7 +3,7 @@
 describe MergeRequestApprovalsEntity do
   let(:project)  { create(:empty_project, approvals_before_merge: 2) }
   let(:user)     { create(:user) }
-  let(:other_user)     { create(:user) }
+  let(:other_user) { create(:user) }
   let(:resource) { create(:merge_request, source_project: project) }
   let(:request) { double('request', current_user: user) }
 
@@ -12,16 +12,18 @@
   before do
     project.add_master(user)
 
-    resource.approvals.create(user: user)
-    resource.approvers.create(user: other_user)
+    resource.approvals.create!(user: user)
+    resource.approvers.create!(user: other_user)
   end
 
   it 'exposes approvals properties' do
-    expect(entity[:approvals_required]).to eq(2)
-    expect(entity[:approvals_left]).to eq(1)
-    expect(entity[:approved_by].to_json).to eq([user: UserEntity.represent(user)].to_json)
-    expect(entity[:suggested_approvers].to_json).to eq([UserEntity.represent(other_user)].to_json)
-    expect(entity[:user_can_approve]).to eq(resource.can_approve?(user))
-    expect(entity[:user_has_approved]).to eq(resource.has_approved?(user))
+    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
-- 
GitLab