Merge branch '2548-improve-codeclimate' into 'master'

Resolve "Improve code quality widget design"

Closes #2548

See merge request gitlab-org/gitlab-ee!3215

app/assets/stylesheets/pages/merge_requests.scss

@@ -778,7 +778,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;
@@ -789,15 +791,25 @@
 
     .mr-widget-code-quality-list {
       list-style: none;
-      padding: 4px 36px;
+      padding: 0 12px;
       margin: 0;
       line-height: $code_line_height;
 
-      li.success {
+      .mr-widget-code-quality-icon {
+        margin-right: 12px;
+        fill: currentColor;
+
+        svg {
+          width: 10px;
+          height: 10px;
+        }
+      }
+
+      .success {
         color: $green-500;
       }
 
-      li.failed {
+      .failed {
         color: $red-500;
       }
     }

app/serializers/merge_request_entity.rb

@@ -197,11 +197,19 @@ class MergeRequestEntity < IssuableEntity
                                       path: 'codeclimate.json')
     end
 
+    expose :head_blob_path, if: -> (mr, _) { mr.head_pipeline_sha } do |merge_request|
+      project_blob_path(merge_request.project, merge_request.head_pipeline_sha)
+    end
+
     expose :base_path, if: -> (mr, _) { can?(current_user, :read_build, mr.base_codeclimate_artifact) } do |merge_request|
       raw_project_build_artifacts_url(merge_request.target_project,
                                       merge_request.base_codeclimate_artifact,
                                       path: 'codeclimate.json')
     end
+
+    expose :base_blob_path, if: -> (mr, _) { mr.base_pipeline_sha } do |merge_request|
+      project_blob_path(merge_request.project, merge_request.base_pipeline_sha)
+    end
   end
 
   private

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';
     },
@@ -107,7 +107,7 @@ export default {
   },
 
   created() {
-    const { head_path, base_path } = this.mr.codeclimate;
+    const { head_path, head_blob_path, base_path, base_blob_path } = this.mr.codeclimate;
 
     this.isLoading = true;
 
@@ -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());
@@ -132,10 +132,7 @@ export default {
       v-if="isLoading"
       class="media">
       <div class="mr-widget-icon">
-        <i
-          class="fa fa-spinner fa-spin"
-          aria-hidden="true">
-        </i>
+        <loading-icon />
       </div>
       <div class="media-body">
         Loading codeclimate report

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,40 @@ export default {
         required: true,
       },
     },
-};
+    computed: {
+      icon() {
+        return this.isTypeFailed ? spriteIcon('cut') : spriteIcon('plus');
+      },
+      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>
-        {{issue.check_name}}
-        {{issue.location.path}}
-        {{issue.location.positions}}
-        {{issue.location.lines}}
+      <span
+        class="mr-widget-code-quality-icon"
+        v-html="icon">
       </span>
+      <template v-if="isTypeSuccess">Fixed:</template>
+      {{issue.check_name}}
+      <template v-if="issue.location.path">in</template>
+      <a
+        :href="issue.location.urlPath"
+        target="_blank"
+        rel="noopener noreferrer nofollow">
+        {{issue.location.path}}<template v-if="issue.location.lines && issue.location.lines.begin">:{{issue.location.lines.begin}}</template>
+      </a>
   </li>
   </ul>
 </template>

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

@@ -56,12 +56,39 @@ export default class MergeRequestStore extends CEMergeRequestStore {
     };
   }
 
-  compareCodeclimateMetrics(headIssues, baseIssues) {
-    this.codeclimateMetrics.newIssues = this.filterByFingerprint(headIssues, baseIssues);
-    this.codeclimateMetrics.resolvedIssues = this.filterByFingerprint(baseIssues, headIssues);
+  compareCodeclimateMetrics(headIssues, baseIssues, headBlobPath, baseBlobPath) {
+    const parsedHeadIssues = MergeRequestStore.addPathToIssues(headIssues, headBlobPath);
+    const parsedBaseIssues = MergeRequestStore.addPathToIssues(baseIssues, baseBlobPath);
+
+    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) => {
+      if (issue.location) {
+        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 }),
+        });
+      }
+      return issue;
+    });
+  }
 }
+

ee/app/models/ee/merge_request.rb

@@ -11,6 +11,8 @@ module EE
 
       delegate :codeclimate_artifact, to: :head_pipeline, prefix: :head, allow_nil: true
       delegate :codeclimate_artifact, to: :base_pipeline, prefix: :base, allow_nil: true
+      delegate :sha, to: :head_pipeline, prefix: :head_pipeline, allow_nil: true
+      delegate :sha, to: :base_pipeline, prefix: :base_pipeline, allow_nil: true
     end
 
     def rebase_dir_path

spec/fixtures/api/schemas/entities/merge_request.json

@@ -113,7 +113,9 @@
     "approvals_path": { "type": ["string", "null"] },
     "codeclimate": {
       "head_path": { "type": "string" },
-      "base_path": { "type": "string" }
+      "head_blob_path": { "type": "string" },
+      "base_path": { "type": "string" },
+      "base_blob_path": { "type": "string" }
     }
   },
   "additionalProperties": false

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,37 +20,20 @@ describe('Merge Request Code Quality Issues', () => {
             check_name: 'foo',
             location: {
               path: 'bar',
+              urlPath: 'foo',
               positions: '81',
-              lines: '21',
+              lines: {
+                begin: '21',
               },
-          }],
-        });
-      });
-
-      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', () => {
+      it('should render issue', () => {
         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:21');
       });
     });
 
@@ -63,7 +46,9 @@ describe('Merge Request Code Quality Issues', () => {
             location: {
               path: 'bar',
               positions: '81',
-              lines: '21',
+              lines: {
+                begin: '21',
+              },
             },
           }],
         });
@@ -71,7 +56,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');
       });
     });
 
@@ -84,7 +69,9 @@ describe('Merge Request Code Quality Issues', () => {
             location: {
               path: 'bar',
               positions: '81',
-              lines: '21',
+              lines: {
+                begin: '21',
+              },
             },
           }],
         });
@@ -92,7 +79,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

@@ -213,7 +213,9 @@ export default {
   "commit_change_content_path": "/root/acets-app/merge_requests/22/commit_change_content",
   "codeclimate": {
     "head_path": "head.json",
-    "base_path": "base.json"
+    "head_blob_path": "/root/acets-app/blob/abcdef",
+    "base_path": "base.json",
+    "base_blob_path": "/root/acets-app/blob/abcdef"
   }
 };
 
@@ -222,15 +224,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",
@@ -274,4 +270,4 @@ export const baseIssues = [
     },
     "fingerprint": "ca2354534dee94ae60ba2f54e3857c50e5",
   }
-]
+];

spec/javascripts/vue_mr_widget/stores/mr_widget_store_spec.js

@@ -56,15 +56,31 @@ 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}`);
+    });
+
+    it('should return the same object whe there is no locaiton', () => {
+      expect(
+        MergeRequestStore.addPathToIssues([{ check_name: 'foo' }], 'path'),
+      ).toEqual([{ check_name: 'foo' }]);
     });
   });
 });