Merge branch '214677-improve-vsa-summary-values' into 'master'

Move out VSA summary value formatting from the serializer

Closes #214677

See merge request gitlab-org/gitlab!31345

app/serializers/analytics_summary_entity.rb

@@ -8,8 +8,6 @@ class AnalyticsSummaryEntity < Grape::Entity
   private
 
   def value
-    return object.value if object.value.is_a? String
-
-    object.value&.nonzero? ? object.value.to_s : '-'
+    object.value.to_s
   end
 end

ee/lib/gitlab/analytics/cycle_analytics/summary/group/base_time.rb

@@ -13,7 +13,7 @@ module Gitlab
             end
 
             def value
-              @value ||= data_collector.median.days&.round(1)
+              @value ||= Gitlab::CycleAnalytics::Summary::Value::PrettyNumeric.new(data_collector.median.days&.round(1))
             end
 
             def unit

ee/lib/gitlab/analytics/cycle_analytics/summary/group/deploy.rb

@@ -13,19 +13,21 @@ module Gitlab
             end
 
             def value
-              @value ||= find_deployments
+              @value ||= ::Gitlab::CycleAnalytics::Summary::Value::PrettyNumeric.new(deployments_count)
             end
 
             private
 
             # rubocop: disable CodeReuse/ActiveRecord
-            def find_deployments
+            def deployments_count
+              @deployments_count ||= begin
                                        deployments = Deployment.joins(:project).merge(Project.inside_path(group.full_path))
                                        deployments = deployments.where(projects: { id: options[:projects] }) if options[:projects]
                                        deployments = deployments.where("deployments.created_at > ?", options[:from])
                                        deployments = deployments.where("deployments.created_at < ?", options[:to]) if options[:to]
                                        deployments.success.count
                                      end
+            end
             # rubocop: enable CodeReuse/ActiveRecord
           end
         end

ee/lib/gitlab/analytics/cycle_analytics/summary/group/issue.rb

@@ -19,13 +19,13 @@ module Gitlab
             end
 
             def value
-              @value ||= find_issues
+              @value ||= ::Gitlab::CycleAnalytics::Summary::Value::PrettyNumeric.new(issues_count)
             end
 
             private
 
             # rubocop: disable CodeReuse/ActiveRecord
-            def find_issues
+            def issues_count
               issues = IssuesFinder.new(current_user, finder_params).execute
               issues = issues.where(projects: { id: options[:projects] }) if options[:projects]
               issues.count

ee/lib/gitlab/analytics/cycle_analytics/summary/group/stage_summary.rb

@@ -41,7 +41,7 @@ module Gitlab
             def deployment_frequency_stats
               serialize(
                 Summary::Group::DeploymentFrequency.new(
-                  deployments: deployments_summary.value,
+                  deployments: deployments_summary.value.raw_value,
                   group: group,
                   options: options),
                   with_unit: true

lib/gitlab/cycle_analytics/stage_summary.rb

@@ -28,8 +28,7 @@ module Gitlab
       end
 
       def deployments_summary
-        @deployments_summary ||=
-          Summary::Deploy.new(project: @project, from: @from, to: @to)
+        @deployments_summary ||= Summary::Deploy.new(project: @project, from: @from, to: @to)
       end
 
       def deploy_stats
@@ -39,7 +38,7 @@ module Gitlab
       def deployment_frequency_stats
         serialize(
           Summary::DeploymentFrequency.new(
-            deployments: deployments_summary.value,
+            deployments: deployments_summary.value.raw_value,
             from: @from,
             to: @to),
           with_unit: true

lib/gitlab/cycle_analytics/summary/commit.rb

@@ -9,7 +9,7 @@ module Gitlab
         end
 
         def value
-          @value ||= count_commits
+          @value ||= commits_count ? Value::PrettyNumeric.new(commits_count) : Value::None.new
         end
 
         private
@@ -18,10 +18,10 @@ module Gitlab
         # a limit. Since we need a commit count, we _can't_ enforce a limit, so
         # the easiest way forward is to replicate the relevant portions of the
         # `log` function here.
-        def count_commits
+        def commits_count
           return unless ref
 
-          gitaly_commit_client.commit_count(ref, after: @from, before: @to)
+          @commits_count ||= gitaly_commit_client.commit_count(ref, after: @from, before: @to)
         end
 
         def gitaly_commit_client

lib/gitlab/cycle_analytics/summary/deploy.rb

@@ -4,14 +4,17 @@ module Gitlab
   module CycleAnalytics
     module Summary
       class Deploy < Base
-        include Gitlab::Utils::StrongMemoize
-
         def title
           n_('Deploy', 'Deploys', value)
         end
 
         def value
-          strong_memoize(:value) do
+          @value ||= Value::PrettyNumeric.new(deployments_count)
+        end
+
+        private
+
+        def deployments_count
           query = @project.deployments.success.where("created_at >= ?", @from)
           query = query.where("created_at <= ?", @to) if @to
           query.count
@@ -19,5 +22,4 @@ module Gitlab
       end
     end
   end
-  end
 end

lib/gitlab/cycle_analytics/summary/deployment_frequency.rb

@@ -17,8 +17,7 @@ module Gitlab
         end
 
         def value
-          @value ||=
-            frequency(@deployments, @from, @to || Time.now)
+          @value ||= frequency(@deployments, @from, @to || Time.now)
         end
 
         def unit

lib/gitlab/cycle_analytics/summary/issue.rb

@@ -16,7 +16,16 @@ module Gitlab
         end
 
         def value
-          @value ||= IssuesFinder.new(@current_user, project_id: @project.id, created_after: @from, created_before: @to).execute.count
+          @value ||= Value::PrettyNumeric.new(issues_count)
+        end
+
+        private
+
+        def issues_count
+          IssuesFinder
+            .new(@current_user, project_id: @project.id, created_after: @from, created_before: @to)
+            .execute
+            .count
         end
       end
     end

lib/gitlab/cycle_analytics/summary/value.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module CycleAnalytics
+    module Summary
+      class Value
+        attr_reader :value
+
+        def raw_value
+          value
+        end
+
+        def to_s
+          raise NotImplementedError
+        end
+
+        class None < self
+          def to_s
+            '-'
+          end
+        end
+
+        class Numeric < self
+          def initialize(value)
+            @value = value
+          end
+
+          def to_s
+            value.zero? ? '0' : value.to_s
+          end
+        end
+
+        class PrettyNumeric < Numeric
+          def to_s
+            # 0 is shown as -
+            value.nonzero? ? super : None.new.to_s
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/cycle_analytics/summary_helper.rb

@@ -4,10 +4,11 @@ module Gitlab
   module CycleAnalytics
     module SummaryHelper
       def frequency(count, from, to)
-        return count if count.zero?
+        return Summary::Value::None.new if count.zero?
 
         freq = (count / days(from, to)).round(1)
-        freq.zero? ? '0' : freq
+
+        Summary::Value::Numeric.new(freq)
       end
 
       def days(from, to)

spec/lib/gitlab/cycle_analytics/summary/value_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::CycleAnalytics::Summary::Value do
+  describe Gitlab::CycleAnalytics::Summary::Value::None do
+    it 'returns `-`' do
+      expect(described_class.new.to_s).to eq('-')
+    end
+  end
+
+  describe Gitlab::CycleAnalytics::Summary::Value::Numeric do
+    it 'returns the string representation of the number' do
+      expect(described_class.new(3.2).to_s).to eq('3.2')
+    end
+  end
+
+  describe Gitlab::CycleAnalytics::Summary::Value::PrettyNumeric do
+    describe '#to_s' do
+      it 'returns `-` when the number is 0' do
+        expect(described_class.new(0).to_s).to eq('-')
+      end
+
+      it 'returns the string representation of the number' do
+        expect(described_class.new(100).to_s).to eq('100')
+      end
+    end
+  end
+end