Paginate discussions endpoint

This can be used to progressively load the issuable discussions

app/controllers/concerns/issuable_actions.rb

@@ -149,8 +149,20 @@ module IssuableActions
                 .includes(:noteable)
                 .fresh
 
+    if paginated_discussions
+      paginated_discussions_by_type = paginated_discussions.records.group_by(&:table_name)
+
+      notes = if paginated_discussions_by_type['notes'].present?
+                notes.with_discussion_ids(paginated_discussions_by_type['notes'].map(&:discussion_id))
+              else
+                notes.none
+              end
+
+      response.headers['X-Next-Page-Cursor'] = paginated_discussions.cursor_for_next_page if paginated_discussions.has_next_page?
+    end
+
     if notes_filter != UserPreference::NOTES_FILTERS[:only_comments]
-      notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user).execute(notes)
+      notes = ResourceEvents::MergeIntoNotesService.new(issuable, current_user, paginated_notes: paginated_discussions_by_type).execute(notes)
     end
 
     notes = prepare_notes_for_rendering(notes)
@@ -170,6 +182,17 @@ module IssuableActions
 
   private
 
+  def paginated_discussions
+    return if params[:per_page].blank?
+    return unless issuable.instance_of?(Issue) && Feature.enabled?(:paginated_issue_discussions, project, default_enabled: :yaml)
+
+    strong_memoize(:paginated_discussions) do
+      issuable
+        .discussion_root_note_ids(notes_filter: notes_filter)
+        .keyset_paginate(cursor: params[:cursor], per_page: params[:per_page].to_i)
+    end
+  end
+
   def notes_filter
     strong_memoize(:notes_filter) do
       notes_filter_param = params[:notes_filter]&.to_i

app/helpers/notes_helper.rb

@@ -167,11 +167,11 @@ module NotesHelper
     }
   end
 
-  def discussions_path(issuable)
+  def discussions_path(issuable, **params)
     if issuable.is_a?(Issue)
-      discussions_project_issue_path(@project, issuable, format: :json)
+      discussions_project_issue_path(@project, issuable, params.merge(format: :json))
     else
-      discussions_project_merge_request_path(@project, issuable, format: :json)
+      discussions_project_merge_request_path(@project, issuable, params.merge(format: :json))
     end
   end
 

app/models/concerns/noteable.rb

@@ -98,6 +98,27 @@ module Noteable
       .order('MIN(created_at), MIN(id)')
   end
 
+  # This does not consider OutOfContextDiscussions in MRs
+  # where notes from commits are overriden so that they have
+  # the same discussion_id
+  def discussion_root_note_ids(notes_filter:)
+    relations = []
+
+    relations << discussion_notes.select(
+      "'notes' AS table_name",
+      'discussion_id',
+      'MIN(id) AS id',
+      'MIN(created_at) AS created_at'
+    ).with_notes_filter(notes_filter)
+     .group(:discussion_id)
+
+    if notes_filter != UserPreference::NOTES_FILTERS[:only_comments]
+      relations += synthetic_note_ids_relations
+    end
+
+    Note.from_union(relations, remove_duplicates: false).fresh
+  end
+
   def capped_notes_count(max)
     notes.limit(max).count
   end
@@ -179,6 +200,18 @@ module Noteable
 
     project_email.sub('@', "-#{iid}@")
   end
+
+  private
+
+  # Synthetic system notes don't have discussion IDs because these are generated dynamically
+  # in Ruby. These are always root notes anyway so we don't need to group by discussion ID.
+  def synthetic_note_ids_relations
+    [
+      resource_label_events.select("'resource_label_events'", "'NULL'", :id, :created_at),
+      resource_milestone_events.select("'resource_milestone_events'", "'NULL'", :id, :created_at),
+      resource_state_events.select("'resource_state_events'", "'NULL'", :id, :created_at)
+    ]
+  end
 end
 
 Noteable.extend(Noteable::ClassMethods)

app/models/note.rb

@@ -114,6 +114,7 @@ class Note < ApplicationRecord
   scope :fresh, -> { order_created_asc.with_order_id_asc }
   scope :updated_after, ->(time) { where('updated_at > ?', time) }
   scope :with_updated_at, ->(time) { where(updated_at: time) }
+  scope :with_discussion_ids, ->(discussion_ids) { where(discussion_id: discussion_ids) }
   scope :with_suggestions, -> { joins(:suggestions) }
   scope :inc_author, -> { includes(:author) }
   scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }

app/services/resource_events/base_synthetic_notes_builder_service.rb

@@ -24,10 +24,18 @@ module ResourceEvents
     private
 
     def apply_common_filters(events)
+      events = apply_pagination(events)
       events = apply_last_fetched_at(events)
       apply_fetch_until(events)
     end
 
+    def apply_pagination(events)
+      return events if params[:paginated_notes].nil?
+      return events.none if params[:paginated_notes][table_name].blank?
+
+      events.id_in(params[:paginated_notes][table_name].map(&:id))
+    end
+
     def apply_last_fetched_at(events)
       return events unless params[:last_fetched_at].present?
 
@@ -47,5 +55,9 @@ module ResourceEvents
         resource.project || resource.group
       end
     end
+
+    def table_name
+      raise NotImplementedError
+    end
   end
 end

app/services/resource_events/synthetic_label_notes_builder_service.rb

@@ -23,5 +23,9 @@ module ResourceEvents
 
       events.group_by { |event| event.discussion_id }
     end
+
+    def table_name
+      'resource_label_events'
+    end
   end
 end

app/services/resource_events/synthetic_milestone_notes_builder_service.rb

@@ -21,5 +21,9 @@ module ResourceEvents
       events = resource.resource_milestone_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
       apply_common_filters(events)
     end
+
+    def table_name
+      'resource_milestone_events'
+    end
   end
 end

app/services/resource_events/synthetic_state_notes_builder_service.rb

@@ -16,5 +16,9 @@ module ResourceEvents
       events = resource.resource_state_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
       apply_common_filters(events)
     end
+
+    def table_name
+      'resource_state_events'
+    end
   end
 end

app/views/projects/issues/_discussion.html.haml

-- add_page_startup_api_call discussions_path(@issue)
+- add_page_startup_api_call Feature.enabled?(:paginated_discussions, @project) ? discussions_path(@issue, per_page: 20) : discussions_path(@issue)
 
 - @gfm_form = true
 

ee/app/helpers/ee/notes_helper.rb

@@ -13,9 +13,9 @@ module EE
     end
 
     override :discussions_path
-    def discussions_path(issuable)
-      return discussions_group_epic_path(issuable.group, issuable, format: :json) if issuable.is_a?(Epic)
-      return discussions_project_security_vulnerability_path(issuable.project, issuable, format: :json) if issuable.is_a?(Vulnerability)
+    def discussions_path(issuable, **params)
+      return discussions_group_epic_path(issuable.group, issuable, params.merge(format: :json)) if issuable.is_a?(Epic)
+      return discussions_project_security_vulnerability_path(issuable.project, issuable, params.merge(format: :json)) if issuable.is_a?(Vulnerability)
 
       super
     end

ee/app/models/concerns/ee/noteable.rb

@@ -24,5 +24,22 @@ module EE
         super
       end
     end
+
+    private
+
+    override :synthetic_note_ids_relations
+    def synthetic_note_ids_relations
+      relations = super
+
+      if respond_to?(:resource_weight_events)
+        relations << resource_weight_events.select("'resource_weight_events'", "'NULL'", :id, :created_at)
+      end
+
+      if respond_to?(:resource_iteration_events)
+        relations << resource_iteration_events.select("'resource_iteration_events'", "'NULL'", :id, :created_at)
+      end
+
+      relations
+    end
   end
 end

ee/app/services/ee/resource_events/synthetic_iteration_notes_builder_service.rb

@@ -22,6 +22,10 @@ module EE
         events = resource.resource_iteration_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord
         apply_common_filters(events)
       end
+
+      def table_name
+        'resource_iteration_events'
+      end
     end
   end
 end

ee/app/services/ee/resource_events/synthetic_weight_notes_builder_service.rb

@@ -22,6 +22,10 @@ module EE
         events = resource.resource_weight_events.includes(user: :status).order(:id) # rubocop: disable CodeReuse/ActiveRecord
         apply_common_filters(events)
       end
+
+      def table_name
+        'resource_weight_events'
+      end
     end
   end
 end

ee/spec/models/concerns/ee/noteable_spec.rb

@@ -14,4 +14,44 @@ RSpec.describe EE::Noteable do
       expect(klazz.replyable_types).to include("Vulnerability")
     end
   end
+
+  describe '#discussion_root_note_ids' do
+    let_it_be(:issue) { create(:issue) }
+    let_it_be(:weight_event) { create(:resource_weight_event, issue: issue) }
+    let_it_be(:regular_note) { create(:note, project: issue.project, noteable: issue) }
+    let_it_be(:iteration_event) { create(:resource_iteration_event, issue: issue) }
+
+    it 'includes weight and iteration synthetic notes' do
+      discussions = issue.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:all_notes]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'resource_weight_events', id: weight_event.id),
+        a_hash_including(table_name: 'notes', discussion_id: regular_note.discussion_id),
+        a_hash_including(table_name: 'resource_iteration_events', id: iteration_event.id)
+      ])
+    end
+
+    it 'filters by comments only' do
+      discussions = issue.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:only_comments]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'notes', discussion_id: regular_note.discussion_id)
+      ])
+    end
+
+    it 'filters by system notes only' do
+      discussions = issue.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:only_activity]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'resource_weight_events', id: weight_event.id),
+        a_hash_including(table_name: 'resource_iteration_events', id: iteration_event.id)
+      ])
+    end
+  end
 end

ee/spec/services/ee/resource_events/synthetic_iteration_notes_builder_service_spec.rb

@@ -21,5 +21,7 @@ RSpec.describe EE::ResourceEvents::SyntheticIterationNotesBuilderService do
         expect(notes.size).to eq(3)
       end
     end
+
+    it_behaves_like 'filters by paginated notes', :resource_iteration_event
   end
 end

ee/spec/services/ee/resource_events/synthetic_weight_notes_builder_service_spec.rb

@@ -4,18 +4,20 @@ require 'spec_helper'
 
 RSpec.describe EE::ResourceEvents::SyntheticWeightNotesBuilderService do
   describe '#execute' do
-    let!(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
 
-    let!(:issue) { create(:issue, author: user) }
+    let_it_be(:issue) { create(:issue, author: user) }
 
-    let!(:event1) { create(:resource_weight_event, issue: issue) }
-    let!(:event2) { create(:resource_weight_event, issue: issue) }
-    let!(:event3) { create(:resource_weight_event, issue: issue) }
+    let_it_be(:event1) { create(:resource_weight_event, issue: issue) }
+    let_it_be(:event2) { create(:resource_weight_event, issue: issue) }
+    let_it_be(:event3) { create(:resource_weight_event, issue: issue) }
 
     it 'returns the expected synthetic notes' do
       notes = EE::ResourceEvents::SyntheticWeightNotesBuilderService.new(issue, user).execute
 
       expect(notes.size).to eq(3)
     end
+
+    it_behaves_like 'filters by paginated notes', :resource_weight_event
   end
 end

lib/api/discussions.rb

@@ -239,7 +239,7 @@ module API
       # rubocop: disable CodeReuse/ActiveRecord
       def readable_discussion_notes(noteable, discussion_ids)
         notes = noteable.notes
-          .where(discussion_id: discussion_ids)
+          .with_discussion_ids(discussion_ids)
           .inc_relations_for_view
           .includes(:noteable)
           .fresh

spec/models/concerns/noteable_spec.rb

@@ -77,6 +77,70 @@ RSpec.describe Noteable do
     end
   end
 
+  describe '#discussion_root_note_ids' do
+    let!(:label_event) { create(:resource_label_event, merge_request: subject) }
+    let!(:system_note) { create(:system_note, project: project, noteable: subject) }
+    let!(:milestone_event) { create(:resource_milestone_event, merge_request: subject) }
+    let!(:state_event) { create(:resource_state_event, merge_request: subject) }
+
+    it 'returns ordered discussion_ids and synthetic note ids' do
+      discussions = subject.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:all_notes]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'notes', discussion_id: active_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: active_diff_note3.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: outdated_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: discussion_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_note2.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_discussion_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_discussion_note3.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: note2.discussion_id),
+        a_hash_including(table_name: 'resource_label_events', id: label_event.id),
+        a_hash_including(table_name: 'notes', discussion_id: system_note.discussion_id),
+        a_hash_including(table_name: 'resource_milestone_events', id: milestone_event.id),
+        a_hash_including(table_name: 'resource_state_events', id: state_event.id)
+      ])
+    end
+
+    it 'filters by comments only' do
+      discussions = subject.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:only_comments]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'notes', discussion_id: active_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: active_diff_note3.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: outdated_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: discussion_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_diff_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_note2.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_discussion_note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: commit_discussion_note3.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: note1.discussion_id),
+        a_hash_including(table_name: 'notes', discussion_id: note2.discussion_id)
+      ])
+    end
+
+    it 'filters by system notes only' do
+      discussions = subject.discussion_root_note_ids(notes_filter: UserPreference::NOTES_FILTERS[:only_activity]).map do |n|
+        { table_name: n.table_name, discussion_id: n.discussion_id, id: n.id }
+      end
+
+      expect(discussions).to match([
+        a_hash_including(table_name: 'resource_label_events', id: label_event.id),
+        a_hash_including(table_name: 'notes', discussion_id: system_note.discussion_id),
+        a_hash_including(table_name: 'resource_milestone_events', id: milestone_event.id),
+        a_hash_including(table_name: 'resource_state_events', id: state_event.id)
+      ])
+    end
+  end
+
   describe '#grouped_diff_discussions' do
     let(:grouped_diff_discussions) { subject.grouped_diff_discussions }
 

spec/requests/projects/issues_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::IssuesController do
+  let_it_be(:issue) { create(:issue) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { issue.project }
+  let_it_be(:user) { issue.author }
+
+  before do
+    login_as(user)
+  end
+
+  describe 'GET #discussions' do
+    let_it_be(:discussion) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) }
+    let_it_be(:discussion_reply) { create(:discussion_note_on_issue, noteable: issue, project: issue.project, in_reply_to: discussion) }
+    let_it_be(:state_event) { create(:resource_state_event, issue: issue) }
+    let_it_be(:discussion_2) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) }
+    let_it_be(:discussion_3) { create(:discussion_note_on_issue, noteable: issue, project: issue.project) }
+
+    context 'pagination' do
+      def get_discussions(**params)
+        get discussions_project_issue_path(project, issue, params: params.merge(format: :json))
+      end
+
+      it 'returns paginated notes and cursor based on per_page param' do
+        get_discussions(per_page: 2)
+
+        discussions = Gitlab::Json.parse(response.body)
+        notes = discussions.flat_map { |d| d['notes'] }
+
+        expect(discussions.count).to eq(2)
+        expect(notes).to match([
+          a_hash_including('id' => discussion.id.to_s),
+          a_hash_including('id' => discussion_reply.id.to_s),
+          a_hash_including('type' => 'StateNote')
+        ])
+
+        cursor = response.header['X-Next-Page-Cursor']
+        expect(cursor).to be_present
+
+        get_discussions(per_page: 1, cursor: cursor)
+
+        discussions = Gitlab::Json.parse(response.body)
+        notes = discussions.flat_map { |d| d['notes'] }
+
+        expect(discussions.count).to eq(1)
+        expect(notes).to match([
+          a_hash_including('id' => discussion_2.id.to_s)
+        ])
+      end
+
+      context 'when paginated_issue_discussions is disabled' do
+        before do
+          stub_feature_flags(paginated_issue_discussions: false)
+        end
+
+        it 'returns all discussions and ignores per_page param' do
+          get_discussions(per_page: 2)
+
+          discussions = Gitlab::Json.parse(response.body)
+          notes = discussions.flat_map { |d| d['notes'] }
+
+          expect(discussions.count).to eq(4)
+          expect(notes.count).to eq(5)
+        end
+      end
+    end
+  end
+end

spec/services/resource_events/synthetic_label_notes_builder_service_spec.rb

@@ -4,18 +4,20 @@ require 'spec_helper'
 
 RSpec.describe ResourceEvents::SyntheticLabelNotesBuilderService do
   describe '#execute' do
-    let!(:user) { create(:user) }
+    let_it_be(:user) { create(:user) }
 
-    let!(:issue) { create(:issue, author: user) }
+    let_it_be(:issue) { create(:issue, author: user) }
 
-    let!(:event1) { create(:resource_label_event, issue: issue) }
-    let!(:event2) { create(:resource_label_event, issue: issue) }
-    let!(:event3) { create(:resource_label_event, issue: issue) }
+    let_it_be(:event1) { create(:resource_label_event, issue: issue) }
+    let_it_be(:event2) { create(:resource_label_event, issue: issue) }
+    let_it_be(:event3) { create(:resource_label_event, issue: issue) }
 
     it 'returns the expected synthetic notes' do
       notes = ResourceEvents::SyntheticLabelNotesBuilderService.new(issue, user).execute
 
       expect(notes.size).to eq(3)
     end
+
+    it_behaves_like 'filters by paginated notes', :resource_label_event
   end
 end

spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb

@@ -24,5 +24,7 @@ RSpec.describe ResourceEvents::SyntheticMilestoneNotesBuilderService do
         'removed milestone'
       ])
     end
+
+    it_behaves_like 'filters by paginated notes', :resource_milestone_event
   end
 end

spec/services/resource_events/synthetic_state_notes_builder_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ResourceEvents::SyntheticStateNotesBuilderService do
+  describe '#execute' do
+    let_it_be(:user) { create(:user) }
+
+    it_behaves_like 'filters by paginated notes', :resource_state_event
+  end
+end

spec/support/shared_examples/services/resource_events/synthetic_notes_builder_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'filters by paginated notes' do |event_type|
+  let(:event) { create(event_type) } # rubocop:disable Rails/SaveBang
+
+  before do
+    create(event_type, issue: event.issue)
+  end
+
+  it 'only returns given notes' do
+    paginated_notes = { event_type.to_s.pluralize => [double(id: event.id)] }
+    notes = described_class.new(event.issue, user, paginated_notes: paginated_notes).execute
+
+    expect(notes.size).to eq(1)
+    expect(notes.first.event).to eq(event)
+  end
+
+  context 'when paginated notes is empty' do
+    it 'does not return any notes' do
+      notes = described_class.new(event.issue, user, paginated_notes: {}).execute
+
+      expect(notes.size).to eq(0)
+    end
+  end
+end