Merge branch '33182-fix-productivity-analytics-multiple-labels-bug' into 'master'

Fix productivity analytics listing with multiple labels

Closes #33182

See merge request gitlab-org/gitlab!19238

changelogs/unreleased/33182-fix-productivity-analytics-multiple-labels-bug.yml

+---
+title: Fix productivity analytics listing with multiple labels
+merge_request: 33182
+author:
+type: fixed

ee/app/models/productivity_analytics.rb

@@ -43,28 +43,36 @@ class ProductivityAnalytics
     end
     columns.unshift(MergeRequest.arel_table[Arel.star])
 
-    mrs = merge_requests.select(columns)
-    mrs = mrs.reorder(custom_sorting) if custom_sorting
-    mrs
+    MergeRequest.joins(:metrics).select(columns).where(id: merge_requests).order(sorting)
   end
 
   private
 
   def histogram_query(column)
-    merge_requests.except(:select).select("#{column} as metric, count(*) as mr_count").group(column).reorder(nil)
+    MergeRequest::Metrics.joins(:merge_request)
+      .where(merge_request_id: merge_requests)
+      .select("#{column} as metric, count(*) as mr_count")
+      .group(column)
   end
 
   def scatterplot_query(column)
-    merge_requests.except(:select).select("#{column} as metric, merge_requests.id, merge_request_metrics.merged_at").reorder("merge_request_metrics.merged_at ASC")
+    merge_requests
+      .except(:select)
+      .select("#{column} as metric, merge_requests.id, merge_request_metrics.merged_at")
+      .reorder("merge_request_metrics.merged_at ASC")
   end
 
-  def custom_sorting
-    return unless sort
+  def sorting
+    return default_sorting unless sort
 
     column, direction = sort.split(/_(asc|desc)$/i)
 
-    return unless column.in?(METRIC_TYPES)
+    return default_sorting unless column.in?(METRIC_TYPES)
 
     Arel.sql("#{column} #{direction}")
   end
+
+  def default_sorting
+    { id: :desc }
+  end
 end

ee/spec/models/productivity_analytics_spec.rb

@@ -3,10 +3,17 @@
 require 'spec_helper'
 
 describe ProductivityAnalytics do
-  subject(:analytics) { described_class.new(merge_requests: MergeRequest.all, sort: custom_sort) }
+  describe 'metrics data' do
+    let(:analytics) { described_class.new(merge_requests: finder_mrs, sort: custom_sort) }
+
+    let(:finder_mrs) { ProductivityAnalyticsFinder.new(create(:admin), finder_options).execute }
+    let(:finder_options) { { state: 'merged' } }
 
     let(:custom_sort) { nil }
 
+    let(:label_a) { create(:label) }
+    let(:label_b) { create(:label) }
+
     let(:long_mr) do
       metrics_data = {
         merged_at: 1.day.ago,
@@ -16,7 +23,10 @@ describe ProductivityAnalytics do
         diff_size: 310,
         modified_paths_size: 15
       }
-    create(:merge_request, :merged, :with_productivity_metrics, created_at: 31.days.ago, metrics_data: metrics_data)
+      create(:labeled_merge_request, :merged, :with_productivity_metrics,
+             labels: [label_a, label_b],
+             created_at: 31.days.ago,
+             metrics_data: metrics_data)
     end
 
     let(:medium_mr) do
@@ -29,7 +39,9 @@ describe ProductivityAnalytics do
         modified_paths_size: 3
       }
 
-    create(:merge_request, :merged, :with_productivity_metrics, created_at: 15.days.ago, metrics_data: metrics_data)
+      create(:labeled_merge_request, :merged, :with_productivity_metrics,
+             created_at: 15.days.ago,
+             metrics_data: metrics_data)
     end
 
     let(:short_mr) do
@@ -42,7 +54,10 @@ describe ProductivityAnalytics do
         modified_paths_size: 3
       }
 
-    create(:merge_request, :merged, :with_productivity_metrics, created_at: 31.days.ago, metrics_data: metrics_data)
+      create(:labeled_merge_request, :merged, :with_productivity_metrics,
+             labels: [label_a, label_b],
+             created_at: 31.days.ago,
+             metrics_data: metrics_data)
     end
 
     let(:short_mr_2) do
@@ -55,7 +70,51 @@ describe ProductivityAnalytics do
         modified_paths_size: 1
       }
 
-    create(:merge_request, :merged, :with_productivity_metrics, created_at: 31.days.ago, metrics_data: metrics_data)
+      create(:labeled_merge_request, :merged, :with_productivity_metrics,
+             labels: [label_a, label_b],
+             created_at: 31.days.ago,
+             metrics_data: metrics_data)
+    end
+
+    let(:expected_mr_data) do
+      {
+        long_mr: {
+          'days_to_merge' => 30,
+          'time_to_first_comment' => 0,
+          'time_to_last_commit' => 29 * 24,
+          'time_to_merge' => 24,
+          'commits_count' => 20,
+          'loc_per_commit' => 15,
+          'files_touched' => 15
+        },
+        medium_mr: {
+          'days_to_merge' => 14,
+          'time_to_first_comment' => 0,
+          'time_to_last_commit' => 13 * 24,
+          'time_to_merge' => 24,
+          'commits_count' => 5,
+          'loc_per_commit' => 16,
+          'files_touched' => 3
+        },
+        short_mr: {
+          'days_to_merge' => 3,
+          'time_to_first_comment' => 24,
+          'time_to_last_commit' => 2 * 24,
+          'time_to_merge' => 0,
+          'commits_count' => 1,
+          'loc_per_commit' => 14,
+          'files_touched' => 3
+        },
+        short_mr_2: {
+          'days_to_merge' => 3,
+          'time_to_first_comment' => 0,
+          'time_to_last_commit' => 2 * 24,
+          'time_to_merge' => 24,
+          'commits_count' => 1,
+          'loc_per_commit' => 5,
+          'files_touched' => 1
+        }
+      }
     end
 
     before do
@@ -68,77 +127,43 @@ describe ProductivityAnalytics do
     end
 
     describe '#histogram_data' do
-    subject { analytics.histogram_data(type: metric) }
-
-    context 'days_to_merge metric' do
-      let(:metric) { 'days_to_merge' }
+      subject(:histogram_data) { analytics.histogram_data(type: metric) }
 
-      it 'returns aggregated data per days to merge from MR creation date' do
-        expect(subject).to eq(3 => 2, 14 => 1, 30 => 1)
-      end
-    end
-
-    context 'time_to_first_comment metric' do
-      let(:metric) { 'time_to_first_comment' }
-
-      it 'returns aggregated data per hours from MR creation to first comment' do
-        expect(subject).to eq(0 => 3, 24 => 1)
-      end
-    end
-
-    context 'time_to_last_commit metric' do
-      let(:metric) { 'time_to_last_commit' }
+      using RSpec::Parameterized::TableSyntax
 
-      it 'returns aggregated data per hours from first comment to last commit' do
-        expect(subject).to eq(13 * 24 => 1, 29 * 24 => 1, 2 * 24 => 2)
+      where(:metric, :expected_result) do
+        'days_to_merge' | { 3 => 2, 14 => 1, 30 => 1 }
+        'time_to_first_comment' | { 0 => 3, 24 => 1 }
+        'time_to_last_commit' | { 13 * 24 => 1, 29 * 24 => 1, 2 * 24 => 2 }
+        'time_to_merge' | { 24 => 3, 0 => 1 }
+        'commits_count' | { 1 => 2, 5 => 1, 20 => 1 }
+        'loc_per_commit' | { 15 => 1, 16 => 1, 14 => 1, 5 => 1 }
+        'files_touched' | { 15 => 1, 3 => 2, 1 => 1 }
+        'something_invalid' | nil
       end
-    end
-
-    context 'time_to_merge metric' do
-      let(:metric) { 'time_to_merge' }
-
-      it 'returns aggregated data per hours from last commit to merge' do
-        expect(subject).to eq(24 => 3, 0 => 1)
-      end
-    end
-
-    context 'commits_count metric' do
-      let(:metric) { 'commits_count' }
 
-      it 'returns aggregated data per number of commits' do
-        expect(subject).to eq(1 => 2, 5 => 1, 20 => 1)
+      with_them do
+        it 'calculates correctly' do
+          expect(analytics.histogram_data(type: metric)).to eq(expected_result)
         end
       end
 
-    context 'loc_per_commit metric' do
-      let(:metric) { 'loc_per_commit' }
+      context 'for multiple labeled mrs' do
+        let(:finder_options) { super().merge(label_name: [label_a.title, label_b.title]) }
+        let(:metric) { 'days_to_merge' }
 
-      it 'returns aggregated data per number of LoC/commits_count' do
-        expect(subject).to eq(15 => 1, 16 => 1, 14 => 1, 5 => 1)
-      end
+        it 'returns aggregated data' do
+          expect(analytics.histogram_data(type: 'days_to_merge')).to eq(3 => 2, 30 => 1)
         end
-
-    context 'files_touched metric' do
-      let(:metric) { 'files_touched' }
-
-      it 'returns aggregated data per number of modified files' do
-        expect(subject).to eq(15 => 1, 3 => 2, 1 => 1)
-      end
-    end
-
-    context 'for invalid metric' do
-      let(:metric) { 'something_invalid' }
-
-      it { is_expected.to eq nil }
       end
     end
 
     # Test coverage depends on #histogram_data tests. We want to avoid duplication here, so test only for 1 metric.
     describe '#scatterplot_data' do
-    subject { analytics.scatterplot_data(type: 'days_to_merge') }
+      subject(:scatterplot_data) { analytics.scatterplot_data(type: 'days_to_merge') }
 
       it 'returns metric values for each MR' do
-      expect(subject).to match(
+        expect(scatterplot_data).to match(
           short_mr.id => { metric: 3, merged_at: be_like_time(short_mr.merged_at) },
           short_mr_2.id => { metric: 3, merged_at: be_like_time(short_mr_2.merged_at) },
           medium_mr.id => { metric: 14, merged_at: be_like_time(medium_mr.merged_at) },
@@ -148,50 +173,18 @@ describe ProductivityAnalytics do
     end
 
     describe '#merge_requests_extended' do
-    subject { analytics.merge_requests_extended }
+      subject(:merge_requests) { analytics.merge_requests_extended }
 
       it 'returns MRs data with all the metrics calculated' do
         expected_data = {
-        long_mr.id => {
-          'days_to_merge' => 30,
-          'time_to_first_comment' => 0,
-          'time_to_last_commit' => 29 * 24,
-          'time_to_merge' => 24,
-          'commits_count' => 20,
-          'loc_per_commit' => 15,
-          'files_touched' => 15
-        },
-        medium_mr.id => {
-          'days_to_merge' => 14,
-          'time_to_first_comment' => 0,
-          'time_to_last_commit' => 13 * 24,
-          'time_to_merge' => 24,
-          'commits_count' => 5,
-          'loc_per_commit' => 16,
-          'files_touched' => 3
-        },
-        short_mr.id => {
-          'days_to_merge' => 3,
-          'time_to_first_comment' => 24,
-          'time_to_last_commit' => 2 * 24,
-          'time_to_merge' => 0,
-          'commits_count' => 1,
-          'loc_per_commit' => 14,
-          'files_touched' => 3
-        },
-        short_mr_2.id => {
-          'days_to_merge' => 3,
-          'time_to_first_comment' => 0,
-          'time_to_last_commit' => 2 * 24,
-          'time_to_merge' => 24,
-          'commits_count' => 1,
-          'loc_per_commit' => 5,
-          'files_touched' => 1
-        }
+          long_mr.id => expected_mr_data[:long_mr],
+          medium_mr.id => expected_mr_data[:medium_mr],
+          short_mr.id => expected_mr_data[:short_mr],
+          short_mr_2.id => expected_mr_data[:short_mr_2]
         }
 
         expected_data.each do |mr_id, expected_attributes|
-        expect(subject.detect { |mr| mr.id == mr_id}.attributes).to include(expected_attributes)
+          expect(merge_requests.detect { |mr| mr.id == mr_id }.attributes).to include(expected_attributes)
         end
       end
 
@@ -199,13 +192,31 @@ describe ProductivityAnalytics do
         let(:custom_sort) { 'loc_per_commit_asc' }
 
         it 'reorders MRs according to custom sorting' do
-        expect(subject).to eq [short_mr_2, short_mr, long_mr, medium_mr]
+          expect(merge_requests).to eq [short_mr_2, short_mr, long_mr, medium_mr]
         end
 
         context 'with unknown sorting' do
           let(:custom_sort) { 'weird_stuff' }
-        it 'does not apply custom sorting' do
-          expect(subject).to eq [long_mr, medium_mr, short_mr, short_mr_2]
+
+          it 'sorts by id desc' do
+            expect(merge_requests).to eq [short_mr_2, short_mr, medium_mr, long_mr]
+          end
+        end
+      end
+
+      context 'for multiple labeled mrs' do
+        let(:finder_options) { super().merge(label_name: [label_a.title, label_b.title]) }
+
+        it 'properly returns MRs with metrics calculated' do
+          expected_data = {
+            long_mr.id => expected_mr_data[:long_mr],
+            short_mr.id => expected_mr_data[:short_mr],
+            short_mr_2.id => expected_mr_data[:short_mr_2]
+          }
+
+          expected_data.each do |mr_id, expected_attributes|
+            expect(merge_requests.detect { |mr| mr.id == mr_id }.attributes).to include(expected_attributes)
+          end
         end
       end
     end