Merge branch '321575-add-sorting-to-vsa' into 'master'

Add sorting options to value stream analytics

See merge request gitlab-org/gitlab!54696

ee/lib/gitlab/analytics/cycle_analytics/data_for_duration_chart.rb

@@ -16,7 +16,7 @@ module Gitlab
 
         # rubocop: disable CodeReuse/ActiveRecord
         def load
-          order_by_end_event(query)
+          order_by(query, :end_event, :desc)
             .select(round_duration_to_seconds.as('duration_in_seconds'), stage.end_event.timestamp_projection.as('finished_at'))
             .limit(MAX_RESULTS)
         end

ee/lib/gitlab/analytics/cycle_analytics/request_params.rb

@@ -16,6 +16,8 @@ module Gitlab
           :created_after,
           :author_username,
           :milestone_title,
+          :sort,
+          :direction,
           label_name: [].freeze,
           assignee_username: [].freeze,
           project_ids: [].freeze
@@ -35,6 +37,8 @@ module Gitlab
         attribute :group
         attribute :current_user
         attribute :value_stream
+        attribute :sort
+        attribute :direction
 
         FINDER_PARAM_NAMES.each do |param_name|
           attribute param_name
@@ -62,7 +66,9 @@ module Gitlab
             current_user: current_user,
             from: created_after,
             to: created_before,
-            project_ids: project_ids
+            project_ids: project_ids,
+            sort: sort&.to_sym,
+            direction: direction&.to_sym
           }.merge(attributes.symbolize_keys.slice(*FINDER_PARAM_NAMES))
         end
 
@@ -77,6 +83,8 @@ module Gitlab
             attrs[:assignees] = assignee_username.to_json if assignee_username.present?
             attrs[:author] = author_username if author_username.present?
             attrs[:milestone] = milestone_title if milestone_title.present?
+            attrs[:sort] = sort if sort.present?
+            attrs[:direction] = direction if direction.present?
           end
         end
 

ee/spec/lib/gitlab/analytics/cycle_analytics/data_collector_spec.rb

@@ -22,33 +22,43 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do
     let(:params) { { from: Time.new(2019), to: Time.new(2020), current_user: user } }
     let(:data_collector) { described_class.new(stage: stage, params: params) }
 
-    before do
+    let!(:resource1) do
       # takes 10 days
-      resource1 = travel_to(Time.new(2019, 3, 5)) do
+      resource = travel_to(Time.new(2019, 3, 5)) do
         create_data_for_start_event(self)
       end
 
       travel_to(Time.new(2019, 3, 15)) do
-        create_data_for_end_event(resource1, self)
+        create_data_for_end_event(resource, self)
       end
 
+      resource
+    end
+
+    let!(:resource2) do
       # takes 5 days
-      resource2 = travel_to(Time.new(2019, 3, 5)) do
+      resource = travel_to(Time.new(2019, 3, 5)) do
         create_data_for_start_event(self)
       end
 
       travel_to(Time.new(2019, 3, 10)) do
-        create_data_for_end_event(resource2, self)
+        create_data_for_end_event(resource, self)
       end
 
+      resource
+    end
+
+    let!(:resource3) do
       # takes 15 days
-      resource3 = travel_to(Time.new(2019, 3, 5)) do
+      resource = travel_to(Time.new(2019, 3, 5)) do
         create_data_for_start_event(self)
       end
 
       travel_to(Time.new(2019, 3, 20)) do
-        create_data_for_end_event(resource3, self)
+        create_data_for_end_event(resource, self)
       end
+
+      resource
     end
 
     it 'loads serialized records' do
@@ -56,6 +66,20 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::DataCollector do
       expect(items.size).to eq(3)
     end
 
+    context 'when sorting by duration' do
+      before do
+        params[:sort] = :duration
+        params[:direction] = :desc
+      end
+
+      it 'returns serialized records sorted by duration DESC' do
+        expected_ordered_iids = [resource3.iid, resource1.iid, resource2.iid]
+
+        iids = data_collector.serialized_records.map { |record| record[:iid].to_i }
+        expect(iids).to eq(expected_ordered_iids)
+      end
+    end
+
     it 'calculates median' do
       expect(round_to_days(data_collector.median.seconds)).to eq(10)
     end

ee/spec/lib/gitlab/analytics/cycle_analytics/request_params_spec.rb

@@ -205,4 +205,24 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::RequestParams do
     it { expect(subject[:labels]).to eq('["label1","label2"]') }
     it { expect(subject[:author]).to eq('author') }
   end
+
+  describe 'sorting params' do
+    before do
+      params.merge!(sort: 'duration', direction: 'asc')
+    end
+
+    it 'converts sorting params to symbol when passing it to data collector' do
+      data_collector_params = subject.to_data_collector_params
+
+      expect(data_collector_params[:sort]).to eq(:duration)
+      expect(data_collector_params[:direction]).to eq(:asc)
+    end
+
+    it 'adds corting params to data attributes' do
+      data_attributes = subject.to_data_attributes
+
+      expect(data_attributes[:sort]).to eq('duration')
+      expect(data_attributes[:direction]).to eq('asc')
+    end
+  end
 end

ee/spec/support/shared_examples/controllers/analytics/cycle_analytics/shared_stage_shared_examples.rb

@@ -220,6 +220,20 @@ RSpec.shared_examples 'Value Stream Analytics Stages controller' do
 
       include_examples 'Value Stream Analytics data endpoint examples'
       include_examples 'group permission check on the controller level'
+
+      context 'sort params' do
+        before do
+          params.merge!(sort: 'duration', direction: 'asc')
+        end
+
+        it 'accepts sort params' do
+          expect(Gitlab::Analytics::CycleAnalytics::Sorting).to receive(:apply).with(kind_of(ActiveRecord::Relation), kind_of(Analytics::CycleAnalytics::GroupStage), :duration, :asc).and_call_original
+
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
     end
 
     describe 'GET #duration_chart' do

lib/gitlab/analytics/cycle_analytics/records_fetcher.rb

@@ -29,6 +29,8 @@ module Gitlab
           @stage = stage
           @query = query
           @params = params
+          @sort = params[:sort] || :end_event
+          @direction = params[:direction] || :desc
         end
 
         def serialized_records
@@ -52,7 +54,7 @@ module Gitlab
 
         private
 
-        attr_reader :stage, :query, :params
+        attr_reader :stage, :query, :params, :sort, :direction
 
         def columns
           MAPPINGS.fetch(subject_class).fetch(:columns_for_select).map do |column_name|
@@ -90,7 +92,7 @@ module Gitlab
         end
 
         def ordered_and_limited_query
-          order_by_end_event(query, columns).limit(MAX_RECORDS)
+          order_by(query, sort, direction, columns).limit(MAX_RECORDS)
         end
 
         def records

lib/gitlab/analytics/cycle_analytics/sorting.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Analytics
+    module CycleAnalytics
+      class Sorting
+        # rubocop: disable CodeReuse/ActiveRecord
+        SORTING_OPTIONS = {
+          end_event: {
+            asc: -> (query, stage) { query.reorder(stage.end_event.timestamp_projection.asc) },
+            desc: -> (query, stage) { query.reorder(stage.end_event.timestamp_projection.desc) }
+          }.freeze,
+          duration: {
+            asc: -> (query, stage) { query.reorder(Arel::Nodes::Subtraction.new(stage.end_event.timestamp_projection, stage.start_event.timestamp_projection).asc) },
+            desc: -> (query, stage) { query.reorder(Arel::Nodes::Subtraction.new(stage.end_event.timestamp_projection, stage.start_event.timestamp_projection).desc) }
+          }.freeze
+        }.freeze
+        # rubocop: enable CodeReuse/ActiveRecord,
+
+        def self.apply(query, stage, sort, direction)
+          sort_lambda = SORTING_OPTIONS.dig(sort, direction) || SORTING_OPTIONS.dig(:end_event, :desc)
+          sort_lambda.call(query, stage)
+        end
+      end
+    end
+  end
+end

lib/gitlab/analytics/cycle_analytics/stage_query_helpers.rb

@@ -24,8 +24,8 @@ module Gitlab
         end
 
         # rubocop: disable CodeReuse/ActiveRecord
-        def order_by_end_event(query, extra_columns_to_select = [:id])
-          ordered_query = query.reorder(stage.end_event.timestamp_projection.desc)
+        def order_by(query, sort, direction, extra_columns_to_select = [:id])
+          ordered_query = Gitlab::Analytics::CycleAnalytics::Sorting.apply(query, stage, sort, direction)
 
           # When filtering for more than one label, postgres requires the columns in ORDER BY to be present in the GROUP BY clause
           if requires_grouping?

spec/lib/gitlab/analytics/cycle_analytics/sorting_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Analytics::CycleAnalytics::Sorting do
+  let(:stage) { build(:cycle_analytics_project_stage, start_event_identifier: :merge_request_created, end_event_identifier: :merge_request_merged) }
+
+  subject(:order_values) { described_class.apply(MergeRequest.joins(:metrics), stage, sort, direction).order_values }
+
+  context 'when invalid sorting params are given' do
+    let(:sort) { :unknown_sort }
+    let(:direction) { :unknown_direction }
+
+    it 'falls back to end_event DESC sorting' do
+      expect(order_values).to eq([stage.end_event.timestamp_projection.desc])
+    end
+  end
+
+  context 'sorting end_event' do
+    let(:sort) { :end_event }
+
+    context 'direction desc' do
+      let(:direction) { :desc }
+
+      specify do
+        expect(order_values).to eq([stage.end_event.timestamp_projection.desc])
+      end
+    end
+
+    context 'direction asc' do
+      let(:direction) { :asc }
+
+      specify do
+        expect(order_values).to eq([stage.end_event.timestamp_projection.asc])
+      end
+    end
+  end
+
+  context 'sorting duration' do
+    let(:sort) { :duration }
+
+    context 'direction desc' do
+      let(:direction) { :desc }
+
+      specify do
+        expect(order_values).to eq([Arel::Nodes::Subtraction.new(stage.end_event.timestamp_projection, stage.start_event.timestamp_projection).desc])
+      end
+    end
+
+    context 'direction asc' do
+      let(:direction) { :asc }
+
+      specify do
+        expect(order_values).to eq([Arel::Nodes::Subtraction.new(stage.end_event.timestamp_projection, stage.start_event.timestamp_projection).asc])
+      end
+    end
+  end
+end