summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@mcgivern.me.uk>2017-12-18 11:28:02 +0000
committerSean McGivern <sean@mcgivern.me.uk>2017-12-18 11:28:02 +0000
commit3870a1bde276144a05a31185ede7a5032818d489 (patch)
tree0e8d69c1859be1d59de6a2edd1abf599d3618948
parentc20720cf84132d7bf14763aba0570bd8882f9c6d (diff)
parentcba40a1f551e4c1b46bfa49a709f59feb59782bd (diff)
downloadgitlab-ce-3870a1bde276144a05a31185ede7a5032818d489.tar.gz
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
-rw-r--r--app/assets/javascripts/vue_merge_request_widget/services/mr_widget_service.js2
-rw-r--r--app/controllers/projects/merge_requests_controller.rb8
-rw-r--r--app/helpers/issuables_helper.rb4
-rw-r--r--app/serializers/issuable_entity.rb8
-rw-r--r--app/serializers/issuable_sidebar_entity.rb6
-rw-r--r--app/serializers/issue_entity.rb8
-rw-r--r--app/serializers/merge_request_serializer.rb6
-rw-r--r--app/serializers/merge_request_widget_entity.rb (renamed from app/serializers/merge_request_entity.rb)5
-rw-r--r--app/views/projects/merge_requests/show.html.haml2
-rw-r--r--changelogs/unreleased/osw-isolate-mr-widget-exposed-attributes.yml5
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb8
-rw-r--r--spec/features/merge_requests/mini_pipeline_graph_spec.rb8
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_widget.json (renamed from spec/fixtures/api/schemas/entities/merge_request.json)8
-rw-r--r--spec/serializers/merge_request_serializer_spec.rb48
-rw-r--r--spec/serializers/merge_request_widget_entity_spec.rb (renamed from spec/serializers/merge_request_entity_spec.rb)41
15 files changed, 68 insertions, 99 deletions
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 99f5c305df5..5fa838baba3 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
@@ -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);
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index e7b3b73024b..6b59c8461a3 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 4c60f4b0cd0..b4ca0e68e0b 100644
--- a/app/helpers/issuables_helper.rb
+++ b/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
diff --git a/app/serializers/issuable_entity.rb b/app/serializers/issuable_entity.rb
index 3b5a4fd4f79..6f31fbd6b7c 100644
--- a/app/serializers/issuable_entity.rb
+++ b/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
diff --git a/app/serializers/issuable_sidebar_entity.rb b/app/serializers/issuable_sidebar_entity.rb
index ff23d8bf0c7..29138c803df 100644
--- a/app/serializers/issuable_sidebar_entity.rb
+++ b/app/serializers/issuable_sidebar_entity.rb
@@ -1,4 +1,5 @@
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
diff --git a/app/serializers/issue_entity.rb b/app/serializers/issue_entity.rb
index 9d52b8d9752..0bdd4d7a272 100644
--- a/app/serializers/issue_entity.rb
+++ b/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
diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb
index e9d98d8baca..52eb30d688a 100644
--- a/app/serializers/merge_request_serializer.rb
+++ b/app/serializers/merge_request_serializer.rb
@@ -1,14 +1,14 @@
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)
diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_widget_entity.rb
index eece9445dca..f8e59b2ffd7 100644
--- a/app/serializers/merge_request_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -1,8 +1,5 @@
-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
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index abff702fd9d..8740c6895df 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/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
diff --git a/changelogs/unreleased/osw-isolate-mr-widget-exposed-attributes.yml b/changelogs/unreleased/osw-isolate-mr-widget-exposed-attributes.yml
new file mode 100644
index 00000000000..6b05713d1a1
--- /dev/null
+++ b/changelogs/unreleased/osw-isolate-mr-widget-exposed-attributes.yml
@@ -0,0 +1,5 @@
+---
+title: Stop sending milestone and labels data over the wire for MR widget requests
+merge_request:
+author:
+type: performance
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 51d5d6a52b3..d03ecefe47f 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/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
diff --git a/spec/features/merge_requests/mini_pipeline_graph_spec.rb b/spec/features/merge_requests/mini_pipeline_graph_spec.rb
index 93c5e945453..a7e7c0eeff6 100644
--- a/spec/features/merge_requests/mini_pipeline_graph_spec.rb
+++ b/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)
diff --git a/spec/fixtures/api/schemas/entities/merge_request.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json
index ba094ba1657..342890c3dee 100644
--- a/spec/fixtures/api/schemas/entities/merge_request.json
+++ b/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" },
diff --git a/spec/serializers/merge_request_serializer_spec.rb b/spec/serializers/merge_request_serializer_spec.rb
index e3abefa6d63..1ad974c774b 100644
--- a/spec/serializers/merge_request_serializer_spec.rb
+++ b/spec/serializers/merge_request_serializer_spec.rb
@@ -1,37 +1,43 @@
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
- end
+ 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 }
+ context 'basic merge request serialization' do
+ let(:serializer) { 'basic' }
+
+ it 'matches basic merge request json schema' do
+ expect(json_entity).to match_schema('entities/merge_request_basic')
+ end
+ end
- it 'calls super class #represent with correct params' do
- expect_any_instance_of(BaseSerializer).to receive(:represent)
- .with(merge_request, opts, MergeRequestEntity)
+ context 'no serializer' do
+ let(:serializer) { nil }
- subject
- end
+ it 'raises an error' do
+ expect { json_entity }.to raise_error(NoMethodError)
end
end
end
diff --git a/spec/serializers/merge_request_entity_spec.rb b/spec/serializers/merge_request_widget_entity_spec.rb
index 1ad672fd355..a5924a8589c 100644
--- a/spec/serializers/merge_request_entity_spec.rb
+++ b/spec/serializers/merge_request_widget_entity_spec.rb
@@ -1,6 +1,6 @@
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