From f77e26d4fbf97f42bf4d3d64c3f4146110725832 Mon Sep 17 00:00:00 2001
From: Oswaldo Ferreira <oswaldo@gitlab.com>
Date: Tue, 17 Oct 2017 13:07:32 +0200
Subject: Load participants async BE changes

---
 .../services/mr_widget_service.js                  |  2 +-
 app/controllers/projects/issues_controller.rb      |  2 +-
 .../projects/merge_requests_controller.rb          |  2 +-
 app/helpers/issuables_helper.rb                    | 22 ++++++-----
 app/models/concerns/subscribable.rb                |  2 +
 app/serializers/issuable_entity.rb                 | 10 -----
 app/serializers/issuable_sidebar_entity.rb         | 16 ++++++++
 app/serializers/issue_serializer.rb                | 15 +++++++-
 app/serializers/issue_sidebar_entity.rb            |  3 ++
 app/serializers/merge_request_basic_entity.rb      | 18 +--------
 app/serializers/merge_request_serializer.rb        |  9 ++++-
 .../projects/merge_requests_controller_spec.rb     |  6 +--
 spec/features/boards/boards_spec.rb                |  2 +-
 spec/fixtures/api/schemas/entities/issue.json      | 44 ++++++++++++++++++++++
 .../api/schemas/entities/issue_sidebar.json        | 21 +++++++++++
 spec/fixtures/api/schemas/entities/label.json      | 26 +++++++++++++
 .../api/schemas/entities/merge_request_basic.json  |  4 +-
 spec/fixtures/api/schemas/issue.json               | 27 +------------
 spec/models/concerns/subscribable_spec.rb          |  6 +++
 spec/serializers/issue_serializer_spec.rb          | 27 +++++++++++++
 .../merge_request_basic_serializer_spec.rb         | 10 +++--
 spec/serializers/merge_request_serializer_spec.rb  | 12 +++---
 22 files changed, 204 insertions(+), 82 deletions(-)
 create mode 100644 app/serializers/issuable_sidebar_entity.rb
 create mode 100644 app/serializers/issue_sidebar_entity.rb
 create mode 100644 spec/fixtures/api/schemas/entities/issue.json
 create mode 100644 spec/fixtures/api/schemas/entities/issue_sidebar.json
 create mode 100644 spec/fixtures/api/schemas/entities/label.json
 create mode 100644 spec/serializers/issue_serializer_spec.rb

diff --git a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js
index 79c3d335679..99f5c305df5 100644
--- a/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js
+++ b/app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js
@@ -11,7 +11,7 @@ export default class MRWidgetService {
     this.removeWIPResource = Vue.resource(endpoints.removeWIPPath);
     this.removeSourceBranchResource = Vue.resource(endpoints.sourceBranchPath);
     this.deploymentsResource = Vue.resource(endpoints.ciEnvironmentsStatusPath);
-    this.pollResource = Vue.resource(`${endpoints.statusPath}?basic=true`);
+    this.pollResource = Vue.resource(`${endpoints.statusPath}?serializer=basic`);
     this.mergeActionsContentResource = Vue.resource(endpoints.mergeActionsContentPath);
   }
 
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index fe1334c0cfe..6a5e4538717 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -74,7 +74,7 @@ class Projects::IssuesController < Projects::ApplicationController
     respond_to do |format|
       format.html
       format.json do
-        render json: serializer.represent(@issue)
+        render json: serializer.represent(@issue, serializer: params[:serializer])
       end
     end
   end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index c5204080333..2b0294c8387 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -83,7 +83,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       format.json do
         Gitlab::PollingInterval.set_header(response, interval: 10_000)
 
-        render json: serializer.represent(@merge_request, basic: params[:basic])
+        render json: serializer.represent(@merge_request, serializer: params[:serializer])
       end
 
       format.patch  do
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 09621c886ba..d0069cd48cf 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -33,15 +33,17 @@ module IssuablesHelper
   end
 
   def serialize_issuable(issuable)
-    case issuable
-    when Issue
-      IssueSerializer.new(current_user: current_user, project: issuable.project).represent(issuable).to_json
-    when MergeRequest
-      MergeRequestSerializer
-        .new(current_user: current_user, project: issuable.project)
-        .represent(issuable)
-        .to_json
-    end
+    serializer_klass = case issuable
+                       when Issue
+                         IssueSerializer
+                       when MergeRequest
+                         MergeRequestSerializer
+                       end
+
+    serializer_klass
+      .new(current_user: current_user, project: issuable.project)
+      .represent(issuable)
+      .to_json
   end
 
   def template_dropdown_tag(issuable, &block)
@@ -357,7 +359,7 @@ module IssuablesHelper
 
   def issuable_sidebar_options(issuable, can_edit_issuable)
     {
-      endpoint: "#{issuable_json_path(issuable)}?basic=true",
+      endpoint: "#{issuable_json_path(issuable)}?serializer=sidebar",
       toggleSubscriptionEndpoint: toggle_subscription_path(issuable),
       moveIssueEndpoint: move_namespace_project_issue_path(namespace_id: issuable.project.namespace.to_param, project_id: issuable.project, id: issuable),
       projectsAutocompleteEndpoint: autocomplete_projects_path(project_id: @project.id),
diff --git a/app/models/concerns/subscribable.rb b/app/models/concerns/subscribable.rb
index 274b38a7708..f478c8ede18 100644
--- a/app/models/concerns/subscribable.rb
+++ b/app/models/concerns/subscribable.rb
@@ -13,6 +13,8 @@ module Subscribable
   end
 
   def subscribed?(user, project = nil)
+    return false unless user
+
     if subscription = subscriptions.find_by(user: user, project: project)
       subscription.subscribed
     else
diff --git a/app/serializers/issuable_entity.rb b/app/serializers/issuable_entity.rb
index 91efad08ef8..61c7a428745 100644
--- a/app/serializers/issuable_entity.rb
+++ b/app/serializers/issuable_entity.rb
@@ -17,14 +17,4 @@ class IssuableEntity < Grape::Entity
   expose :human_total_time_spent
   expose :milestone, using: API::Entities::Milestone
   expose :labels, using: LabelEntity
-  expose :participants, using: ::API::Entities::UserBasic do |issuable|
-    issuable.participants(current_user)
-  end
-  expose :subscribed do |issuable|
-    issuable.subscribed?(current_user)
-  end
-
-  private
-
-  delegate :current_user, to: :request
 end
diff --git a/app/serializers/issuable_sidebar_entity.rb b/app/serializers/issuable_sidebar_entity.rb
new file mode 100644
index 00000000000..ff23d8bf0c7
--- /dev/null
+++ b/app/serializers/issuable_sidebar_entity.rb
@@ -0,0 +1,16 @@
+class IssuableSidebarEntity < Grape::Entity
+  include RequestAwareEntity
+
+  expose :participants, using: ::API::Entities::UserBasic do |issuable|
+    issuable.participants(request.current_user)
+  end
+
+  expose :subscribed do |issuable|
+    issuable.subscribed?(request.current_user, issuable.project)
+  end
+
+  expose :time_estimate
+  expose :total_time_spent
+  expose :human_time_estimate
+  expose :human_total_time_spent
+end
diff --git a/app/serializers/issue_serializer.rb b/app/serializers/issue_serializer.rb
index 4fff54a9126..2555595379b 100644
--- a/app/serializers/issue_serializer.rb
+++ b/app/serializers/issue_serializer.rb
@@ -1,3 +1,16 @@
 class IssueSerializer < BaseSerializer
-  entity IssueEntity
+  # This overrided method takes care of which entity should be used
+  # to serialize the `issue` based on `basic` key in `opts` param.
+  # Hence, `entity` doesn't need to be declared on the class scope.
+  def represent(merge_request, opts = {})
+    entity =
+      case opts[:serializer]
+      when 'sidebar'
+        IssueSidebarEntity
+      else
+        IssueEntity
+      end
+
+    super(merge_request, opts, entity)
+  end
 end
diff --git a/app/serializers/issue_sidebar_entity.rb b/app/serializers/issue_sidebar_entity.rb
new file mode 100644
index 00000000000..6c823dbfe95
--- /dev/null
+++ b/app/serializers/issue_sidebar_entity.rb
@@ -0,0 +1,3 @@
+class IssueSidebarEntity < IssuableSidebarEntity
+  expose :assignees, using: API::Entities::UserBasic
+end
diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb
index 70a60f19d23..d54a6516aed 100644
--- a/app/serializers/merge_request_basic_entity.rb
+++ b/app/serializers/merge_request_basic_entity.rb
@@ -1,23 +1,7 @@
-class MergeRequestBasicEntity < Grape::Entity
-  include RequestAwareEntity
-
+class MergeRequestBasicEntity < IssuableSidebarEntity
   expose :assignee_id
   expose :merge_status
   expose :merge_error
   expose :state
   expose :source_branch_exists?, as: :source_branch_exists
-  expose :time_estimate
-  expose :total_time_spent
-  expose :human_time_estimate
-  expose :human_total_time_spent
-  expose :participants, using: ::API::Entities::UserBasic do |issuable|
-    issuable.participants(current_user)
-  end
-  expose :subscribed do |issuable|
-    issuable.subscribed?(current_user)
-  end
-
-  private
-
-  delegate :current_user, to: :request
 end
diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb
index f67034ce47a..e9d98d8baca 100644
--- a/app/serializers/merge_request_serializer.rb
+++ b/app/serializers/merge_request_serializer.rb
@@ -3,7 +3,14 @@ class MergeRequestSerializer < BaseSerializer
   # to serialize the `merge_request` based on `basic` key in `opts` param.
   # Hence, `entity` doesn't need to be declared on the class scope.
   def represent(merge_request, opts = {})
-    entity = opts[:basic] ? MergeRequestBasicEntity : MergeRequestEntity
+    entity =
+      case opts[:serializer]
+      when 'basic', 'sidebar'
+        MergeRequestBasicEntity
+      else
+        MergeRequestEntity
+      end
+
     super(merge_request, opts, entity)
   end
 end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index b7cccddefdd..52ef8c6a589 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -83,15 +83,15 @@ describe Projects::MergeRequestsController do
     end
 
     describe 'as json' do
-      context 'with basic param' do
+      context 'with basic serializer param' do
         it 'renders basic MR entity as json' do
-          go(basic: true, format: :json)
+          go(serializer: 'basic', format: :json)
 
           expect(response).to match_response_schema('entities/merge_request_basic')
         end
       end
 
-      context 'without basic param' do
+      context 'without basic serializer param' do
         it 'renders the merge request in the json format' do
           go(format: :json)
 
diff --git a/spec/features/boards/boards_spec.rb b/spec/features/boards/boards_spec.rb
index 60ed17c0c81..ebe6939df4c 100644
--- a/spec/features/boards/boards_spec.rb
+++ b/spec/features/boards/boards_spec.rb
@@ -538,7 +538,7 @@ describe 'Issue Boards', :js do
     end
 
     it 'does not show create new list' do
-      expect(page).not_to have_selector('.js-new-board-list')
+      expect(page).not_to have_button('.js-new-board-list')
     end
 
     it 'does not allow dragging' do
diff --git a/spec/fixtures/api/schemas/entities/issue.json b/spec/fixtures/api/schemas/entities/issue.json
new file mode 100644
index 00000000000..3d3329a3406
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/issue.json
@@ -0,0 +1,44 @@
+{
+  "type": "object",
+  "properties" : {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "author_id": { "type": "integer" },
+    "description": { "type": ["string", "null"] },
+    "lock_version": { "type": ["string", "null"] },
+    "milestone_id": { "type": ["string", "null"] },
+    "title": { "type": "string" },
+    "moved_to_id": { "type": ["integer", "null"] },
+    "project_id": { "type": "integer" },
+    "web_url": { "type": "string" },
+    "state": { "type": "string" },
+    "create_note_path": { "type": "string" },
+    "preview_note_path": { "type": "string" },
+    "current_user": {
+      "type": "object",
+      "properties": {
+        "can_create_note": { "type": "boolean" },
+        "can_update": { "type": "boolean" }
+      }
+    },
+    "created_at": { "type": "date-time" },
+    "updated_at": { "type": "date-time" },
+    "branch_name": { "type": ["string", "null"] },
+    "due_date": { "type": "date" },
+    "confidential": { "type": "boolean" },
+    "discussion_locked": { "type": ["boolean", "null"] },
+    "updated_by_id": { "type": ["string", "null"] },
+    "deleted_at": { "type": ["string", "null"] },
+    "time_estimate": { "type": "integer" },
+    "total_time_spent": { "type": "integer" },
+    "human_time_estimate": { "type": ["integer", "null"] },
+    "human_total_time_spent": { "type": ["integer", "null"] },
+    "milestone": { "type": ["object", "null"] },
+    "labels": {
+      "type": "array",
+      "items": { "$ref": "label.json" }
+    },
+    "assignees": { "type": ["array", "null"] }
+  },
+  "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/entities/issue_sidebar.json b/spec/fixtures/api/schemas/entities/issue_sidebar.json
new file mode 100644
index 00000000000..682e345d5f5
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/issue_sidebar.json
@@ -0,0 +1,21 @@
+{
+  "type": "object",
+  "properties" : {
+    "id": { "type": "integer" },
+    "iid": { "type": "integer" },
+    "subscribed": { "type": "boolean" },
+    "time_estimate": { "type": "integer" },
+    "total_time_spent": { "type": "integer" },
+    "human_time_estimate": { "type": ["integer", "null"] },
+    "human_total_time_spent": { "type": ["integer", "null"] },
+    "participants": {
+      "type": "array",
+      "items": { "$ref": "../public_api/v4/user/basic.json" }
+    },
+    "assignees": {
+      "type": "array",
+      "items": { "$ref": "../public_api/v4/user/basic.json" }
+    }
+  },
+  "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/entities/label.json b/spec/fixtures/api/schemas/entities/label.json
new file mode 100644
index 00000000000..40dff764c17
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/label.json
@@ -0,0 +1,26 @@
+{
+  "type": "object",
+  "required": [
+    "id",
+    "color",
+    "description",
+    "title",
+    "priority"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "color": {
+      "type": "string",
+      "pattern": "^#[0-9A-Fa-f]{3}{1,2}$"
+    },
+    "description": { "type": ["string", "null"] },
+    "text_color": {
+      "type": "string",
+      "pattern": "^#[0-9A-Fa-f]{3}{1,2}$"
+    },
+    "type": { "type": "string" },
+    "title": { "type": "string" },
+    "priority": { "type": ["integer", "null"] }
+  },
+  "additionalProperties": false
+}
\ No newline at end of file
diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json
index 6b14188582a..995f13381ad 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_basic.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json
@@ -9,7 +9,9 @@
     "human_time_estimate": { "type": ["string", "null"] },
     "human_total_time_spent": { "type": ["string", "null"] },
     "merge_error": { "type": ["string", "null"] },
-    "assignee_id": { "type": ["integer", "null"] }
+    "assignee_id": { "type": ["integer", "null"] },
+    "subscribed": { "type": ["boolean", "null"] },
+    "participants": { "type": "array" }
   },
   "additionalProperties": false
 }
diff --git a/spec/fixtures/api/schemas/issue.json b/spec/fixtures/api/schemas/issue.json
index e1f62508933..a55ecaa5697 100644
--- a/spec/fixtures/api/schemas/issue.json
+++ b/spec/fixtures/api/schemas/issue.json
@@ -19,32 +19,7 @@
     },
     "labels": {
       "type": "array",
-      "items": {
-        "type": "object",
-        "required": [
-          "id",
-          "color",
-          "description",
-          "title",
-          "priority"
-        ],
-        "properties": {
-          "id": { "type": "integer" },
-          "color": {
-            "type": "string",
-            "pattern": "^#[0-9A-Fa-f]{3}{1,2}+$"
-          },
-          "description": { "type": ["string", "null"] },
-          "text_color": {
-            "type": "string",
-            "pattern": "^#[0-9A-Fa-f]{3}{1,2}+$"
-          },
-          "type": { "type": "string" },
-          "title": { "type": "string" },
-          "priority": { "type": ["integer", "null"] }
-        },
-        "additionalProperties": false
-      }
+      "items": { "$ref": "entities/label.json" }
     },
     "assignee": {
       "id": { "type": "integet" },
diff --git a/spec/models/concerns/subscribable_spec.rb b/spec/models/concerns/subscribable_spec.rb
index 28ff8158e0e..45dfb136aea 100644
--- a/spec/models/concerns/subscribable_spec.rb
+++ b/spec/models/concerns/subscribable_spec.rb
@@ -6,6 +6,12 @@ describe Subscribable, 'Subscribable' do
   let(:user_1)   { create(:user) }
 
   describe '#subscribed?' do
+    context 'without user' do
+      it 'returns false' do
+        expect(resource.subscribed?(nil, project)).to be_falsey
+      end
+    end
+
     context 'without project' do
       it 'returns false when no subscription exists' do
         expect(resource.subscribed?(user_1)).to be_falsey
diff --git a/spec/serializers/issue_serializer_spec.rb b/spec/serializers/issue_serializer_spec.rb
new file mode 100644
index 00000000000..75578816e75
--- /dev/null
+++ b/spec/serializers/issue_serializer_spec.rb
@@ -0,0 +1,27 @@
+require 'spec_helper'
+
+describe IssueSerializer do
+  let(:resource) { create(:issue) }
+  let(:user)     { create(:user) }
+  let(:json_entity) do
+    described_class.new(current_user: user)
+      .represent(resource, serializer: serializer)
+      .with_indifferent_access
+  end
+
+  context 'non-sidebar issue serialization' do
+    let(:serializer) { nil }
+
+    it 'matches issue json schema' do
+      expect(json_entity).to match_schema('entities/issue')
+    end
+  end
+
+  context 'sidebar issue serialization' do
+    let(:serializer) { 'sidebar' }
+
+    it 'matches sidebar issue json schema' do
+      expect(json_entity).to match_schema('entities/issue_sidebar')
+    end
+  end
+end
diff --git a/spec/serializers/merge_request_basic_serializer_spec.rb b/spec/serializers/merge_request_basic_serializer_spec.rb
index 4daf5a59d0c..1fad8e6bc5d 100644
--- a/spec/serializers/merge_request_basic_serializer_spec.rb
+++ b/spec/serializers/merge_request_basic_serializer_spec.rb
@@ -4,9 +4,13 @@ describe MergeRequestBasicSerializer do
   let(:resource) { create(:merge_request) }
   let(:user)     { create(:user) }
 
-  subject { described_class.new.represent(resource) }
+  let(:json_entity) do
+    described_class.new(current_user: user)
+      .represent(resource, serializer: 'basic')
+      .with_indifferent_access
+  end
 
-  it 'has important MergeRequest attributes' do
-    expect(subject).to include(:merge_status)
+  it 'matches basic merge request json' do
+    expect(json_entity).to match_schema('entities/merge_request_basic')
   end
 end
diff --git a/spec/serializers/merge_request_serializer_spec.rb b/spec/serializers/merge_request_serializer_spec.rb
index 73fbecc153d..e3abefa6d63 100644
--- a/spec/serializers/merge_request_serializer_spec.rb
+++ b/spec/serializers/merge_request_serializer_spec.rb
@@ -9,11 +9,11 @@ describe MergeRequestSerializer do
   end
 
   describe '#represent' do
-    let(:opts) { { basic: basic } }
-    subject { serializer.represent(merge_request, basic: basic) }
+    let(:opts) { { serializer: serializer_entity } }
+    subject { serializer.represent(merge_request, serializer: serializer_entity) }
 
-    context 'when basic param is truthy' do
-      let(:basic) { true }
+    context 'when passing basic serializer param' do
+      let(:serializer_entity) { 'basic' }
 
       it 'calls super class #represent with correct params' do
         expect_any_instance_of(BaseSerializer).to receive(:represent)
@@ -23,8 +23,8 @@ describe MergeRequestSerializer do
       end
     end
 
-    context 'when basic param is falsy' do
-      let(:basic) { false }
+    context 'when serializer param is falsy' do
+      let(:serializer_entity) { nil }
 
       it 'calls super class #represent with correct params' do
         expect_any_instance_of(BaseSerializer).to receive(:represent)
-- 
cgit v1.2.1