Merge branch 'mw-productivity-analytics-scatterplot-axis-label' into 'master'

Update axis label on scatterplot on metric change

See merge request gitlab-org/gitlab!17599

ee/app/assets/javascripts/analytics/productivity_analytics/components/app.vue

@@ -61,8 +61,9 @@ export default {
       'getColumnChartDatazoomOption',
       'getScatterPlotMainData',
       'getScatterPlotMedianData',
-      'getMetricDropdownLabel',
+      'getMetricLabel',
       'getSelectedMetric',
+      'scatterplotYaxisLabel',
       'hasNoAccessError',
     ]),
     ...mapGetters('table', [
@@ -216,7 +217,7 @@ export default {
                 :data="{ full: getColumnChartData(chartKeys.commitBasedHistogram) }"
                 :option="getColumnChartOption(chartKeys.commitBasedHistogram)"
                 :y-axis-title="s__('ProductivityAanalytics|Merge requests')"
-                :x-axis-title="getMetricDropdownLabel(chartKeys.commitBasedHistogram)"
+                :x-axis-title="getMetricLabel(chartKeys.commitBasedHistogram)"
                 x-axis-type="category"
               />
             </metric-chart>
@@ -236,7 +237,7 @@ export default {
           >
             <scatterplot
               :x-axis-title="s__('ProductivityAnalytics|Merge date')"
-              :y-axis-title="s__('ProductivityAnalytics|Days')"
+              :y-axis-title="scatterplotYaxisLabel"
               :scatter-data="getScatterPlotMainData"
               :median-line-data="getScatterPlotMedianData"
             />

ee/app/assets/javascripts/analytics/productivity_analytics/store/modules/charts/getters.js

 import _ from 'underscore';
+import { s__ } from '~/locale';
 import httpStatus from '~/lib/utils/http_status';
 import { getDateInPast } from '~/lib/utils/datetime_utility';
 import {
@@ -9,6 +10,7 @@ import {
   maxColumnChartItemsPerPage,
   dataZoomOptions,
   scatterPlotAddonQueryDays,
+  daysToMergeMetric,
 } from '../../../constants';
 import { getScatterPlotData, getMedianLineData } from '../../../utils';
 
@@ -91,7 +93,7 @@ export const getScatterPlotMedianData = (state, getters) =>
     scatterPlotAddonQueryDays,
   );
 
-export const getMetricDropdownLabel = state => chartKey =>
+export const getMetricLabel = state => chartKey =>
   metricTypes.find(m => m.key === state.charts[chartKey].params.metricType).label;
 
 export const getFilterParams = (state, getters, rootState, rootGetters) => chartKey => {
@@ -150,6 +152,20 @@ export const getColumnChartDatazoomOption = state => chartKey => {
 
 export const getSelectedMetric = state => chartKey => state.charts[chartKey].params.metricType;
 
+/**
+ * Returns the y axis label for the scatterplot.
+ * This can either be "Days", "Hours" or some other metric label from the state's metricTypes.
+ */
+export const scatterplotYaxisLabel = (_state, getters, rootState) => {
+  const selectedMetric = getters.getSelectedMetric(chartKeys.scatterplot);
+  const metricTypesInHours = rootState.metricTypes
+    .filter(metric => metric.charts.indexOf(chartKeys.timeBasedHistogram) !== -1)
+    .map(metric => metric.key);
+  if (selectedMetric === daysToMergeMetric.key) return s__('ProductivityAnalytics|Days');
+  if (metricTypesInHours.indexOf(selectedMetric) !== -1) return s__('ProductivityAnalytics|Hours');
+  return getters.getMetricLabel(chartKeys.scatterplot);
+};
+
 export const hasNoAccessError = state =>
   state.charts[chartKeys.main].errorCode === httpStatus.FORBIDDEN;
 

ee/spec/frontend/analytics/productivity_analytics/components/app_spec.js

@@ -3,6 +3,7 @@ import Vuex from 'vuex';
 import axios from 'axios';
 import MockAdapter from 'axios-mock-adapter';
 import ProductivityApp from 'ee/analytics/productivity_analytics/components/app.vue';
+import Scatterplot from 'ee/analytics/shared/components/scatterplot.vue';
 import MergeRequestTable from 'ee/analytics/productivity_analytics/components/mr_table.vue';
 import store from 'ee/analytics/productivity_analytics/store';
 import { chartKeys } from 'ee/analytics/productivity_analytics/constants';
@@ -24,7 +25,6 @@ describe('ProductivityApp component', () => {
   };
 
   const actionSpies = {
-    setMetricType: jest.fn(),
     chartItemClicked: jest.fn(),
     setSortField: jest.fn(),
     setMergeRequestsPage: jest.fn(),
@@ -192,12 +192,17 @@ describe('ProductivityApp component', () => {
                     expect(findTimeBasedMetricChart().props('chartData')).not.toEqual([]);
                   });
 
-                  it('should call setMetricType  when `metricTypeChange` is emitted on the metric chart', () => {
-                    findTimeBasedMetricChart().vm.$emit('metricTypeChange', 'time_to_merge');
+                  describe('when the user changes the metric', () => {
+                    beforeEach(() => {
+                      jest.spyOn(store, 'dispatch');
+                      findTimeBasedMetricChart().vm.$emit('metricTypeChange', 'time_to_merge');
+                    });
 
-                    expect(actionSpies.setMetricType).toHaveBeenCalledWith({
-                      metricType: 'time_to_merge',
-                      chartKey: chartKeys.timeBasedHistogram,
+                    it('should call setMetricType  when `metricTypeChange` is emitted on the metric chart', () => {
+                      expect(store.dispatch).toHaveBeenCalledWith('charts/setMetricType', {
+                        metricType: 'time_to_merge',
+                        chartKey: chartKeys.timeBasedHistogram,
+                      });
                     });
                   });
                 });
@@ -228,11 +233,12 @@ describe('ProductivityApp component', () => {
 
                   describe('when the user changes the metric', () => {
                     beforeEach(() => {
+                      jest.spyOn(store, 'dispatch');
                       findCommitBasedMetricChart().vm.$emit('metricTypeChange', 'loc_per_commit');
                     });
 
                     it('should call setMetricType  when `metricTypeChange` is emitted on the metric chart', () => {
-                      expect(actionSpies.setMetricType).toHaveBeenCalledWith({
+                      expect(store.dispatch).toHaveBeenCalledWith('charts/setMetricType', {
                         metricType: 'loc_per_commit',
                         chartKey: chartKeys.commitBasedHistogram,
                       });
@@ -240,7 +246,7 @@ describe('ProductivityApp component', () => {
 
                     it("should update the chart's x axis label", () => {
                       const columnChart = findCommitBasedMetricChart().find(GlColumnChart);
-                      expect(columnChart.props('xAxisTitle')).toBe('Number of commits per MR');
+                      expect(columnChart.props('xAxisTitle')).toBe('Number of LOCs per commit');
                     });
                   });
                 });
@@ -274,15 +280,21 @@ describe('ProductivityApp component', () => {
 
                   describe('when the user changes the metric', () => {
                     beforeEach(() => {
+                      jest.spyOn(store, 'dispatch');
                       findScatterplotMetricChart().vm.$emit('metricTypeChange', 'loc_per_commit');
                     });
 
                     it('should call setMetricType  when `metricTypeChange` is emitted on the metric chart', () => {
-                      expect(actionSpies.setMetricType).toHaveBeenCalledWith({
+                      expect(store.dispatch).toHaveBeenCalledWith('charts/setMetricType', {
                         metricType: 'loc_per_commit',
                         chartKey: chartKeys.scatterplot,
                       });
                     });
+
+                    it("should update the chart's y axis label", () => {
+                      const scatterplot = findScatterplotMetricChart().find(Scatterplot);
+                      expect(scatterplot.props('yAxisTitle')).toBe('Number of LOCs per commit');
+                    });
                   });
                 });
               });

ee/spec/frontend/analytics/productivity_analytics/store/modules/charts/getters_spec.js

 import createState from 'ee/analytics/productivity_analytics/store/modules/charts/state';
 import * as getters from 'ee/analytics/productivity_analytics/store/modules/charts/getters';
 import {
+  metricTypes,
   chartKeys,
   columnHighlightStyle,
   maxColumnChartItemsPerPage,
@@ -91,13 +92,13 @@ describe('Productivity analytics chart getters', () => {
     });
   });
 
-  describe('getMetricDropdownLabel', () => {
+  describe('getMetricLabel', () => {
     it('returns the correct label for the "time_to_last_commit" metric', () => {
       state.charts[chartKeys.timeBasedHistogram].params = {
         metricType: 'time_to_last_commit',
       };
 
-      expect(getters.getMetricDropdownLabel(state)(chartKeys.timeBasedHistogram)).toBe(
+      expect(getters.getMetricLabel(state)(chartKeys.timeBasedHistogram)).toBe(
         'Time from first comment to last commit',
       );
     });
@@ -233,6 +234,44 @@ describe('Productivity analytics chart getters', () => {
     });
   });
 
+  describe('scatterplotYaxisLabel', () => {
+    const metricsInHours = ['time_to_first_comment', 'time_to_last_commit', 'time_to_merge'];
+
+    const mockRootState = {
+      metricTypes,
+    };
+
+    it('returns "Days" for "days_to_merge" metric', () => {
+      const mockGetters = {
+        getSelectedMetric: () => 'days_to_merge',
+      };
+      expect(getters.scatterplotYaxisLabel(null, mockGetters, mockRootState)).toBe('Days');
+    });
+
+    it.each(metricsInHours)('returns "Hours" for the "%s" metric', metric => {
+      const mockGetters = {
+        getSelectedMetric: () => metric,
+      };
+      expect(getters.scatterplotYaxisLabel(null, mockGetters, mockRootState)).toBe('Hours');
+    });
+
+    it.each`
+      metric              | label
+      ${'commits_count'}  | ${'Number of commits per MR'}
+      ${'loc_per_commit'} | ${'Number of LOCs per commit'}
+      ${'files_touched'}  | ${'Number of files touched'}
+    `('calls getMetricLabel for the "$metric" metric', ({ metric }) => {
+      const mockGetters = {
+        getSelectedMetric: () => metric,
+        getMetricLabel: jest.fn(),
+      };
+
+      getters.scatterplotYaxisLabel(null, mockGetters, mockRootState);
+
+      expect(mockGetters.getMetricLabel).toHaveBeenCalled();
+    });
+  });
+
   describe('hasNoAccessError', () => {
     it('returns true if errorCode is set to 403', () => {
       state.charts[chartKeys.main].errorCode = 403;