Hide non-numeric metrics

- This adds a sanity check to avoid
null values from being passed to n__

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

@@ -22,8 +22,8 @@ export default {
     },
   },
   computed: {
-    isNumericValue() {
-      return this.value !== null && !Number.isNaN(Number(this.value));
+    isNumber() {
+      return typeof this.value === 'number';
     },
     unit() {
       return this.type === 'days_to_merge'
@@ -36,7 +36,7 @@ export default {
 <template>
   <div class="metric-col">
     <span class="time">
-      <template v-if="isNumericValue">
+      <template v-if="isNumber">
         {{ value }}
         <span> {{ unit }} </span>
       </template>

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

@@ -39,10 +39,10 @@ export default {
     selectedMetric() {
       return this.mergeRequest[this.metricType];
     },
-    metricTimeUnit() {
-      return this.metricType === 'days_to_merge'
-        ? n__('day', 'days', this.selectedMetric)
-        : n__('Time|hr', 'Time|hrs', this.selectedMetric);
+  },
+  methods: {
+    isNumber(metric) {
+      return typeof metric === 'number';
     },
   },
 };
@@ -50,26 +50,30 @@ export default {
 <template>
   <div class="gl-responsive-table-row-layout gl-responsive-table-row">
     <div
-      class="table-section section-50 d-flex flex-row-reverse flex-md-row justify-content-between justify-content-md-start qa-mr-details"
+      class="table-section section-50 d-flex flex-row-reverse flex-md-row justify-content-between justify-content-md-start js-mr-details"
     >
       <div class="d-flex mr-md-2">
         <gl-avatar :src="mergeRequest.author_avatar_url" :size="16" />
       </div>
-      <div class="mw-90p">
-        <h5 class="item-title mr-title my-0 mw-90p d-block str-truncated">
+      <div class="d-flex flex-column flex-grow mw-90p">
+        <h5 class="item-title mr-title my-0 d-block str-truncated">
           <gl-link :href="mergeRequest.merge_request_url" target="_blank">{{
             mergeRequest.title
           }}</gl-link>
         </h5>
-        <ul class="horizontal-list list-items-separated text-secondary">
+        <ul class="horizontal-list list-items-separated text-secondary mb-0">
           <li>{{ mrId }}</li>
-          <li>{{ commitCount }}</li>
-          <li>{{ locPerCommit }}</li>
-          <li>{{ filesTouched }}</li>
+          <li v-if="isNumber(mergeRequest.commits_count)" ref="commitCount">{{ commitCount }}</li>
+          <li v-if="isNumber(mergeRequest.loc_per_commit)" ref="locPerCommitCount">
+            {{ locPerCommit }}
+          </li>
+          <li v-if="isNumber(mergeRequest.files_touched)" ref="filesTouchedCount">
+            {{ filesTouched }}
+          </li>
         </ul>
       </div>
     </div>
-    <div class="table-section section-50 d-flex flex-row align-items-start qa-mr-metrics">
+    <div class="table-section section-50 d-flex flex-row align-items-start js-mr-metrics">
       <metric-column
         type="days_to_merge"
         :value="mergeRequest.days_to_merge"

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

@@ -5,7 +5,7 @@ exports[`MergeRequestTableRow component on creation matches the snapshot 1`] = `
   class="gl-responsive-table-row-layout gl-responsive-table-row"
 >
   <div
-    class="table-section section-50 d-flex flex-row-reverse flex-md-row justify-content-between justify-content-md-start qa-mr-details"
+    class="table-section section-50 d-flex flex-row-reverse flex-md-row justify-content-between justify-content-md-start js-mr-details"
   >
     <div
       class="d-flex mr-md-2"
@@ -20,10 +20,10 @@ exports[`MergeRequestTableRow component on creation matches the snapshot 1`] = `
     </div>
      
     <div
-      class="mw-90p"
+      class="d-flex flex-column flex-grow mw-90p"
     >
       <h5
-        class="item-title mr-title my-0 mw-90p d-block str-truncated"
+        class="item-title mr-title my-0 d-block str-truncated"
       >
         <gllink-stub
           href="http://gitlab.example.com/gitlab-org/gitlab-test/merge_requests/10"
@@ -34,7 +34,7 @@ exports[`MergeRequestTableRow component on creation matches the snapshot 1`] = `
       </h5>
        
       <ul
-        class="horizontal-list list-items-separated text-secondary"
+        class="horizontal-list list-items-separated text-secondary mb-0"
       >
         <li>
           !10
@@ -45,18 +45,22 @@ exports[`MergeRequestTableRow component on creation matches the snapshot 1`] = `
         </li>
          
         <li>
+          
           3 LOC/commit
+        
         </li>
          
         <li>
+          
           1 files touched
+        
         </li>
       </ul>
     </div>
   </div>
    
   <div
-    class="table-section section-50 d-flex flex-row align-items-start qa-mr-metrics"
+    class="table-section section-50 d-flex flex-row align-items-start js-mr-metrics"
   >
     <metriccolumn-stub
       label="Time to merge"

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

@@ -64,21 +64,6 @@ describe('MetricColumn component', () => {
   });
 
   describe('computed', () => {
-    describe('isNumericValue', () => {
-      it('returns true when the value is neither null nor undefined', () => {
-        factory();
-        expect(wrapper.vm.isNumericValue).toBe(true);
-      });
-
-      it('returns false when the value is null', () => {
-        factory({
-          ...defaultProps,
-          value: null,
-        });
-        expect(wrapper.vm.isNumericValue).toBe(false);
-      });
-    });
-
     describe('unit', () => {
       it('returns "days" for the "days_to_merge" metric', () => {
         factory({

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

@@ -23,8 +23,8 @@ describe('MergeRequestTableRow component', () => {
     });
   };
 
-  const findMrDetails = () => wrapper.find('.qa-mr-details');
-  const findMrMetrics = () => wrapper.find('.qa-mr-metrics');
+  const findMrDetails = () => wrapper.find('.js-mr-details');
+  const findMrMetrics = () => wrapper.find('.js-mr-metrics');
   const findMetricColumns = () => findMrMetrics().findAll(MetricColumn);
 
   afterEach(() => {
@@ -60,6 +60,26 @@ describe('MergeRequestTableRow component', () => {
       expect(title.text()).toContain(defaultProps.mergeRequest.title);
     });
 
+    describe('metric list', () => {
+      it.each`
+        metric              | selector
+        ${'commits_count'}  | ${'commitCount'}
+        ${'loc_per_commit'} | ${'locPerCommitCount'}
+        ${'files_touched'}  | ${'filesTouchedCount'}
+      `("metric '$metric' won't be rendered if null", ({ metric, selector }) => {
+        // let's update our test data and set the metric to null
+        const props = {
+          ...defaultProps,
+          mergeRequest: {
+            ...defaultProps.mergeRequest,
+            [metric]: null,
+          },
+        };
+        factory(props);
+        expect(wrapper.find({ ref: selector }).exists()).toBe(false);
+      });
+    });
+
     describe('metric columns', () => {
       it('renders two metric columns', () => {
         expect(findMetricColumns().length).toBe(2);
@@ -129,33 +149,5 @@ describe('MergeRequestTableRow component', () => {
         expect(wrapper.vm.selectedMetric).toBe(defaultProps.mergeRequest[defaultProps.metricType]);
       });
     });
-
-    describe('metricTimeUnit', () => {
-      describe('when metricType is "days_to_merge"', () => {
-        beforeEach(() => {
-          factory({
-            ...defaultProps,
-            metricType: 'days_to_merge',
-          });
-        });
-
-        it('returns "days"', () => {
-          expect(wrapper.vm.metricTimeUnit).toBe('days');
-        });
-      });
-
-      describe('when metricType is not "days_to_merge"', () => {
-        beforeEach(() => {
-          factory({
-            ...defaultProps,
-            metricType: 'time_to_last_commit',
-          });
-        });
-
-        it('returns "hrs"', () => {
-          expect(wrapper.vm.metricTimeUnit).toBe('hrs');
-        });
-      });
-    });
   });
 });