Merge branch '290116-notes-finder' into 'master'

Add sort param to NotesFinder

See merge request gitlab-org/gitlab!56174

app/finders/notes_finder.rb

@@ -17,6 +17,7 @@ class NotesFinder
   #     target_id: integer
   #     last_fetched_at: time
   #     search: string
+  #     sort: string
   #
   def initialize(current_user, params = {})
     @project = params[:project]
@@ -29,8 +30,7 @@ class NotesFinder
     notes = init_collection
     notes = since_fetch_at(notes)
     notes = notes.with_notes_filter(@params[:notes_filter]) if notes_filter?
-
-    notes.fresh
+    sort(notes)
   end
 
   def target
@@ -173,6 +173,14 @@ class NotesFinder
   def notes_filter?
     @params[:notes_filter].present?
   end
+
+  def sort(notes)
+    sort = @params[:sort].presence
+
+    return notes.fresh unless sort
+
+    notes.order_by(sort)
+  end
 end
 
 NotesFinder.prepend_if_ee('EE::NotesFinder')

app/models/concerns/sortable.rb

@@ -9,6 +9,7 @@ module Sortable
 
   included do
     scope :with_order_id_desc, -> { order(self.arel_table['id'].desc) }
+    scope :with_order_id_asc, -> { order(self.arel_table['id'].asc) }
     scope :order_id_desc, -> { reorder(self.arel_table['id'].desc) }
     scope :order_id_asc, -> { reorder(self.arel_table['id'].asc) }
     scope :order_created_desc, -> { reorder(self.arel_table['created_at'].desc) }

app/models/note.rb

@@ -19,6 +19,7 @@ class Note < ApplicationRecord
   include Gitlab::SQL::Pattern
   include ThrottledTouch
   include FromUnion
+  include Sortable
 
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
 
@@ -103,10 +104,9 @@ class Note < ApplicationRecord
   scope :system, -> { where(system: true) }
   scope :user, -> { where(system: false) }
   scope :common, -> { where(noteable_type: ["", nil]) }
-  scope :fresh, -> { order(created_at: :asc, id: :asc) }
+  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 :by_updated_at, -> { reorder(:updated_at, :id) }
   scope :inc_author_project, -> { includes(:project, :author) }
   scope :inc_author, -> { includes(:author) }
   scope :inc_relations_for_view, -> do
@@ -148,6 +148,8 @@ class Note < ApplicationRecord
   after_commit :notify_after_destroy, on: :destroy
 
   class << self
+    extend Gitlab::Utils::Override
+
     def model_name
       ActiveModel::Name.new(self, nil, 'note')
     end
@@ -204,6 +206,13 @@ class Note < ApplicationRecord
     def search(query)
       fuzzy_search(query, [:note])
     end
+
+    # Override the `Sortable` module's `.simple_sorts` to remove name sorting,
+    # as a `Note` does not have any property that correlates to a "name".
+    override :simple_sorts
+    def simple_sorts
+      super.except('name_asc', 'name_desc')
+    end
   end
 
   # rubocop: disable CodeReuse/ServiceClass

app/services/concerns/integrations/project_test_data.rb

@@ -9,7 +9,7 @@ module Integrations
     end
 
     def note_events_data
-      note = NotesFinder.new(current_user, project: project, target: project).execute.reorder(nil).last # rubocop: disable CodeReuse/ActiveRecord
+      note = NotesFinder.new(current_user, project: project, target: project, sort: 'id_desc').execute.first
 
       return { error: s_('TestHooks|Ensure the project has notes.') } unless note.present?
 

lib/gitlab/updated_notes_paginator.rb

@@ -37,7 +37,7 @@ module Gitlab
     end
 
     def fetch_page(relation)
-      relation = relation.by_updated_at
+      relation = relation.order_updated_asc.with_order_id_asc
       notes = relation.limit(LIMIT + 1).to_a
 
       return [notes, false] unless notes.size > LIMIT

spec/finders/notes_finder_spec.rb

@@ -213,6 +213,24 @@ RSpec.describe NotesFinder do
         expect { described_class.new(user, params).execute }.to raise_error(RuntimeError)
       end
     end
+
+    describe 'sorting' do
+      it 'allows sorting' do
+        params = { project: project, sort: 'id_desc' }
+
+        expect(Note).to receive(:order_id_desc).once
+
+        described_class.new(user, params).execute
+      end
+
+      it 'defaults to sort by .fresh' do
+        params = { project: project }
+
+        expect(Note).to receive(:fresh).once
+
+        described_class.new(user, params).execute
+      end
+    end
   end
 
   describe '.search' do

spec/models/concerns/sortable_spec.rb

@@ -3,6 +3,31 @@
 require 'spec_helper'
 
 RSpec.describe Sortable do
+  describe 'scopes' do
+    describe 'secondary ordering by id' do
+      let(:sorted_relation) { Group.all.order_created_asc }
+
+      def arel_orders(relation)
+        relation.arel.orders
+      end
+
+      it 'allows secondary ordering by id ascending' do
+        orders = arel_orders(sorted_relation.with_order_id_asc)
+
+        expect(orders.map { |arel| arel.expr.name }).to eq(%w(created_at id))
+        expect(orders).to all(be_kind_of(Arel::Nodes::Ascending))
+      end
+
+      it 'allows secondary ordering by id descending' do
+        orders = arel_orders(sorted_relation.with_order_id_desc)
+
+        expect(orders.map { |arel| arel.expr.name }).to eq(%w(created_at id))
+        expect(orders.first).to be_kind_of(Arel::Nodes::Ascending)
+        expect(orders.last).to be_kind_of(Arel::Nodes::Descending)
+      end
+    end
+  end
+
   describe '.order_by' do
     let(:arel_table) { Group.arel_table }
     let(:relation) { Group.all }

spec/models/note_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Note do
     it { is_expected.to include_module(Participable) }
     it { is_expected.to include_module(Mentionable) }
     it { is_expected.to include_module(Awardable) }
+    it { is_expected.to include_module(Sortable) }
   end
 
   describe 'validation' do
@@ -856,6 +857,12 @@ RSpec.describe Note do
     end
   end
 
+  describe '.simple_sorts' do
+    it 'does not contain name sorts' do
+      expect(described_class.simple_sorts.grep(/name/)).to be_empty
+    end
+  end
+
   describe '#for_project_snippet?' do
     it 'returns true for a project snippet note' do
       expect(build(:note_on_project_snippet).for_project_snippet?).to be true
@@ -1322,7 +1329,7 @@ RSpec.describe Note do
     let_it_be(:note1) { create(:note, note: 'Test 345') }
     let_it_be(:note2) { create(:note, note: 'Test 789') }
 
-    describe '#for_note_or_capitalized_note' do
+    describe '.for_note_or_capitalized_note' do
       it 'returns the expected matching note' do
         notes = described_class.for_note_or_capitalized_note('Test 345')
 
@@ -1344,7 +1351,7 @@ RSpec.describe Note do
       end
     end
 
-    describe '#like_note_or_capitalized_note' do
+    describe '.like_note_or_capitalized_note' do
       it 'returns the expected matching note' do
         notes = described_class.like_note_or_capitalized_note('Test 345')
 
@@ -1367,69 +1374,69 @@ RSpec.describe Note do
         expect(notes.second.id).to eq(note2.id)
       end
     end
+  end
 
-    describe '#noteable_assignee_or_author' do
-      let(:user) { create(:user) }
-      let(:noteable) { create(:issue) }
-      let(:note) { create(:note, project: noteable.project, noteable: noteable) }
+  describe '#noteable_assignee_or_author?' do
+    let(:user) { create(:user) }
+    let(:noteable) { create(:issue) }
+    let(:note) { create(:note, project: noteable.project, noteable: noteable) }
 
-      subject { note.noteable_assignee_or_author?(user) }
+    subject { note.noteable_assignee_or_author?(user) }
 
-      shared_examples 'assignee check' do
-        context 'when the provided user is one of the assignees' do
-          before do
-            note.noteable.update(assignees: [user, create(:user)])
-          end
+    shared_examples 'assignee check' do
+      context 'when the provided user is one of the assignees' do
+        before do
+          note.noteable.update(assignees: [user, create(:user)])
+        end
 
-          it 'returns true' do
-            expect(subject).to be_truthy
-          end
+        it 'returns true' do
+          expect(subject).to be_truthy
         end
       end
+    end
 
-      shared_examples 'author check' do
-        context 'when the provided user is the author' do
-          before do
-            note.noteable.update(author: user)
-          end
-
-          it 'returns true' do
-            expect(subject).to be_truthy
-          end
+    shared_examples 'author check' do
+      context 'when the provided user is the author' do
+        before do
+          note.noteable.update(author: user)
         end
 
-        context 'when the provided user is neither author nor assignee' do
-          it 'returns true' do
-            expect(subject).to be_falsey
-          end
+        it 'returns true' do
+          expect(subject).to be_truthy
         end
       end
 
-      context 'when user is nil' do
-        let(:user) { nil }
-
-        it 'returns false' do
+      context 'when the provided user is neither author nor assignee' do
+        it 'returns true' do
           expect(subject).to be_falsey
         end
       end
+    end
+
+    context 'when user is nil' do
+      let(:user) { nil }
 
-      context 'when noteable is an issue' do
-        it_behaves_like 'author check'
-        it_behaves_like 'assignee check'
+      it 'returns false' do
+        expect(subject).to be_falsey
       end
+    end
 
-      context 'when noteable is a merge request' do
-        let(:noteable) { create(:merge_request) }
+    context 'when noteable is an issue' do
+      it_behaves_like 'author check'
+      it_behaves_like 'assignee check'
+    end
 
-        it_behaves_like 'author check'
-        it_behaves_like 'assignee check'
-      end
+    context 'when noteable is a merge request' do
+      let(:noteable) { create(:merge_request) }
 
-      context 'when noteable is a snippet' do
-        let(:noteable) { create(:personal_snippet) }
+      it_behaves_like 'author check'
+      it_behaves_like 'assignee check'
+    end
 
-        it_behaves_like 'author check'
-      end
+    context 'when noteable is a snippet' do
+      let(:noteable) { create(:personal_snippet) }
+
+      it_behaves_like 'author check'
     end
   end