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

The `notes.json` endpoint doesn't return sufficient information to
build a functional discussion for a diff note, so we don't bother
at the moment, instead pulling anew from `discussions.json`. However,
this is quite expensive, and we're hoping to replace `discussions.json`
with `notes.json` entirely in the future, so we're going to need it to
support these attributes.

Including information about the diff makes `notes.json` a little more
expensive, but it's a paginatable endpoint with incremental updates,
whereas `discussions.json` is not paginatable and always returns all
notes for a given noteable, so this seems like an uncontroversial win.

app/assets/javascripts/notes/stores/actions.js

@@ -167,7 +167,7 @@ export const updateOrCreateNotes = ({ commit, state, getters, dispatch }, notes)
 
       if (discussion) {
         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);
       } else {
         commit(types.ADD_NEW_NOTE, note);

app/assets/javascripts/notes/stores/mutations.js

@@ -11,24 +11,30 @@ export default {
     const isDiscussion = type === constants.DISCUSSION_NOTE || type === constants.DIFF_NOTE;
 
     if (!exists) {
-      const noteData = {
+      let discussion = data.discussion || note.base_discussion;
+
+      if (!discussion) {
+        discussion = {
           expanded: true,
           id: discussion_id,
           individual_note: !isDiscussion,
-        notes: [note],
           reply_id: discussion_id,
         };
 
         if (isDiscussion && isInMRPage()) {
-        noteData.resolvable = note.resolvable;
-        noteData.resolved = false;
-        noteData.active = true;
-        noteData.resolve_path = note.resolve_path;
-        noteData.resolve_with_issue_path = note.resolve_with_issue_path;
-        noteData.diff_discussion = false;
+          discussion.resolvable = note.resolvable;
+          discussion.resolved = false;
+          discussion.active = true;
+          discussion.resolve_path = note.resolve_path;
+          discussion.resolve_with_issue_path = note.resolve_with_issue_path;
+          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);
     }
   },
 

app/controllers/projects/notes_controller.rb

@@ -60,7 +60,7 @@ class Projects::NotesController < Projects::ApplicationController
   def render_json_with_notes_serializer
     prepare_notes_for_rendering([note])
 
-    render json: note_serializer.represent(note)
+    render json: note_serializer.represent(note, render_truncated_diff_lines: true)
   end
 
   def note

app/serializers/base_discussion_entity.rb

+# 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

app/serializers/discussion_entity.rb

 # frozen_string_literal: true
 
-class DiscussionEntity < Grape::Entity
-  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
-
+class DiscussionEntity < BaseDiscussionEntity
   expose :notes do |discussion, opts|
-    request.note_entity.represent(discussion.notes, opts)
-  end
-
-  expose :discussion_path do |discussion|
-    discussion_path(discussion)
+    request.note_entity.represent(discussion.notes, opts.merge(with_base_discussion: false))
   end
 
   expose :positions, if: -> (d, _) { display_merge_ref_discussions?(d) } do |discussion|
@@ -28,42 +13,13 @@ class DiscussionEntity < Grape::Entity
     discussion.diff_note_positions.map(&:line_code)
   end
 
-  expose :individual_note?, as: :individual_note
-  expose :resolvable do |discussion|
-    discussion.resolvable?
-  end
-
   expose :resolved?, as: :resolved
   expose :resolved_by_push?, as: :resolved_by_push
   expose :resolved_by, using: NoteUserEntity
   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
 
-  def render_truncated_diff_lines?
-    options[:render_truncated_diff_lines]
-  end
-
   def current_user
     request.current_user
   end

app/serializers/note_entity.rb

@@ -77,11 +77,25 @@ class NoteEntity < API::Entities::Note
 
   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
 
+  def discussion
+    @discussion ||= object.to_discussion(request.noteable)
+  end
+
   def current_user
     request.current_user
   end
+
+  def with_base_discussion?
+    options.fetch(:with_base_discussion, true)
+  end
 end
 
 NoteEntity.prepend_if_ee('EE::NoteEntity')

changelogs/unreleased/254627-include-diff-note-details-in-notes-json.yml

+---
+title: Don't refresh all discussions for a new diff note on a merge request
+merge_request: 43015
+author:
+type: performance

spec/serializers/base_discussion_entity_spec.rb

+# 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

spec/serializers/discussion_entity_spec.rb

@@ -39,6 +39,10 @@ RSpec.describe DiscussionEntity do
     )
   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
     Notes::ResolveService.new(note.project, user).execute(note)
 

spec/support/shared_examples/serializers/note_entity_shared_examples.rb

@@ -5,8 +5,21 @@ RSpec.shared_examples 'note entity' do
 
   context 'basic note' do
     it 'exposes correct elements' do
-      expect(subject).to include(:type, :author, :note, :note_html, :current_user, :discussion_id,
-        :emoji_awardable, :award_emoji, :report_abuse_path, :attachment, :noteable_note_url, :resolvable)
+      expect(subject).to include(
+        :attachment,
+        :author,
+        :award_emoji,
+        :base_discussion,
+        :current_user,
+        :discussion_id,
+        :emoji_awardable,
+        :note,
+        :note_html,
+        :noteable_note_url,
+        :report_abuse_path,
+        :resolvable,
+        :type
+      )
     end
 
     it 'does not expose elements for specific notes cases' do