Improve codeclimate UI

app/assets/stylesheets/pages/merge_requests.scss

@@ -777,7 +777,9 @@
 }
 
 .mr-widget-code-quality {
-  padding-top: $gl-padding-top;
+  .ci-status-icon-warning svg {
+    fill: $theme-gray-600;
+  }
 
   .code-quality-container {
     border-top: 1px solid $gray-darker;
@@ -792,13 +794,30 @@
       margin: 0;
       line-height: $code_line_height;
 
+      .mr-widget-code-quality-icon svg {
+        width: 10px;
+        height: 10px;
+      }
+
       li.success {
+        .mr-widget-code-quality-title {
           color: $green-500;
         }
 
+        .mr-widget-code-quality-icon svg {
+          fill: $green-500;
+        }
+      }
+
       li.failed {
+        .mr-widget-code-quality-title {
           color: $red-500;
         }
+
+        .mr-widget-code-quality-icon svg {
+          fill: $red-500;
+        }
+      }
     }
   }
 }

changelogs/unreleased-ee/2548-improve-codeclimate.yml

+---
+title: Improve Codeclimate UI
+merge_request:
+author:
+type: changed

ee/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_code_quality.vue

@@ -36,7 +36,7 @@ export default {
   computed: {
     status() {
       if (this.loadingFailed || this.mr.codeclimateMetrics.newIssues.length) {
-        return 'failed';
+        return 'warning';
       }
       return 'success';
     },
@@ -118,7 +118,7 @@ export default {
         .then(resp => resp.json()),
     ])
       .then((values) => {
-        this.mr.compareCodeclimateMetrics(values[0], values[1]);
+        this.mr.compareCodeclimateMetrics(values[0], values[1], head_blob_path, base_blob_path);
         this.isLoading = false;
       })
       .catch(() => this.handleError());

ee/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_code_quality_issues.vue

 <script>
-export default {
+  import { spriteIcon } from '~/lib/utils/common_utils';
+
+  export default {
     name: 'MRWidgetCodeQualityIssues',
     props: {
       issues: {
@@ -11,32 +13,55 @@ export default {
         required: true,
       },
     },
-};
+    computed: {
+      plusIcon() {
+        return spriteIcon('plus');
+      },
+      minusIcon() {
+        return spriteIcon('cut');
+      },
+      isTypeFailed() {
+        return this.type === 'failed';
+      },
+      isTypeSuccess() {
+        return this.type === 'success';
+      },
+    },
+  };
 </script>
 <template>
   <ul class="mr-widget-code-quality-list">
     <li
-      class="commit-sha"
       :class="{
-        failed: type === 'failed',
-        success: type === 'success'
+        failed: isTypeFailed,
+        success: isTypeSuccess,
       }
       "v-for="issue in issues">
-      <i
-        class="fa"
-        :class="{
-          'fa-minus': type === 'failed',
-          'fa-plus': type === 'success'
-        }"
-        aria-hidden="true">
-      </i>
-      <span>
-        <span v-if="type === 'success'">Fixed:</span>
+      <span
+        class="mr-widget-code-quality-icon"
+        v-if="isTypeFailed"
+        v-html="minusIcon">
+      </span>
+      <span
+        class="mr-widget-code-quality-icon"
+        v-else-if="isTypeSuccess"
+        v-html="plusIcon">
+      </span>
+      <span
+        class="mr-widget-code-quality-title"
+        v-if="isTypeSuccess">
+        Fixed:
+      </span>
+      <span class="mr-widget-code-quality-title">
         {{issue.check_name}}
-        {{issue.location.path}}
-        {{issue.location.positions}}
-        {{issue.location.lines}}
       </span>
+      <template v-if="issue.location.path">in</template>
+      <a
+        :href="issue.location.urlPath"
+        target="_blank"
+        rel="noopener noreferrer nofollow">
+        {{issue.location.path}}
+      </a>
   </li>
   </ul>
 </template>

ee/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -56,15 +56,36 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     };
   }
 
-  compareCodeclimateMetrics(headIssues, baseIssues) {
-    // newIssues link to head_blob_path+"/"+filename+"#L"+begin
-    this.codeclimateMetrics.newIssues = this.filterByFingerprint(headIssues, baseIssues);
+  compareCodeclimateMetrics(headIssues, baseIssues, headBlobPath, baseBlobPath) {
+    const parsedHeadIssues = MergeRequestStore.addPathToIssues(headIssues, headBlobPath);
+    const parsedBaseIssues = MergeRequestStore.addPathToIssues(baseIssues, baseBlobPath);
 
-    // resolvedIssues link to base_blob_path+"/"+filename+"#L"+begin
-    this.codeclimateMetrics.resolvedIssues = this.filterByFingerprint(baseIssues, headIssues);
+    this.codeclimateMetrics.newIssues = MergeRequestStore.filterByFingerprint(
+      parsedHeadIssues,
+      parsedBaseIssues,
+    );
+    this.codeclimateMetrics.resolvedIssues = MergeRequestStore.filterByFingerprint(
+      parsedBaseIssues,
+      parsedHeadIssues,
+    );
   }
 
-  filterByFingerprint(firstArray, secondArray) { // eslint-disable-line
+  static filterByFingerprint(firstArray, secondArray) {
     return firstArray.filter(item => !secondArray.find(el => el.fingerprint === item.fingerprint));
   }
+
+  static addPathToIssues(issues, path) {
+    return issues.map((issue) => {
+      let parsedUrl = `${path}/${issue.location.path}`;
+
+      if (issue.location.lines && issue.location.lines.begin) {
+        parsedUrl += `#L${issue.location.lines.begin}`;
+      }
+
+      return Object.assign({}, issue, {
+        location: Object.assign({}, issue.location, { urlPath: parsedUrl }),
+      });
+    });
+  }
 }
+

spec/javascripts/vue_mr_widget/components/mr_widget_code_quality_issues_spec.js

 import Vue from 'vue';
 import mrWidgetCodeQualityIssues from 'ee/vue_merge_request_widget/components/mr_widget_code_quality_issues.vue';
 
-describe('Merge Request Code Quality Issues', () => {
+describe('merge request code quality issues', () => {
   let vm;
   let MRWidgetCodeQualityIssues;
   let mountComponent;
@@ -11,7 +11,7 @@ describe('Merge Request Code Quality Issues', () => {
     mountComponent = props => new MRWidgetCodeQualityIssues({ propsData: props }).$mount();
   });
 
-  describe('Renders provided list of issues', () => {
+  describe('renders provided list of issues', () => {
     describe('with positions and lines', () => {
       beforeEach(() => {
         vm = mountComponent({
@@ -20,6 +20,7 @@ describe('Merge Request Code Quality Issues', () => {
             check_name: 'foo',
             location: {
               path: 'bar',
+              urlPath: 'foo',
               positions: '81',
               lines: '21',
             },
@@ -29,28 +30,8 @@ describe('Merge Request Code Quality Issues', () => {
 
       it('should render issue', () => {
         expect(
-          vm.$el.querySelector('li span').textContent.trim().replace(/\s+/g, ''),
-        ).toEqual('Fixed:foobar8121');
-      });
-    });
-
-    describe('without positions and lines', () => {
-      beforeEach(() => {
-        vm = mountComponent({
-          type: 'success',
-          issues: [{
-            check_name: 'foo',
-            location: {
-              path: 'bar',
-            },
-          }],
-        });
-      });
-
-      it('should render issue without position and lines', () => {
-        expect(
-          vm.$el.querySelector('li span').textContent.trim().replace(/\s+/g, ''),
-        ).toEqual('Fixed:foobar');
+          vm.$el.querySelector('li').textContent.trim().replace(/\s+/g, ''),
+        ).toEqual('Fixed:fooinbar');
       });
     });
 
@@ -71,7 +52,7 @@ describe('Merge Request Code Quality Issues', () => {
 
       it('should render failed minus icon', () => {
         expect(vm.$el.querySelector('li').classList.contains('failed')).toEqual(true);
-        expect(vm.$el.querySelector('li i').classList.contains('fa-minus')).toEqual(true);
+        expect(vm.$el.querySelector('li svg use').getAttribute('xlink:href')).toContain('cut');
       });
     });
 
@@ -92,7 +73,7 @@ describe('Merge Request Code Quality Issues', () => {
 
       it('should render success plus icon', () => {
         expect(vm.$el.querySelector('li').classList.contains('success')).toEqual(true);
-        expect(vm.$el.querySelector('li i').classList.contains('fa-plus')).toEqual(true);
+        expect(vm.$el.querySelector('li svg use').getAttribute('xlink:href')).toContain('plus');
       });
     });
   });

spec/javascripts/vue_mr_widget/mock_data.js

@@ -222,15 +222,9 @@ export const headIssues = [
     "check_name": "Rubocop/Lint/UselessAssignment",
     "location": {
       "path": "lib/six.rb",
-      "positions": {
-        "begin": {
-          "column": 6,
-          "line": 59
-        },
-        "end": {
-          "column": 7,
-          "line": 59
-        }
+      "lines": {
+        "begin": 6,
+        "end": 7,
       }
     },
     "fingerprint": "e879dd9bbc0953cad5037cde7ff0f627",

spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js

@@ -56,15 +56,25 @@ describe('MergeRequestStore', () => {
 
   describe('compareCodeclimateMetrics', () => {
     beforeEach(() => {
-      store.compareCodeclimateMetrics(headIssues, baseIssues);
+      store.compareCodeclimateMetrics(headIssues, baseIssues, 'headPath', 'basePath');
     });
 
     it('should return the new issues', () => {
-      expect(store.codeclimateMetrics.newIssues[0]).toEqual(headIssues[0]);
+      const parsed = MergeRequestStore.addPathToIssues(headIssues, 'headPath');
+      expect(store.codeclimateMetrics.newIssues[0]).toEqual(parsed[0]);
     });
 
     it('should return the resolved issues', () => {
-      expect(store.codeclimateMetrics.resolvedIssues[0]).toEqual(baseIssues[1]);
+      const parsed = MergeRequestStore.addPathToIssues(baseIssues, 'basePath');
+      expect(store.codeclimateMetrics.resolvedIssues[0]).toEqual(parsed[1]);
+    });
+  });
+
+  describe('addPathToIssues', () => {
+    it('should add urlPath key to each entry', () => {
+      expect(
+        MergeRequestStore.addPathToIssues(headIssues, 'path')[0].location.urlPath,
+      ).toEqual(`path/${headIssues[0].location.path}#L${headIssues[0].location.lines.begin}`);
     });
   });
 });