Commit 533b95f9 authored by Phil Hughes's avatar Phil Hughes

Merge branch '254627-include-diff-note-details-in-notes-json' into 'master'

Don't refresh all discussions for a new diff note on a merge request

See merge request gitlab-org/gitlab!43015
parents 9a054c85 3f2ab0cb
...@@ -167,7 +167,7 @@ export const updateOrCreateNotes = ({ commit, state, getters, dispatch }, notes) ...@@ -167,7 +167,7 @@ export const updateOrCreateNotes = ({ commit, state, getters, dispatch }, notes)
if (discussion) { if (discussion) {
commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note); commit(types.ADD_NEW_REPLY_TO_DISCUSSION, note);
} else if (note.type === constants.DIFF_NOTE) { } else if (note.type === constants.DIFF_NOTE && !note.base_discussion) {
debouncedFetchDiscussions(state.currentlyFetchingDiscussions); debouncedFetchDiscussions(state.currentlyFetchingDiscussions);
} else { } else {
commit(types.ADD_NEW_NOTE, note); commit(types.ADD_NEW_NOTE, note);
......
...@@ -11,24 +11,30 @@ export default { ...@@ -11,24 +11,30 @@ export default {
const isDiscussion = type === constants.DISCUSSION_NOTE || type === constants.DIFF_NOTE; const isDiscussion = type === constants.DISCUSSION_NOTE || type === constants.DIFF_NOTE;
if (!exists) { if (!exists) {
const noteData = { let discussion = data.discussion || note.base_discussion;
if (!discussion) {
discussion = {
expanded: true, expanded: true,
id: discussion_id, id: discussion_id,
individual_note: !isDiscussion, individual_note: !isDiscussion,
notes: [note],
reply_id: discussion_id, reply_id: discussion_id,
}; };
if (isDiscussion && isInMRPage()) { if (isDiscussion && isInMRPage()) {
noteData.resolvable = note.resolvable; discussion.resolvable = note.resolvable;
noteData.resolved = false; discussion.resolved = false;
noteData.active = true; discussion.active = true;
noteData.resolve_path = note.resolve_path; discussion.resolve_path = note.resolve_path;
noteData.resolve_with_issue_path = note.resolve_with_issue_path; discussion.resolve_with_issue_path = note.resolve_with_issue_path;
noteData.diff_discussion = false; discussion.diff_discussion = false;
}
} }
state.discussions.push(noteData); note.base_discussion = undefined; // No point keeping a reference to this
discussion.notes = [note];
state.discussions.push(discussion);
} }
}, },
......
...@@ -60,7 +60,7 @@ class Projects::NotesController < Projects::ApplicationController ...@@ -60,7 +60,7 @@ class Projects::NotesController < Projects::ApplicationController
def render_json_with_notes_serializer def render_json_with_notes_serializer
prepare_notes_for_rendering([note]) prepare_notes_for_rendering([note])
render json: note_serializer.represent(note) render json: note_serializer.represent(note, render_truncated_diff_lines: true)
end end
def note def note
......
# frozen_string_literal: true
class BaseDiscussionEntity < Grape::Entity
include RequestAwareEntity
include NotesHelper
expose :id
expose :reply_id
expose :project_id
expose :commit_id
expose :confidential?, as: :confidential
expose :diff_discussion?, as: :diff_discussion
expose :expanded?, as: :expanded
expose :for_commit?, as: :for_commit
expose :individual_note?, as: :individual_note
expose :resolvable?, as: :resolvable
expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) }
with_options if: -> (d, _) { d.diff_discussion? } do
expose :active?, as: :active
expose :line_code
expose :diff_file, using: DiscussionDiffFileEntity
end
with_options if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } do
expose :position
expose :original_position
end
expose :discussion_path do |discussion|
discussion_path(discussion)
end
with_options if: -> (d, _) { d.resolvable? } do
expose :resolve_path do |discussion|
resolve_project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion.id)
end
expose :resolve_with_issue_path do |discussion|
new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id)
end
end
expose :truncated_diff_lines_path, if: -> (d, _) { !d.expanded? && !render_truncated_diff_lines? } do |discussion|
project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion)
end
private
def render_truncated_diff_lines?
options.fetch(:render_truncated_diff_lines, false)
end
end
# frozen_string_literal: true # frozen_string_literal: true
class DiscussionEntity < Grape::Entity class DiscussionEntity < BaseDiscussionEntity
include RequestAwareEntity
include NotesHelper
expose :id, :reply_id
expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
expose :line_code, if: -> (d, _) { d.diff_discussion? }
expose :expanded?, as: :expanded
expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? }
expose :project_id
expose :notes do |discussion, opts| expose :notes do |discussion, opts|
request.note_entity.represent(discussion.notes, opts) request.note_entity.represent(discussion.notes, opts.merge(with_base_discussion: false))
end
expose :discussion_path do |discussion|
discussion_path(discussion)
end end
expose :positions, if: -> (d, _) { display_merge_ref_discussions?(d) } do |discussion| expose :positions, if: -> (d, _) { display_merge_ref_discussions?(d) } do |discussion|
...@@ -28,42 +13,13 @@ class DiscussionEntity < Grape::Entity ...@@ -28,42 +13,13 @@ class DiscussionEntity < Grape::Entity
discussion.diff_note_positions.map(&:line_code) discussion.diff_note_positions.map(&:line_code)
end end
expose :individual_note?, as: :individual_note
expose :resolvable do |discussion|
discussion.resolvable?
end
expose :resolved?, as: :resolved expose :resolved?, as: :resolved
expose :resolved_by_push?, as: :resolved_by_push expose :resolved_by_push?, as: :resolved_by_push
expose :resolved_by, using: NoteUserEntity expose :resolved_by, using: NoteUserEntity
expose :resolved_at expose :resolved_at
expose :resolve_path, if: -> (d, _) { d.resolvable? } do |discussion|
resolve_project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion.id)
end
expose :resolve_with_issue_path, if: -> (d, _) { d.resolvable? } do |discussion|
new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id)
end
expose :diff_file, using: DiscussionDiffFileEntity, if: -> (d, _) { d.diff_discussion? }
expose :diff_discussion?, as: :diff_discussion
expose :truncated_diff_lines_path, if: -> (d, _) { !d.expanded? && !render_truncated_diff_lines? } do |discussion|
project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion)
end
expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) }
expose :for_commit?, as: :for_commit
expose :commit_id
expose :confidential?, as: :confidential
private private
def render_truncated_diff_lines?
options[:render_truncated_diff_lines]
end
def current_user def current_user
request.current_user request.current_user
end end
......
...@@ -81,11 +81,25 @@ class NoteEntity < API::Entities::Note ...@@ -81,11 +81,25 @@ class NoteEntity < API::Entities::Note
expose :cached_markdown_version expose :cached_markdown_version
# Correctly rendering a note requires some background information about any
# discussion it is part of. This is essential for the notes endpoint, but
# optional for the discussions endpoint, which will include the discussion
# along with the note
expose :discussion, as: :base_discussion, using: BaseDiscussionEntity, if: -> (_, _) { with_base_discussion? }
private private
def discussion
@discussion ||= object.to_discussion(request.noteable)
end
def current_user def current_user
request.current_user request.current_user
end end
def with_base_discussion?
options.fetch(:with_base_discussion, true)
end
end end
NoteEntity.prepend_if_ee('EE::NoteEntity') NoteEntity.prepend_if_ee('EE::NoteEntity')
---
title: Don't refresh all discussions for a new diff note on a merge request
merge_request: 43015
author:
type: performance
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe BaseDiscussionEntity do
let_it_be(:user) { create(:user) }
let_it_be(:note) { create(:discussion_note_on_merge_request) }
let(:request) { double('request', note_entity: ProjectNoteEntity) }
let(:controller) { double('controller') }
let(:entity) { described_class.new(discussion, request: request, context: controller) }
let(:discussion) { note.discussion }
subject { entity.as_json }
before do
allow(controller).to receive(:render_to_string)
allow(request).to receive(:current_user).and_return(user)
allow(request).to receive(:noteable).and_return(note.noteable)
end
it 'exposes correct attributes' do
expect(subject.keys.sort).to include(
:commit_id,
:confidential,
:diff_discussion,
:discussion_path,
:expanded,
:for_commit,
:id,
:individual_note,
:resolvable,
:resolve_path,
:resolve_with_issue_path
)
end
context 'when is LegacyDiffDiscussion' do
let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:discussion) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
it 'exposes correct attributes' do
expect(subject.keys.sort).to include(
:commit_id,
:diff_discussion,
:discussion_path,
:expanded,
:for_commit,
:id,
:individual_note
)
end
end
context 'when diff file is present' do
let(:note) { create(:diff_note_on_merge_request) }
it 'exposes diff file attributes' do
expect(subject.keys.sort).to include(
:active,
:diff_file,
:line_code,
:position,
:truncated_diff_lines
)
end
end
end
...@@ -39,6 +39,10 @@ RSpec.describe DiscussionEntity do ...@@ -39,6 +39,10 @@ RSpec.describe DiscussionEntity do
) )
end end
it 'does not include base discussion in the notes' do
expect(subject[:notes].first.keys).not_to include(:base_discussion)
end
it 'resolved_by matches note_user_entity schema' do it 'resolved_by matches note_user_entity schema' do
Notes::ResolveService.new(note.project, user).execute(note) Notes::ResolveService.new(note.project, user).execute(note)
......
...@@ -5,8 +5,21 @@ RSpec.shared_examples 'note entity' do ...@@ -5,8 +5,21 @@ RSpec.shared_examples 'note entity' do
context 'basic note' do context 'basic note' do
it 'exposes correct elements' do it 'exposes correct elements' do
expect(subject).to include(:type, :author, :note, :note_html, :current_user, :discussion_id, expect(subject).to include(
:emoji_awardable, :award_emoji, :report_abuse_path, :attachment, :noteable_note_url, :resolvable) :attachment,
:author,
:award_emoji,
:base_discussion,
:current_user,
:discussion_id,
:emoji_awardable,
:note,
:note_html,
:noteable_note_url,
:report_abuse_path,
:resolvable,
:type
)
end end
it 'does not expose elements for specific notes cases' do it 'does not expose elements for specific notes cases' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment