diff options
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 } |