Merge branch 'dz-integrated-error-tracking-sort' into 'master'

Add sorting to integrated error tracking

See merge request gitlab-org/gitlab!68777

app/finders/error_tracking/errors_finder.rb

@@ -13,9 +13,10 @@ module ErrorTracking
 
       collection = project.error_tracking_errors
       collection = by_status(collection)
+      collection = sort(collection)
 
-      # Limit collection until pagination implemented
-      collection.limit(20)
+      # Limit collection until pagination implemented.
+      limit(collection)
     end
 
     private
@@ -33,5 +34,14 @@ module ErrorTracking
     def authorized?
       Ability.allowed?(current_user, :read_sentry_issue, project)
     end
+
+    def sort(collection)
+      params[:sort] ? collection.sort_by_attribute(params[:sort]) : collection.order_id_desc
+    end
+
+    def limit(collection)
+      # Restrict the maximum limit at 100 records.
+      collection.limit([(params[:limit] || 20).to_i, 100].min)
+    end
   end
 end

app/models/error_tracking/error.rb

 # frozen_string_literal: true
 
 class ErrorTracking::Error < ApplicationRecord
+  include Sortable
+
   belongs_to :project
 
   has_many :events, class_name: 'ErrorTracking::ErrorEvent'
@@ -34,6 +36,19 @@ class ErrorTracking::Error < ApplicationRecord
     end
   end
 
+  def self.sort_by_attribute(method)
+    case method.to_s
+    when 'last_seen'
+      order(last_seen_at: :desc)
+    when 'first_seen'
+      order(first_seen_at: :desc)
+    when 'frequency'
+      order(events_count: :desc)
+    else
+      order_id_desc
+    end
+  end
+
   def title
     if description.present?
       "#{name} #{description}"

app/services/error_tracking/list_issues_service.rb

@@ -74,7 +74,9 @@ module ErrorTracking
         # We are going to support more options in the future.
         # For now we implement the bare minimum for rendering the list in UI.
         filter_opts = {
-          status: opts[:issue_status]
+          status: opts[:issue_status],
+          sort: opts[:sort],
+          limit: opts[:limit]
         }
 
         errors = ErrorTracking::ErrorsFinder.new(current_user, project, filter_opts).execute

db/migrate/20210826124311_add_index_to_error_tracking_error.rb

+# frozen_string_literal: true
+
+class AddIndexToErrorTrackingError < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :last_seen_at], name: 'index_et_errors_on_project_id_and_status_and_last_seen_at'
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :first_seen_at], name: 'index_et_errors_on_project_id_and_status_and_first_seen_at'
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :events_count], name: 'index_et_errors_on_project_id_and_status_and_events_count'
+    add_concurrent_index :error_tracking_errors, [:project_id, :status, :id], name: 'index_et_errors_on_project_id_and_status_and_id'
+  end
+
+  def down
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :last_seen_at], name: 'index_et_errors_on_project_id_and_status_and_last_seen_at'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :first_seen_at], name: 'index_et_errors_on_project_id_and_status_and_first_seen_at'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :events_count], name: 'index_et_errors_on_project_id_and_status_and_events_count'
+    remove_concurrent_index :error_tracking_errors, [:project_id, :status, :id], name: 'index_et_errors_on_project_id_and_status_and_id'
+  end
+end

db/schema_migrations/20210826124311

+2cad14b3b7cb4f958a26cb6d4e76380338b745cc90c2e31c521614ea277c4ee9
\ No newline at end of file

db/structure.sql

@@ -24048,6 +24048,14 @@ CREATE UNIQUE INDEX index_escalation_rules_on_all_attributes ON incident_managem
 
 CREATE INDEX index_escalation_rules_on_user ON incident_management_escalation_rules USING btree (user_id);
 
+CREATE INDEX index_et_errors_on_project_id_and_status_and_events_count ON error_tracking_errors USING btree (project_id, status, events_count);
+
+CREATE INDEX index_et_errors_on_project_id_and_status_and_first_seen_at ON error_tracking_errors USING btree (project_id, status, first_seen_at);
+
+CREATE INDEX index_et_errors_on_project_id_and_status_and_id ON error_tracking_errors USING btree (project_id, status, id);
+
+CREATE INDEX index_et_errors_on_project_id_and_status_and_last_seen_at ON error_tracking_errors USING btree (project_id, status, last_seen_at);
+
 CREATE INDEX index_events_on_action ON events USING btree (action);
 
 CREATE INDEX index_events_on_author_id_and_created_at ON events USING btree (author_id, created_at);

spec/finders/error_tracking/errors_finder_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe ErrorTracking::ErrorsFinder do
   let_it_be(:user) { project.creator }
   let_it_be(:error) { create(:error_tracking_error, project: project) }
   let_it_be(:error_resolved) { create(:error_tracking_error, :resolved, project: project) }
+  let_it_be(:error_yesterday) { create(:error_tracking_error, project: project, first_seen_at: Time.zone.now.yesterday) }
 
   before do
     project.add_maintainer(user)
@@ -17,12 +18,24 @@ RSpec.describe ErrorTracking::ErrorsFinder do
 
     subject { described_class.new(user, project, params).execute }
 
-    it { is_expected.to contain_exactly(error, error_resolved) }
+    it { is_expected.to contain_exactly(error, error_resolved, error_yesterday) }
 
     context 'with status parameter' do
       let(:params) { { status: 'resolved' } }
 
       it { is_expected.to contain_exactly(error_resolved) }
     end
+
+    context 'with sort parameter' do
+      let(:params) { { status: 'unresolved', sort: 'first_seen' } }
+
+      it { is_expected.to eq([error, error_yesterday]) }
+    end
+
+    context 'with limit parameter' do
+      let(:params) { { limit: '1', sort: 'first_seen' } }
+
+      it { is_expected.to contain_exactly(error) }
+    end
   end
 end

spec/models/error_tracking/error_spec.rb

@@ -34,6 +34,44 @@ RSpec.describe ErrorTracking::Error, type: :model do
     end
   end
 
+  describe '.sort_by_attribute' do
+    let!(:error2) { create(:error_tracking_error, first_seen_at: Time.zone.now - 2.weeks, last_seen_at: Time.zone.now - 1.week) }
+    let!(:error3) { create(:error_tracking_error, first_seen_at: Time.zone.now - 3.weeks, last_seen_at: Time.zone.now.yesterday) }
+    let!(:errors) { [error, error2, error3] }
+
+    subject { described_class.where(id: errors).sort_by_attribute(sort) }
+
+    context 'id desc by default' do
+      let(:sort) { nil }
+
+      it { is_expected.to eq([error3, error2, error]) }
+    end
+
+    context 'first_seen' do
+      let(:sort) { 'first_seen' }
+
+      it { is_expected.to eq([error, error2, error3]) }
+    end
+
+    context 'last_seen' do
+      let(:sort) { 'last_seen' }
+
+      it { is_expected.to eq([error, error3, error2]) }
+    end
+
+    context 'frequency' do
+      let(:sort) { 'frequency' }
+
+      before do
+        create(:error_tracking_error_event, error: error2)
+        create(:error_tracking_error_event, error: error2)
+        create(:error_tracking_error_event, error: error3)
+      end
+
+      it { is_expected.to eq([error2, error3, error]) }
+    end
+  end
+
   describe '#title' do
     it { expect(error.title).to eq('ActionView::MissingTemplate Missing template posts/edit') }
   end