Merge branch...

Merge branch '32068-productivity-analytics-mr-table-showing-zeros-in-time-to-merge-column' into 'master'

Resolve "Productivity Analytics: MR table showing zeros in "Time to merge" column"

Closes #32068

See merge request gitlab-org/gitlab!16843

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

@@ -11,7 +11,7 @@ import {
 import { GlColumnChart } from '@gitlab/ui/dist/charts';
 import Icon from '~/vue_shared/components/icon.vue';
 import MergeRequestTable from './mr_table.vue';
-import { chartKeys, metricTypes } from '../constants';
+import { chartKeys } from '../constants';
 
 export default {
   components: {
@@ -48,13 +48,8 @@ export default {
   },
   computed: {
     ...mapState('filters', ['groupNamespace', 'projectPath']),
-    ...mapState('table', [
-      'isLoadingTable',
-      'mergeRequests',
-      'pageInfo',
-      'sortFields',
-      'columnMetric',
-    ]),
+    ...mapState('table', ['isLoadingTable', 'mergeRequests', 'pageInfo', 'columnMetric']),
+    ...mapGetters(['getMetricTypes']),
     ...mapGetters('charts', [
       'chartLoading',
       'getChartData',
@@ -66,7 +61,7 @@ export default {
       'sortFieldDropdownLabel',
       'sortIcon',
       'sortTooltipTitle',
-      'getColumnOptions',
+      'tableSortOptions',
       'columnMetricLabel',
       'isSelectedSortField',
       'hasNoAccessError',
@@ -92,9 +87,6 @@ export default {
       const itemValue = params.data.value[0];
       this.chartItemClicked({ chartKey: this.chartKeys.main, item: itemValue });
     },
-    getMetricTypes(chartKey) {
-      return metricTypes.filter(m => m.chart === chartKey);
-    },
     getColumnChartOption(chartKey) {
       return {
         yAxis: {
@@ -262,21 +254,21 @@ export default {
               :text="sortFieldDropdownLabel"
             >
               <gl-dropdown-item
-                v-for="(value, key) in sortFields"
-                :key="key"
+                v-for="metric in tableSortOptions"
+                :key="metric.key"
                 active-class="is-active"
                 class="w-100"
-                @click="setSortField(key)"
+                @click="setSortField(metric.key)"
               >
                 <span class="d-flex">
                   <icon
                     class="flex-shrink-0 append-right-4"
                     :class="{
-                      invisible: !isSelectedSortField(key),
+                      invisible: !isSelectedSortField(metric.key),
                     }"
                     name="mobile-issue-close"
                   />
-                  {{ value }}
+                  {{ metric.label }}
                 </span>
               </gl-dropdown-item>
             </gl-dropdown>
@@ -292,7 +284,7 @@ export default {
           v-else
           :merge-requests="mergeRequests"
           :page-info="pageInfo"
-          :column-options="getColumnOptions"
+          :column-options="getMetricTypes(chartKeys.timeBasedHistogram)"
           :metric-type="columnMetric"
           :metric-label="columnMetricLabel"
           @columnMetricChange="setColumnMetric"

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

@@ -22,7 +22,7 @@ export default {
       required: true,
     },
     columnOptions: {
-      type: Object,
+      type: Array,
       required: true,
     },
     metricType: {
@@ -36,7 +36,7 @@ export default {
   },
   computed: {
     metricDropdownLabel() {
-      return this.columnOptions[this.metricType];
+      return this.columnOptions.find(option => option.key === this.metricType).label;
     },
     showPagination() {
       return this.pageInfo && this.pageInfo.total;
@@ -72,21 +72,21 @@ export default {
                 :text="metricDropdownLabel"
               >
                 <gl-dropdown-item
-                  v-for="(value, key) in columnOptions"
-                  :key="key"
+                  v-for="option in columnOptions"
+                  :key="option.key"
                   active-class="is-active"
                   class="w-100"
-                  @click="$emit('columnMetricChange', key)"
+                  @click="$emit('columnMetricChange', option.key)"
                 >
                   <span class="d-flex">
                     <icon
                       class="flex-shrink-0 append-right-4"
                       :class="{
-                        invisible: !isSelectedMetric(key),
+                        invisible: !isSelectedMetric(option.key),
                       }"
                       name="mobile-issue-close"
                     />
-                    {{ value }}
+                    {{ option.label }}
                   </span>
                 </gl-dropdown-item>
               </gl-dropdown>

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

@@ -71,8 +71,8 @@ export default {
     </div>
     <div class="table-section section-50 d-flex flex-row align-items-start qa-mr-metrics">
       <metric-column
-        type="time_to_merge"
-        :value="mergeRequest.time_to_merge"
+        type="days_to_merge"
+        :value="mergeRequest.days_to_merge"
         :label="__('Time to merge')"
       />
       <metric-column :type="metricType" :value="selectedMetric" :label="metricLabel" />

ee/app/assets/javascripts/analytics/productivity_analytics/constants.js

@@ -45,17 +45,6 @@ export const metricTypes = [
   },
 ];
 
-export const tableSortFields = metricTypes.reduce(
-  (acc, curr) => {
-    const { key, label, chart } = curr;
-    if (chart === chartKeys.timeBasedHistogram) {
-      acc[key] = label;
-    }
-    return acc;
-  },
-  { days_to_merge: __('Days to merge') },
-);
-
 export const tableSortOrder = {
   asc: {
     title: s__('ProductivityAnalytics|Ascending'),
@@ -69,7 +58,10 @@ export const tableSortOrder = {
   },
 };
 
-export const timeToMergeMetric = 'time_to_merge';
+export const daysToMergeMetric = {
+  key: 'days_to_merge',
+  label: s__('ProductivityAnalytics|Days to merge'),
+};
 
 export const defaultMaxColumnChartItemsPerPage = 20;
 

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

+export const getMetricTypes = state => chartKey =>
+  state.metricTypes.filter(m => m.chart === chartKey);
+
+// prevent babel-plugin-rewire from generating an invalid default during karma tests
+export default () => {};

ee/app/assets/javascripts/analytics/productivity_analytics/store/index.js

 import Vue from 'vue';
 import Vuex from 'vuex';
 import state from './state';
+import * as getters from './getters';
 import * as actions from './actions';
 import mutations from './mutations';
 import filters from './modules/filters/index';
@@ -12,6 +13,7 @@ Vue.use(Vuex);
 const createStore = () =>
   new Vuex.Store({
     state: state(),
+    getters,
     actions,
     mutations,
     modules: {

ee/app/assets/javascripts/analytics/productivity_analytics/store/modules/table/actions.js

 import axios from '~/lib/utils/axios_utils';
 import * as types from './mutation_types';
 import { parseIntPagination, normalizeHeaders } from '~/lib/utils/common_utils';
-import { timeToMergeMetric } from '../../../constants';
+import { daysToMergeMetric } from '../../../constants';
 
 export const fetchMergeRequests = ({ dispatch, state, rootState, rootGetters }) => {
   dispatch('requestMergeRequests');
@@ -41,8 +41,8 @@ export const receiveMergeRequestsError = ({ commit }, { response }) => {
 export const setSortField = ({ commit, dispatch }, data) => {
   commit(types.SET_SORT_FIELD, data);
 
-  // let's make sure we update the column that we sort on (except for 'time_to_merge')
-  if (data !== timeToMergeMetric) {
+  // let's make sure we update the column that we sort on (except for 'days_to_merge')
+  if (data !== daysToMergeMetric.key) {
     dispatch('setColumnMetric', data);
   }
 

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

 import httpStatus from '~/lib/utils/http_status';
-import { tableSortOrder } from './../../../constants';
+import { chartKeys, tableSortOrder, daysToMergeMetric } from '../../../constants';
 
 export const sortIcon = state => tableSortOrder[state.sortOrder].icon;
 
 export const sortTooltipTitle = state => tableSortOrder[state.sortOrder].title;
 
-export const sortFieldDropdownLabel = state => state.sortFields[state.sortField];
+export const sortFieldDropdownLabel = (state, _, rootState) =>
+  rootState.metricTypes.find(metric => metric.key === state.sortField).label;
 
-export const getColumnOptions = state =>
-  Object.keys(state.sortFields)
-    .filter(key => key !== 'time_to_merge')
-    .reduce((obj, key) => {
-      const result = { ...obj, [key]: state.sortFields[key] };
-      return result;
-    }, {});
+export const tableSortOptions = (_state, _getters, _rootState, rootGetters) => [
+  daysToMergeMetric,
+  ...rootGetters.getMetricTypes(chartKeys.timeBasedHistogram),
+];
 
-export const columnMetricLabel = (state, getters) => getters.getColumnOptions[state.columnMetric];
+export const columnMetricLabel = (state, _getters, _rootState, rootGetters) =>
+  rootGetters
+    .getMetricTypes(chartKeys.timeBasedHistogram)
+    .find(metric => metric.key === state.columnMetric).label;
 
 export const isSelectedSortField = state => sortField => state.sortField === sortField;
 

ee/app/assets/javascripts/analytics/productivity_analytics/store/modules/table/state.js

-import { tableSortFields, tableSortOrder } from './../../../constants';
+import { tableSortOrder } from '../../../constants';
 
 export default () => ({
   isLoadingTable: false,
@@ -6,7 +6,6 @@ export default () => ({
   mergeRequests: [],
   pageInfo: {},
   sortOrder: tableSortOrder.asc.value,
-  sortFields: tableSortFields,
   sortField: 'time_to_merge',
   columnMetric: 'time_to_first_comment',
 });

ee/app/assets/javascripts/analytics/productivity_analytics/store/state.js

+import { metricTypes } from '../constants';
+
 export default () => ({
   endpoint: null,
+  metricTypes,
 });

ee/spec/frontend/analytics/productivity_analytics/components/__snapshots__/mr_table_row_spec.js.snap

@@ -60,8 +60,8 @@ exports[`MergeRequestTableRow component on creation matches the snapshot 1`] = `
   >
     <metriccolumn-stub
       label="Time to merge"
-      type="time_to_merge"
-      value="0"
+      type="days_to_merge"
+      value="24"
     />
      
     <metriccolumn-stub

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

@@ -65,12 +65,12 @@ describe('MergeRequestTableRow component', () => {
         expect(findMetricColumns().length).toBe(2);
       });
 
-      it('renders the "Time to merge" metric column', () => {
+      it('renders the "Time to merge" metric column with the "days_to_merge" metric', () => {
         expect(
           findMetricColumns()
             .at(0)
             .props('value'),
-        ).toBe(defaultProps.mergeRequest.time_to_merge);
+        ).toBe(defaultProps.mergeRequest.days_to_merge);
       });
     });
   });

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

@@ -9,11 +9,11 @@ describe('MergeRequestTable component', () => {
 
   const defaultProps = {
     mergeRequests: mockMergeRequests,
-    columnOptions: {
-      time_to_first_comment: 'Time from first commit until first comment',
-      time_to_last_commit: 'Time from first comment to last commit',
-      time_to_merge: 'Time from last commit to merge',
-    },
+    columnOptions: [
+      { key: 'time_to_first_comment', label: 'Time from first commit until first comment' },
+      { key: 'time_to_last_commit', label: 'Time from first comment to last commit' },
+      { key: 'time_to_merge', label: 'Time from last commit to merge' },
+    ],
     metricType: 'time_to_last_commit',
     metricLabel: 'Time from first comment to last commit',
     pageInfo: {},

ee/spec/frontend/analytics/productivity_analytics/store/modules/table/actions_spec.js

@@ -197,12 +197,12 @@ describe('Productivity analytics table actions', () => {
         done,
       ));
 
-    it('should not dispatch setColumnMetric when metric is "time_to_merge"', done =>
+    it('should not dispatch setColumnMetric when metric is "days_to_merge"', done =>
       testAction(
         actions.setSortField,
-        'time_to_merge',
+        'days_to_merge',
         mockedContext.state,
-        [{ type: types.SET_SORT_FIELD, payload: 'time_to_merge' }],
+        [{ type: types.SET_SORT_FIELD, payload: 'days_to_merge' }],
         [{ type: 'fetchMergeRequests' }],
         done,
       ));

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

@@ -5,6 +5,12 @@ import { tableSortOrder } from 'ee/analytics/productivity_analytics/constants';
 describe('Productivity analytics table getters', () => {
   let state;
 
+  const metricTypes = [
+    { key: 'time_to_first_comment', label: 'Time from first commit until first comment' },
+    { key: 'time_to_last_commit', label: 'Time from first comment to last commit' },
+    { key: 'time_to_merge', label: 'Time from last commit to merge' },
+  ];
+
   beforeEach(() => {
     state = createState();
   });
@@ -47,26 +53,27 @@ describe('Productivity analytics table getters', () => {
 
   describe('sortFieldDropdownLabel', () => {
     it('returns the correct label for the current sort field', () => {
+      const rootState = {
+        metricTypes,
+      };
+
       state.sortField = 'time_to_last_commit';
 
-      expect(getters.sortFieldDropdownLabel(state)).toBe('Time from first comment to last commit');
+      expect(getters.sortFieldDropdownLabel(state, null, rootState)).toBe(
+        'Time from first comment to last commit',
+      );
     });
   });
 
-  describe('getColumnOptions', () => {
-    it('returns an object of key/value pairs with the available column options', () => {
-      state.sortFields = {
-        time_to_first_comment: 'Time from first commit until first comment',
-        time_to_last_commit: 'Time from first comment to last commit',
-        time_to_merge: 'Time from last commit to merge',
-        days_to_merge: 'Days to merge',
+  describe('tableSortOptions', () => {
+    it('returns the metricTypes from the timeBasedHistogram and adds "Days to merge"', () => {
+      const rootGetters = {
+        getMetricTypes: () => metricTypes,
       };
 
-      expect(getters.getColumnOptions(state)).toEqual({
-        days_to_merge: 'Days to merge',
-        time_to_first_comment: 'Time from first commit until first comment',
-        time_to_last_commit: 'Time from first comment to last commit',
-      });
+      const expected = [{ key: 'days_to_merge', label: 'Days to merge' }, ...metricTypes];
+
+      expect(getters.tableSortOptions(null, null, null, rootGetters)).toEqual(expected);
     });
   });
 

locale/gitlab.pot

@@ -4656,9 +4656,6 @@ msgstr ""
 msgid "Days"
 msgstr ""
 
-msgid "Days to merge"
-msgstr ""
-
 msgid "Debug"
 msgstr ""
 
@@ -11467,6 +11464,9 @@ msgstr ""
 msgid "ProductivityAnalytics|Ascending"
 msgstr ""
 
+msgid "ProductivityAnalytics|Days to merge"
+msgstr ""
+
 msgid "ProductivityAnalytics|Descending"
 msgstr ""