Use a single color for the Insights time series bar charts

Signed-off-by: Rémy Coutable <remy@rymai.me>

doc/user/project/insights/index.md

@@ -129,7 +129,8 @@ Supported values are:
 
 | Name  | Example |
 | ----- | ------- |
-| `bar` | ![Insights example stacked bar chart](img/insights_example_bar_chart.png) |
+| `bar` | ![Insights example bar chart](img/insights_example_bar_chart.png) |
+| `bar` (time series, i.e. when `group_by` is used) | ![Insights example bar time series chart](img/insights_example_bar_time_series_chart.png) |
 | `line` | ![Insights example stacked bar chart](img/insights_example_line_chart.png) |
 | `stacked-bar` | ![Insights example stacked bar chart](img/insights_example_stacked_bar_chart.png) |
 

ee/app/controllers/concerns/insights_actions.rb

@@ -8,7 +8,8 @@ module InsightsActions
     before_action :validate_params, only: [:query]
 
     rescue_from Gitlab::Insights::Validators::ParamsValidator::ParamsValidatorError,
-      Gitlab::Insights::Finders::IssuableFinder::IssuableFinderError, with: :render_insights_chart_error
+      Gitlab::Insights::Finders::IssuableFinder::IssuableFinderError,
+      Gitlab::Insights::Reducers::BaseReducer::BaseReducerError, with: :render_insights_chart_error
   end
 
   def show
@@ -38,38 +39,57 @@ module InsightsActions
     Gitlab::Insights::Validators::ParamsValidator.new(params).validate!
   end
 
+  def chart_type_param
+    @chart_type_param ||= params[:chart_type]
+  end
+
+  def query_param
+    @query_param ||= params[:query]
+  end
+
+  def period_param
+    @period_param ||= query_param[:group_by]
+  end
+
+  def collection_labels_param
+    @collection_labels_param ||= query_param[:collection_labels]
+  end
+
   def insights_json
-    issuables_finder = finder(params[:query])
-    issuables = issuables_finder.find
-    insights = reduce(
-      issuables: issuables,
-      chart_type: params[:chart_type],
-      period: params[:query][:group_by],
-      period_limit: issuables_finder.period_limit,
-      labels: params[:query][:collection_labels])
-    serializer(params[:chart_type]).present(insights)
-  end
-
-  def reduce(issuables:, chart_type:, period:, period_limit:, labels: nil)
-    case chart_type
+    issuables = finder.find
+    insights = reduce(issuables: issuables, period_limit: finder.period_limit)
+    serializer.present(insights)
+  end
+
+  def reduce(issuables:, period_limit: nil)
+    case chart_type_param
     when 'stacked-bar', 'line'
-      Gitlab::Insights::Reducers::LabelCountPerPeriodReducer.reduce(issuables, period: period, period_limit: period_limit, labels: labels)
+      Gitlab::Insights::Reducers::LabelCountPerPeriodReducer.reduce(issuables, period: period_param, period_limit: period_limit, labels: collection_labels_param)
     when 'bar'
-      Gitlab::Insights::Reducers::CountPerPeriodReducer.reduce(issuables, period: period, period_limit: period_limit)
+      if period_param
+        Gitlab::Insights::Reducers::CountPerPeriodReducer.reduce(issuables, period: period_param, period_limit: period_limit)
+      else
+        Gitlab::Insights::Reducers::CountPerLabelReducer.reduce(issuables, labels: collection_labels_param)
+      end
     end
   end
 
-  def finder(query)
+  def finder
+    @finder ||=
       Gitlab::Insights::Finders::IssuableFinder
-      .new(insights_entity, current_user, query)
+        .new(insights_entity, current_user, query_param)
   end
 
-  def serializer(chart_type)
-    case chart_type
+  def serializer
+    case chart_type_param
     when 'stacked-bar'
       Gitlab::Insights::Serializers::Chartjs::MultiSeriesSerializer
     when 'bar'
+      if period_param
+        Gitlab::Insights::Serializers::Chartjs::BarTimeSeriesSerializer
+      else
         Gitlab::Insights::Serializers::Chartjs::BarSerializer
+      end
     when 'line'
       Gitlab::Insights::Serializers::Chartjs::LineSerializer
     end

ee/changelogs/unreleased/11186-default-bar-colours-for-insights.yml

+---
+title: Use a single color for the Insights time series bar charts
+merge_request: 11076
+author:
+type: changed

ee/lib/gitlab/insights/finders/issuable_finder.rb

@@ -89,7 +89,7 @@ module Gitlab
 
         def period
           @period ||=
-            begin
+            if opts.key?(:group_by)
               period = opts[:group_by].to_s.pluralize.to_sym
 
               unless PERIODS.key?(period)
@@ -97,6 +97,8 @@ module Gitlab
               end
 
               period
+            else
+              :days
             end
         end
       end

ee/lib/gitlab/insights/serializers/chartjs/bar_time_series_serializer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Insights
+    module Serializers
+      module Chartjs
+        class BarTimeSeriesSerializer < Chartjs::BarSerializer
+          private
+
+          # series_data - A hash of the form `Hash[Symbol|String, Integer]`, e.g.
+          #   {
+          #     "January 2019": 1,
+          #     "February 2019": 2
+          #   }
+          #
+          # Returns a datasets array, e.g.
+          #   [{ label: nil, data: [1, 2], borderColor: ['#428bca', '#ffd8b1'] }]
+          def chart_datasets(series_data)
+            background_colors = Array.new(series_data.size - 1, Gitlab::Insights::DEFAULT_COLOR)
+            background_colors << Gitlab::Insights::COLOR_SCHEME[:apricot]
+
+            [dataset(nil, series_data.values, background_colors)]
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/factories/insights.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :insights_issuables, class: Hash do
+  factory :insights_issues_by_team, class: Hash do
     initialize_with do
       {
         Manage: 1,
@@ -12,7 +12,17 @@ FactoryBot.define do
     end
   end
 
-  factory :insights_issuables_per_month, class: Hash do
+  factory :insights_merge_requests_per_month, class: Hash do
+    initialize_with do
+      {
+        'January 2019' => 1,
+        'February 2019' => 2,
+        'March 2019' => 3
+      }
+    end
+  end
+
+  factory :insights_issues_by_team_per_month, class: Hash do
     initialize_with do
       {
         'January 2019' => {

ee/spec/lib/gitlab/insights/finders/issuable_finder_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
     end
 
     it 'raises an error for an invalid :issuable_type option' do
-      expect { find(nil, issuable_type: 'foo') }.to raise_error(described_class::InvalidIssuableTypeError, "Invalid `:issuable_type` option: `foo`. Allowed values are #{described_class::FINDERS.keys}!")
+      expect { find(build(:project), issuable_type: 'foo') }.to raise_error(described_class::InvalidIssuableTypeError, "Invalid `:issuable_type` option: `foo`. Allowed values are #{described_class::FINDERS.keys}!")
     end
 
     it 'raises an error for an invalid entity object' do
@@ -28,11 +28,15 @@ RSpec.describe Gitlab::Insights::Finders::IssuableFinder do
     end
 
     it 'raises an error for an invalid :group_by option' do
-      expect { find(nil, issuable_type: 'issue', group_by: 'foo') }.to raise_error(described_class::InvalidGroupByError, "Invalid `:group_by` option: `foo`. Allowed values are #{described_class::PERIODS.keys}!")
+      expect { find(build(:project), issuable_type: 'issue', group_by: 'foo') }.to raise_error(described_class::InvalidGroupByError, "Invalid `:group_by` option: `foo`. Allowed values are #{described_class::PERIODS.keys}!")
+    end
+
+    it 'defaults to the "days" period if no :group_by is given' do
+      expect(described_class.new(build(:project), nil, issuable_type: 'issue').__send__(:period)).to eq(:days)
     end
 
     it 'raises an error for an invalid :period_limit option' do
-      expect { find(build(:user), issuable_type: 'issue', group_by: 'months', period_limit: 'many') }.to raise_error(described_class::InvalidPeriodLimitError, "Invalid `:period_limit` option: `many`. Expected an integer!")
+      expect { find(build(:project), issuable_type: 'issue', group_by: 'months', period_limit: 'many') }.to raise_error(described_class::InvalidPeriodLimitError, "Invalid `:period_limit` option: `many`. Expected an integer!")
     end
 
     shared_examples_for "insights issuable finder" do

ee/spec/lib/gitlab/insights/serializers/chartjs/bar_serializer_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Insights::Serializers::Chartjs::BarSerializer do
   it 'returns the correct format' do
-    input = build(:insights_issuables)
+    input = build(:insights_issues_by_team)
     expected = {
       labels: %w[Manage Plan Create undefined],
       datasets: [

ee/spec/lib/gitlab/insights/serializers/chartjs/bar_time_series_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Insights::Serializers::Chartjs::BarTimeSeriesSerializer do
+  it 'returns the correct format' do
+    input = build(:insights_merge_requests_per_month)
+    expected = {
+      labels: ['January 2019', 'February 2019', 'March 2019'],
+      datasets: [
+        {
+          label: nil,
+          data: [1, 2, 3],
+          backgroundColor: [Gitlab::Insights::DEFAULT_COLOR, Gitlab::Insights::DEFAULT_COLOR, Gitlab::Insights::COLOR_SCHEME[:apricot]]
+        }
+      ]
+    }.with_indifferent_access
+
+    expect(described_class.present(input)).to eq(expected)
+  end
+end

ee/spec/lib/gitlab/insights/serializers/chartjs/line_serializer_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Insights::Serializers::Chartjs::LineSerializer do
-  let(:input) { build(:insights_issuables_per_month) }
+  let(:input) { build(:insights_issues_by_team_per_month) }
 
   subject { described_class.present(input) }
 

ee/spec/lib/gitlab/insights/serializers/chartjs/multi_series_serializer_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Insights::Serializers::Chartjs::MultiSeriesSerializer do
   it 'returns the correct format' do
-    input = build(:insights_issuables_per_month)
+    input = build(:insights_issues_by_team_per_month)
     expected = {
       labels: ['January 2019', 'February 2019', 'March 2019'],
       datasets: [