Merge branch 'osw-isolate-mr-widget-exposed-attributes' into 'master'

Stop sending milestone and labels for Projects::MergeRequestsController#show.json requests

See merge request gitlab-org/gitlab-ce!15847

app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js

@@ -6,7 +6,7 @@ Vue.use(VueResource);
 export default class MRWidgetService {
   constructor(endpoints) {
     this.mergeResource = Vue.resource(endpoints.mergePath);
-    this.mergeCheckResource = Vue.resource(endpoints.statusPath);
+    this.mergeCheckResource = Vue.resource(`${endpoints.statusPath}?serializer=widget`);
     this.cancelAutoMergeResource = Vue.resource(endpoints.cancelAutoMergePath);
     this.removeWIPResource = Vue.resource(endpoints.removeWIPPath);
     this.removeSourceBranchResource = Vue.resource(endpoints.sourceBranchPath);

app/controllers/projects/merge_requests_controller.rb

@@ -131,7 +131,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       .new(project, current_user, wip_event: 'unwip')
       .execute(@merge_request)
 
-    render json: serializer.represent(@merge_request)
+    render json: serialize_widget(@merge_request)
   end
 
   def commit_change_content
@@ -147,7 +147,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       .new(@project, current_user)
       .cancel(@merge_request)
 
-    render json: serializer.represent(@merge_request)
+    render json: serialize_widget(@merge_request)
   end
 
   def merge
@@ -304,6 +304,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     end
   end
 
+  def serialize_widget(merge_request)
+    serializer.represent(merge_request, serializer: 'widget')
+  end
+
   def serializer
     MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
   end

app/helpers/issuables_helper.rb

@@ -32,7 +32,7 @@ module IssuablesHelper
     end
   end
 
-  def serialize_issuable(issuable)
+  def serialize_issuable(issuable, serializer: nil)
     serializer_klass = case issuable
                        when Issue
                          IssueSerializer
@@ -42,7 +42,7 @@ module IssuablesHelper
 
     serializer_klass
       .new(current_user: current_user, project: issuable.project)
-      .represent(issuable)
+      .represent(issuable, serializer: serializer)
       .to_json
   end
 

app/serializers/issuable_entity.rb

@@ -3,14 +3,6 @@ class IssuableEntity < Grape::Entity
 
   expose :id
   expose :iid
-  expose :author_id
   expose :description
-  expose :lock_version
-  expose :milestone_id
   expose :title
-  expose :updated_by_id
-  expose :created_at
-  expose :updated_at
-  expose :milestone, using: API::Entities::Milestone
-  expose :labels, using: LabelEntity
 end

app/serializers/issuable_sidebar_entity.rb

 class IssuableSidebarEntity < Grape::Entity
+  include TimeTrackableEntity
   include RequestAwareEntity
 
   expose :participants, using: ::API::Entities::UserBasic do |issuable|
@@ -8,9 +9,4 @@ class IssuableSidebarEntity < Grape::Entity
   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

app/serializers/issue_entity.rb

@@ -2,7 +2,15 @@ class IssueEntity < IssuableEntity
   include TimeTrackableEntity
 
   expose :state
+  expose :milestone_id
+  expose :updated_by_id
+  expose :created_at
+  expose :updated_at
   expose :deleted_at
+  expose :milestone, using: API::Entities::Milestone
+  expose :labels, using: LabelEntity
+  expose :lock_version
+  expose :author_id
   expose :confidential
   expose :discussion_locked
   expose :assignees, using: API::Entities::UserBasic

app/serializers/merge_request_serializer.rb

 class MergeRequestSerializer < BaseSerializer
   # This overrided method takes care of which entity should be used
-  # to serialize the `merge_request` based on `basic` key in `opts` param.
+  # to serialize the `merge_request` based on `serializer` 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 'basic', 'sidebar'
         MergeRequestBasicEntity
-      else
-        MergeRequestEntity
+      when 'widget'
+        MergeRequestWidgetEntity
       end
 
     super(merge_request, opts, entity)

app/serializers/merge_request_entity.rb → app/serializers/merge_request_widget_entity.rb

-class MergeRequestEntity < IssuableEntity
-  include TimeTrackableEntity
-
+class MergeRequestWidgetEntity < IssuableEntity
   expose :state
-  expose :deleted_at
   expose :in_progress_merge_commit_sha
   expose :merge_commit_sha
   expose :merge_error

app/views/projects/merge_requests/show.html.haml

@@ -20,7 +20,7 @@
     -# haml-lint:disable InlineJavaScript
     :javascript
       window.gl = window.gl || {};
-      window.gl.mrWidgetData = #{serialize_issuable(@merge_request)}
+      window.gl.mrWidgetData = #{serialize_issuable(@merge_request, serializer: 'widget')}
 
     #js-vue-mr-widget.mr-widget
 

changelogs/unreleased/osw-isolate-mr-widget-exposed-attributes.yml

+---
+title: Stop sending milestone and labels data over the wire for MR widget requests
+merge_request:
+author:
+type: performance

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -91,11 +91,11 @@ describe Projects::MergeRequestsController do
         end
       end
 
-      context 'without basic serializer param' do
-        it 'renders the merge request in the json format' do
-          go(format: :json)
+      context 'with widget serializer param' do
+        it 'renders widget MR entity as json' do
+          go(serializer: 'widget', format: :json)
 
-          expect(response).to match_response_schema('entities/merge_request')
+          expect(response).to match_response_schema('entities/merge_request_widget')
         end
       end
     end

spec/features/merge_requests/mini_pipeline_graph_spec.rb

@@ -15,8 +15,8 @@ feature 'Mini Pipeline Graph', :js do
     visit_merge_request
   end
 
-  def visit_merge_request(format = :html)
-    visit project_merge_request_path(project, merge_request, format: format)
+  def visit_merge_request(format: :html, serializer: nil)
+    visit project_merge_request_path(project, merge_request, format: format, serializer: serializer)
   end
 
   it 'should display a mini pipeline graph' do
@@ -33,12 +33,12 @@ feature 'Mini Pipeline Graph', :js do
     end
 
     it 'avoids repeated database queries' do
-      before = ActiveRecord::QueryRecorder.new { visit_merge_request(:json) }
+      before = ActiveRecord::QueryRecorder.new { visit_merge_request(format: :json, serializer: 'widget') }
 
       create(:ci_build, pipeline: pipeline, legacy_artifacts_file: artifacts_file2)
       create(:ci_build, pipeline: pipeline, when: 'manual')
 
-      after = ActiveRecord::QueryRecorder.new { visit_merge_request(:json) }
+      after = ActiveRecord::QueryRecorder.new { visit_merge_request(format: :json, serializer: 'widget') }
 
       expect(before.count).to eq(after.count)
       expect(before.cached_count).to eq(after.cached_count)

spec/fixtures/api/schemas/entities/merge_request.json → spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -80,15 +80,15 @@
     "target_branch_tree_path": { "type": "string" },
     "source_branch_path": { "type": "string" },
     "conflict_resolution_path": { "type": ["string", "null"] },
-    "cancel_merge_when_pipeline_succeeds_path": { "type": "string" },
-    "create_issue_to_resolve_discussions_path": { "type": "string" },
-    "merge_path": { "type": "string" },
+    "cancel_merge_when_pipeline_succeeds_path": { "type": ["string", "null"] },
+    "create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
+    "merge_path": { "type": ["string", "null"] },
     "cherry_pick_in_fork_path": { "type": ["string", "null"] },
     "revert_in_fork_path": { "type": ["string", "null"] },
     "email_patches_path": { "type": "string" },
     "plain_diff_path": { "type": "string" },
     "status_path": { "type": "string" },
-    "new_blob_path": { "type": "string" },
+    "new_blob_path": { "type": ["string", "null"] },
     "merge_check_path": { "type": "string" },
     "ci_environments_status_path": { "type": "string" },
     "merge_commit_message_with_description": { "type": "string" },

spec/serializers/merge_request_serializer_spec.rb

 require 'spec_helper'
 
 describe MergeRequestSerializer do
-  let(:user) { build_stubbed(:user) }
-  let(:merge_request) { build_stubbed(:merge_request) }
-
-  let(:serializer) do
+  let(:user) { create(:user) }
+  let(:resource) { create(:merge_request) }
+  let(:json_entity) do
     described_class.new(current_user: user)
+      .represent(resource, serializer: serializer)
+      .with_indifferent_access
   end
 
-  describe '#represent' do
-    let(:opts) { { serializer: serializer_entity } }
-    subject { serializer.represent(merge_request, serializer: serializer_entity) }
+  context 'widget merge request serialization' do
+    let(:serializer) { 'widget' }
 
-    context 'when passing basic serializer param' do
-      let(:serializer_entity) { 'basic' }
+    it 'matches issue json schema' do
+      expect(json_entity).to match_schema('entities/merge_request_widget')
+    end
+  end
 
-      it 'calls super class #represent with correct params' do
-        expect_any_instance_of(BaseSerializer).to receive(:represent)
-          .with(merge_request, opts, MergeRequestBasicEntity)
+  context 'sidebar merge request serialization' do
+    let(:serializer) { 'sidebar' }
 
-        subject
+    it 'matches basic merge request json schema' do
+      expect(json_entity).to match_schema('entities/merge_request_basic')
     end
   end
 
-    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)
-          .with(merge_request, opts, MergeRequestEntity)
+  context 'basic merge request serialization' do
+    let(:serializer) { 'basic' }
 
-        subject
+    it 'matches basic merge request json schema' do
+      expect(json_entity).to match_schema('entities/merge_request_basic')
     end
   end
+
+  context 'no serializer' do
+    let(:serializer) { nil }
+
+    it 'raises an error' do
+      expect { json_entity }.to raise_error(NoMethodError)
+    end
   end
 end

spec/serializers/merge_request_entity_spec.rb → spec/serializers/merge_request_widget_entity_spec.rb

 require 'spec_helper'
 
-describe MergeRequestEntity do
+describe MergeRequestWidgetEntity do
   let(:project)  { create :project, :repository }
   let(:resource) { create(:merge_request, source_project: project, target_project: project) }
   let(:user)     { create(:user) }
@@ -35,33 +35,6 @@ describe MergeRequestEntity do
     end
   end
 
-  it 'includes issues_links' do
-    issues_links = subject[:issues_links]
-
-    expect(issues_links).to include(:closing, :mentioned_but_not_closing,
-                                    :assign_to_closing)
-  end
-
-  it 'has Issuable attributes' do
-    expect(subject).to include(:id, :iid, :author_id, :description, :lock_version, :milestone_id,
-                               :title, :updated_by_id, :created_at, :updated_at, :milestone, :labels)
-  end
-
-  it 'has time estimation attributes' do
-    expect(subject).to include(:time_estimate, :total_time_spent, :human_time_estimate, :human_total_time_spent)
-  end
-
-  it 'has important MergeRequest attributes' do
-    expect(subject).to include(:state, :deleted_at, :diff_head_sha, :merge_commit_message,
-                               :has_conflicts, :has_ci, :merge_path,
-                               :conflict_resolution_path,
-                               :cancel_merge_when_pipeline_succeeds_path,
-                               :create_issue_to_resolve_discussions_path,
-                               :source_branch_path, :target_branch_commits_path,
-                               :target_branch_tree_path, :commits_count, :merge_ongoing,
-                               :ff_only_enabled)
-  end
-
   it 'has email_patches_path' do
     expect(subject[:email_patches_path])
       .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.patch")
@@ -116,18 +89,6 @@ describe MergeRequestEntity do
     end
   end
 
-  it 'includes merge_event' do
-    create(:event, :merged, author: user, project: resource.project, target: resource)
-
-    expect(subject[:merge_event]).to include(:author, :updated_at)
-  end
-
-  it 'includes closed_event' do
-    create(:event, :closed, author: user, project: resource.project, target: resource)
-
-    expect(subject[:closed_event]).to include(:author, :updated_at)
-  end
-
   describe 'diverged_commits_count' do
     context 'when MR open and its diverging' do
       it 'returns diverged commits count' do