summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMayra Cabrera <mcabrera@gitlab.com>2019-08-28 21:09:23 +0000
committerMayra Cabrera <mcabrera@gitlab.com>2019-08-28 21:09:23 +0000
commitce8f60cbc2c47557373eb0d71c9c88eab5a3b143 (patch)
treec02bb87adac063768ff0b531d42e11b0c9f688de
parent1ca42a247923a8887a3684c08d5c7012a9177997 (diff)
parent8db6c4c6e770be30e3c49b842c501d66e427c4d9 (diff)
downloadgitlab-ce-ce8f60cbc2c47557373eb0d71c9c88eab5a3b143.tar.gz
Merge branch 'id-optimize-sql-requests-mr-show' into 'master'
Reduce the number of SQL requests on MR-show See merge request gitlab-org/gitlab-ce!32192
-rw-r--r--app/controllers/projects/merge_requests_controller.rb1
-rw-r--r--app/serializers/merge_request_noteable_entity.rb53
-rw-r--r--app/serializers/merge_request_poll_widget_entity.rb10
-rw-r--r--app/serializers/merge_request_serializer.rb2
-rw-r--r--app/serializers/merge_request_widget_entity.rb12
-rw-r--r--app/views/projects/merge_requests/show.html.haml13
-rw-r--r--changelogs/unreleased/id-optimize-sql-requests-mr-show.yml5
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_noteable.json28
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_poll_widget.json8
-rw-r--r--spec/fixtures/api/schemas/entities/merge_request_widget.json3
-rw-r--r--spec/serializers/merge_request_serializer_spec.rb8
11 files changed, 107 insertions, 36 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index f4cc0a5851b..d492c5227cf 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -46,6 +46,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@noteable = @merge_request
@commits_count = @merge_request.commits_count
@issuable_sidebar = serializer.represent(@merge_request, serializer: 'sidebar')
+ @current_user_data = UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json
set_pipeline_variables
diff --git a/app/serializers/merge_request_noteable_entity.rb b/app/serializers/merge_request_noteable_entity.rb
new file mode 100644
index 00000000000..e22be6880bb
--- /dev/null
+++ b/app/serializers/merge_request_noteable_entity.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+class MergeRequestNoteableEntity < Grape::Entity
+ include RequestAwareEntity
+
+ # Currently this attr is exposed to be used in app/assets/javascripts/notes/stores/getters.js
+ # in order to determine whether a noteable is an issue or an MR
+ expose :merge_params
+
+ expose :state
+ expose :source_branch
+ expose :target_branch
+ expose :diff_head_sha
+
+ expose :create_note_path do |merge_request|
+ project_notes_path(merge_request.project, target_type: 'merge_request', target_id: merge_request.id)
+ end
+
+ expose :preview_note_path do |merge_request|
+ preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid)
+ end
+
+ expose :supports_suggestion?, as: :can_receive_suggestion
+
+ expose :create_issue_to_resolve_discussions_path do |merge_request|
+ presenter(merge_request).create_issue_to_resolve_discussions_path
+ end
+
+ expose :new_blob_path do |merge_request|
+ if presenter(merge_request).can_push_to_source_branch?
+ project_new_blob_path(merge_request.source_project, merge_request.source_branch)
+ end
+ end
+
+ expose :current_user do
+ expose :can_create_note do |merge_request|
+ can?(current_user, :create_note, merge_request)
+ end
+
+ expose :can_update do |merge_request|
+ can?(current_user, :update_merge_request, merge_request)
+ end
+ end
+
+ private
+
+ delegate :current_user, to: :request
+
+ def presenter(merge_request)
+ @presenters ||= {}
+ @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user) # rubocop: disable CodeReuse/Presenter
+ end
+end
diff --git a/app/serializers/merge_request_poll_widget_entity.rb b/app/serializers/merge_request_poll_widget_entity.rb
index 65132b4b215..cd33ffa702a 100644
--- a/app/serializers/merge_request_poll_widget_entity.rb
+++ b/app/serializers/merge_request_poll_widget_entity.rb
@@ -65,8 +65,6 @@ class MergeRequestPollWidgetEntity < IssuableEntity
end
end
- expose :supports_suggestion?, as: :can_receive_suggestion
-
expose :create_issue_to_resolve_discussions_path do |merge_request|
presenter(merge_request).create_issue_to_resolve_discussions_path
end
@@ -84,17 +82,9 @@ class MergeRequestPollWidgetEntity < IssuableEntity
presenter(merge_request).can_cherry_pick_on_current_merge_request?
end
- expose :can_create_note do |merge_request|
- can?(current_user, :create_note, merge_request)
- end
-
expose :can_create_issue do |merge_request|
can?(current_user, :create_issue, merge_request.project)
end
-
- expose :can_update do |merge_request|
- can?(current_user, :update_merge_request, merge_request)
- end
end
expose :can_push_to_source_branch do |merge_request|
diff --git a/app/serializers/merge_request_serializer.rb b/app/serializers/merge_request_serializer.rb
index bd2e682a122..aa67cd1f39e 100644
--- a/app/serializers/merge_request_serializer.rb
+++ b/app/serializers/merge_request_serializer.rb
@@ -13,6 +13,8 @@ class MergeRequestSerializer < BaseSerializer
MergeRequestSidebarExtrasEntity
when 'basic'
MergeRequestBasicEntity
+ when 'noteable'
+ MergeRequestNoteableEntity
else
# fallback to widget for old poll requests without `serializer` set
MergeRequestWidgetEntity
diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb
index c8088608cb0..2f2c42a7387 100644
--- a/app/serializers/merge_request_widget_entity.rb
+++ b/app/serializers/merge_request_widget_entity.rb
@@ -3,10 +3,6 @@
class MergeRequestWidgetEntity < Grape::Entity
include RequestAwareEntity
- # Currently this attr is exposed to be used in app/assets/javascripts/notes/stores/getters.js
- # in order to determine whether a noteable is an issue or an MR
- expose :merge_params
-
expose :source_project_full_path do |merge_request|
merge_request.source_project&.full_path
end
@@ -35,18 +31,10 @@ class MergeRequestWidgetEntity < Grape::Entity
cached_widget_project_json_merge_request_path(merge_request.target_project, merge_request, format: :json)
end
- expose :create_note_path do |merge_request|
- project_notes_path(merge_request.project, target_type: 'merge_request', target_id: merge_request.id)
- end
-
expose :commit_change_content_path do |merge_request|
commit_change_content_project_merge_request_path(merge_request.project, merge_request)
end
- expose :preview_note_path do |merge_request|
- preview_markdown_path(merge_request.project, target_type: 'MergeRequest', target_id: merge_request.iid)
- end
-
expose :conflicts_docs_path do |merge_request|
help_page_path('user/project/merge_requests/resolve_conflicts.md')
end
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index af3bd8dcd69..ea166d622eb 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/app/views/projects/merge_requests/show.html.haml
@@ -6,6 +6,7 @@
- page_description @merge_request.description
- page_card_attributes @merge_request.card_attributes
- suggest_changes_help_path = help_page_path('user/discussions/index.md', anchor: 'suggest-changes')
+- number_of_pipelines = @pipelines.size
.merge-request{ data: { mr_action: j(params[:tab].presence || 'show'), url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project), lock_version: @merge_request.lock_version } }
= render "projects/merge_requests/mr_title"
@@ -41,11 +42,11 @@
= tab_link_for @merge_request, :commits do
= _("Commits")
%span.badge.badge-pill= @commits_count
- - if @pipelines.any?
+ - if number_of_pipelines.nonzero?
%li.pipelines-tab
= tab_link_for @merge_request, :pipelines do
= _("Pipelines")
- %span.badge.badge-pill.js-pipelines-mr-count= @pipelines.size
+ %span.badge.badge-pill.js-pipelines-mr-count= number_of_pipelines
%li.diffs-tab.qa-diffs-tab
= tab_link_for @merge_request, :diffs do
= _("Changes")
@@ -63,21 +64,21 @@
%script.js-notes-data{ type: "application/json" }= initial_notes_data(true).to_json.html_safe
.issuable-discussion.js-vue-notes-event
#js-vue-mr-discussions{ data: { notes_data: notes_data(@merge_request).to_json,
- noteable_data: serialize_issuable(@merge_request),
+ noteable_data: serialize_issuable(@merge_request, serializer: 'noteable'),
noteable_type: 'MergeRequest',
target_type: 'merge_request',
help_page_path: suggest_changes_help_path,
- current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json} }
+ current_user_data: @current_user_data} }
#commits.commits.tab-pane
-# This tab is always loaded via AJAX
#pipelines.pipelines.tab-pane
- - if @pipelines.any?
+ - if number_of_pipelines.nonzero?
= render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request)
#js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?,
endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
help_page_path: suggest_changes_help_path,
- current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json,
+ current_user_data: @current_user_data,
project_path: project_path(@merge_request.project),
changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg'),
is_fluid_layout: fluid_layout.to_s,
diff --git a/changelogs/unreleased/id-optimize-sql-requests-mr-show.yml b/changelogs/unreleased/id-optimize-sql-requests-mr-show.yml
new file mode 100644
index 00000000000..8b171a96316
--- /dev/null
+++ b/changelogs/unreleased/id-optimize-sql-requests-mr-show.yml
@@ -0,0 +1,5 @@
+---
+title: Reduce the number of SQL requests on MR-show
+merge_request: 32192
+author:
+type: performance
diff --git a/spec/fixtures/api/schemas/entities/merge_request_noteable.json b/spec/fixtures/api/schemas/entities/merge_request_noteable.json
new file mode 100644
index 00000000000..88b0fecc24c
--- /dev/null
+++ b/spec/fixtures/api/schemas/entities/merge_request_noteable.json
@@ -0,0 +1,28 @@
+{
+ "type": "object",
+ "properties" : {
+ "merge_params": { "type": ["object", "null"] },
+ "state": { "type": "string" },
+ "source_branch": { "type": "string" },
+ "target_branch": { "type": "string" },
+ "diff_head_sha": { "type": "string" },
+ "create_note_path": { "type": ["string", "null"] },
+ "preview_note_path": { "type": ["string", "null"] },
+ "create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
+ "new_blob_path": { "type": ["string", "null"] },
+ "can_receive_suggestion": { "type": "boolean" },
+ "current_user": {
+ "type": "object",
+ "required": [
+ "can_create_note",
+ "can_update"
+ ],
+ "properties": {
+ "can_create_note": { "type": "boolean" },
+ "can_update": { "type": "boolean" }
+ },
+ "additionalProperties": false
+ }
+ },
+ "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/entities/merge_request_poll_widget.json b/spec/fixtures/api/schemas/entities/merge_request_poll_widget.json
index 2052892dfa3..1eda0e12920 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_poll_widget.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_poll_widget.json
@@ -24,22 +24,20 @@
"ci_status": { "type": ["string", "null"] },
"cancel_auto_merge_path": { "type": ["string", "null"] },
"test_reports_path": { "type": ["string", "null"] },
- "can_receive_suggestion": { "type": "boolean" },
"create_issue_to_resolve_discussions_path": { "type": ["string", "null"] },
"current_user": {
"type": "object",
"required": [
"can_remove_source_branch",
"can_revert_on_current_merge_request",
- "can_cherry_pick_on_current_merge_request"
+ "can_cherry_pick_on_current_merge_request",
+ "can_create_issue"
],
"properties": {
"can_remove_source_branch": { "type": "boolean" },
"can_revert_on_current_merge_request": { "type": ["boolean", "null"] },
"can_cherry_pick_on_current_merge_request": { "type": ["boolean", "null"] },
- "can_create_note": { "type": "boolean" },
- "can_create_issue": { "type": "boolean" },
- "can_update": { "type": "boolean" }
+ "can_create_issue": { "type": "boolean" }
},
"additionalProperties": false
},
diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json
index 779a47222b7..e2df7952d8f 100644
--- a/spec/fixtures/api/schemas/entities/merge_request_widget.json
+++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json
@@ -5,7 +5,6 @@
{ "$ref": "merge_request_poll_widget.json" },
{
"properties" : {
- "merge_params": { "type": ["object", "null"] },
"source_project_full_path": { "type": ["string", "null"]},
"target_project_full_path": { "type": ["string", "null"]},
"email_patches_path": { "type": "string" },
@@ -13,9 +12,7 @@
"merge_request_basic_path": { "type": "string" },
"merge_request_widget_path": { "type": "string" },
"merge_request_cached_widget_path": { "type": "string" },
- "create_note_path": { "type": ["string", "null"] },
"commit_change_content_path": { "type": "string" },
- "preview_note_path": { "type": ["string", "null"] },
"conflicts_docs_path": { "type": ["string", "null"] },
"merge_request_pipelines_docs_path": { "type": ["string", "null"] },
"ci_environments_status_path": { "type": "string" },
diff --git a/spec/serializers/merge_request_serializer_spec.rb b/spec/serializers/merge_request_serializer_spec.rb
index 276e0f6ff3d..d1483c3c41e 100644
--- a/spec/serializers/merge_request_serializer_spec.rb
+++ b/spec/serializers/merge_request_serializer_spec.rb
@@ -41,6 +41,14 @@ describe MergeRequestSerializer do
end
end
+ context 'noteable merge request serialization' do
+ let(:serializer) { 'noteable' }
+
+ it 'matches noteable merge request json schema' do
+ expect(json_entity).to match_schema('entities/merge_request_noteable', strict: true)
+ end
+ end
+
context 'no serializer' do
let(:serializer) { nil }