Merge branch '267612-show-code-quality-badge-in-mr-diff-file-header' into 'master'

Show code quality badge on files in MR diff view

See merge request gitlab-org/gitlab!56710

app/assets/javascripts/diffs/components/app.vue

@@ -84,6 +84,11 @@ export default {
       required: false,
       default: '',
     },
+    endpointCodequality: {
+      type: String,
+      required: false,
+      default: '',
+    },
     projectPath: {
       type: String,
       required: true,
@@ -160,6 +165,7 @@ export default {
       plainDiffPath: (state) => state.diffs.plainDiffPath,
       emailPatchPath: (state) => state.diffs.emailPatchPath,
       retrievingBatches: (state) => state.diffs.retrievingBatches,
+      codequalityDiff: (state) => state.diffs.codequalityDiff,
     }),
     ...mapState('diffs', [
       'showTreeList',
@@ -173,7 +179,12 @@ export default {
       'viewDiffsFileByFile',
       'mrReviews',
     ]),
-    ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']),
+    ...mapGetters('diffs', [
+      'whichCollapsedTypes',
+      'isParallelView',
+      'currentDiffIndex',
+      'fileCodequalityDiff',
+    ]),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     diffs() {
       if (!this.viewDiffsFileByFile) {
@@ -271,6 +282,7 @@ export default {
       endpointMetadata: this.endpointMetadata,
       endpointBatch: this.endpointBatch,
       endpointCoverage: this.endpointCoverage,
+      endpointCodequality: this.endpointCodequality,
       projectPath: this.projectPath,
       dismissEndpoint: this.dismissEndpoint,
       showSuggestPopover: this.showSuggestPopover,
@@ -326,6 +338,7 @@ export default {
       'fetchDiffFilesMeta',
       'fetchDiffFilesBatch',
       'fetchCoverageFiles',
+      'fetchCodequality',
       'startRenderDiffsQueue',
       'assignDiscussionsToDiff',
       'setHighlightedRow',
@@ -382,6 +395,10 @@ export default {
         this.fetchCoverageFiles();
       }
 
+      if (this.endpointCodequality) {
+        this.fetchCodequality();
+      }
+
       if (!this.isNotesFetched) {
         notesEventHub.$emit('fetchNotesData');
       }
@@ -509,6 +526,7 @@ export default {
               :help-page-path="helpPagePath"
               :can-current-user-fork="canCurrentUserFork"
               :view-diffs-file-by-file="viewDiffsFileByFile"
+              :codequality-diff="fileCodequalityDiff(file.file_path)"
             />
             <div
               v-if="showFileByFileNavigation"

app/assets/javascripts/diffs/components/diff_file.vue

@@ -67,6 +67,11 @@ export default {
       type: Boolean,
       required: true,
     },
+    codequalityDiff: {
+      type: Array,
+      required: false,
+      default: () => [],
+    },
   },
   data() {
     return {
@@ -148,6 +153,9 @@ export default {
 
       return loggedIn && featureOn;
     },
+    hasCodequalityChanges() {
+      return this.codequalityDiff.length > 0;
+    },
   },
   watch: {
     'file.id': {
@@ -294,6 +302,7 @@ export default {
       :add-merge-request-buttons="true"
       :view-diffs-file-by-file="viewDiffsFileByFile"
       :show-local-file-reviews="showLocalFileReviews"
+      :has-codequality-changes="hasCodequalityChanges"
       class="js-file-title file-title gl-border-1 gl-border-solid gl-border-gray-100"
       :class="hasBodyClasses.header"
       @toggleFile="handleToggle"

app/assets/javascripts/diffs/components/diff_file_header.vue

@@ -41,6 +41,7 @@ export default {
     GlDropdownDivider,
     GlFormCheckbox,
     GlLoadingIcon,
+    CodeQualityBadge: () => import('ee_component/diffs/components/code_quality_badge.vue'),
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -95,6 +96,11 @@ export default {
       required: false,
       default: false,
     },
+    hasCodequalityChanges: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -327,6 +333,8 @@ export default {
         data-track-property="diff_copy_file"
       />
 
+      <code-quality-badge v-if="hasCodequalityChanges" class="gl-mr-2" />
+
       <small v-if="isModeChanged" ref="fileMode" class="mr-1">
         {{ diffFile.a_mode }} → {{ diffFile.b_mode }}
       </small>

app/assets/javascripts/diffs/index.js

@@ -73,6 +73,7 @@ export default function initDiffsApp(store) {
         endpointMetadata: dataset.endpointMetadata || '',
         endpointBatch: dataset.endpointBatch || '',
         endpointCoverage: dataset.endpointCoverage || '',
+        endpointCodequality: dataset.endpointCodequality || '',
         projectPath: dataset.projectPath,
         helpPagePath: dataset.helpPagePath,
         currentUser: JSON.parse(dataset.currentUserData) || {},
@@ -114,6 +115,7 @@ export default function initDiffsApp(store) {
           endpointMetadata: this.endpointMetadata,
           endpointBatch: this.endpointBatch,
           endpointCoverage: this.endpointCoverage,
+          endpointCodequality: this.endpointCodequality,
           currentUser: this.currentUser,
           projectPath: this.projectPath,
           helpPagePath: this.helpPagePath,

app/assets/javascripts/diffs/store/actions.js

 import Cookies from 'js-cookie';
+import Visibility from 'visibilityjs';
 import Vue from 'vue';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
 import { diffViewerModes } from '~/ide/constants';
@@ -52,12 +53,15 @@ import {
   prepareLineForRenamedFile,
 } from './utils';
 
+let eTagPoll;
+
 export const setBaseConfig = ({ commit }, options) => {
   const {
     endpoint,
     endpointMetadata,
     endpointBatch,
     endpointCoverage,
+    endpointCodequality,
     projectPath,
     dismissEndpoint,
     showSuggestPopover,
@@ -70,6 +74,7 @@ export const setBaseConfig = ({ commit }, options) => {
     endpointMetadata,
     endpointBatch,
     endpointCoverage,
+    endpointCodequality,
     projectPath,
     dismissEndpoint,
     showSuggestPopover,
@@ -231,6 +236,48 @@ export const fetchCoverageFiles = ({ commit, state }) => {
   coveragePoll.makeRequest();
 };
 
+export const clearEtagPoll = () => {
+  eTagPoll = null;
+};
+
+export const stopCodequalityPolling = () => {
+  if (eTagPoll) eTagPoll.stop();
+};
+
+export const restartCodequalityPolling = () => {
+  if (eTagPoll) eTagPoll.restart();
+};
+
+export const fetchCodequality = ({ commit, state, dispatch }) => {
+  eTagPoll = new Poll({
+    resource: {
+      getCodequalityDiffReports: (endpoint) => axios.get(endpoint),
+    },
+    data: state.endpointCodequality,
+    method: 'getCodequalityDiffReports',
+    successCallback: ({ status, data }) => {
+      if (status === httpStatusCodes.OK) {
+        commit(types.SET_CODEQUALITY_DATA, data);
+
+        eTagPoll.stop();
+      }
+    },
+    errorCallback: () => createFlash(__('Something went wrong on our end. Please try again!')),
+  });
+
+  if (!Visibility.hidden()) {
+    eTagPoll.makeRequest();
+  }
+
+  Visibility.change(() => {
+    if (!Visibility.hidden()) {
+      dispatch('restartCodequalityPolling');
+    } else {
+      dispatch('stopCodequalityPolling');
+    }
+  });
+};
+
 export const setHighlightedRow = ({ commit }, lineCode) => {
   const fileHash = lineCode.split('_')[0];
   commit(types.SET_HIGHLIGHTED_ROW, lineCode);

app/assets/javascripts/diffs/store/getters.js

@@ -135,6 +135,16 @@ export const fileLineCoverage = (state) => (file, line) => {
   return {};
 };
 
+/**
+ * Returns the codequality diff data for a given file
+ * @param {string} filePath
+ * @returns {Array}
+ */
+export const fileCodequalityDiff = (state) => (filePath) => {
+  if (!state.codequalityDiff.files || !state.codequalityDiff.files[filePath]) return [];
+  return state.codequalityDiff.files[filePath];
+};
+
 /**
  * Returns index of a currently selected diff in diffFiles
  * @returns {number}

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -28,6 +28,7 @@ export default () => ({
   startVersion: null, // Null unless a target diff is selected for comparison that is not the "base" diff
   diffFiles: [],
   coverageFiles: {},
+  codequalityDiff: {},
   mergeRequestDiffs: [],
   mergeRequestDiff: null,
   diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType,

app/assets/javascripts/diffs/store/mutation_types.js

@@ -11,6 +11,7 @@ export const SET_MR_FILE_REVIEWS = 'SET_MR_FILE_REVIEWS';
 
 export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';
 export const SET_COVERAGE_DATA = 'SET_COVERAGE_DATA';
+export const SET_CODEQUALITY_DATA = 'SET_CODEQUALITY_DATA';
 export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS';
 export const TOGGLE_LINE_HAS_FORM = 'TOGGLE_LINE_HAS_FORM';
 export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES';

app/assets/javascripts/diffs/store/mutations.js

@@ -33,6 +33,7 @@ export default {
       endpointMetadata,
       endpointBatch,
       endpointCoverage,
+      endpointCodequality,
       projectPath,
       dismissEndpoint,
       showSuggestPopover,
@@ -45,6 +46,7 @@ export default {
       endpointMetadata,
       endpointBatch,
       endpointCoverage,
+      endpointCodequality,
       projectPath,
       dismissEndpoint,
       showSuggestPopover,
@@ -87,6 +89,10 @@ export default {
     Object.assign(state, { coverageFiles });
   },
 
+  [types.SET_CODEQUALITY_DATA](state, codequalityDiffData) {
+    Object.assign(state, { codequalityDiff: codequalityDiffData });
+  },
+
   [types.RENDER_FILE](state, file) {
     renderFile(file);
   },

app/helpers/merge_requests_helper.rb

@@ -185,6 +185,27 @@ module MergeRequestsHelper
     end
   end
 
+  def diffs_tab_pane_data(project, merge_request, params)
+    {
+      "is-locked": merge_request.discussion_locked?,
+      endpoint: diffs_project_merge_request_path(project, merge_request, 'json', params),
+      endpoint_metadata: @endpoint_metadata_url,
+      endpoint_batch: diffs_batch_project_json_merge_request_path(project, merge_request, 'json', params),
+      endpoint_coverage: @coverage_path,
+      help_page_path: help_page_path('user/discussions/index.md', anchor: 'suggest-changes'),
+      current_user_data: @current_user_data,
+      update_current_user_path: @update_current_user_path,
+      project_path: project_path(merge_request.project),
+      changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg'),
+      is_fluid_layout: fluid_layout.to_s,
+      dismiss_endpoint: user_callouts_path,
+      show_suggest_popover: show_suggest_popover?.to_s,
+      show_whitespace_default: @show_whitespace_default.to_s,
+      file_by_file_default: @file_by_file_default.to_s,
+      default_suggestion_commit_message: default_suggestion_commit_message
+    }
+  end
+
   private
 
   def review_requested_merge_requests_count

app/views/projects/merge_requests/show.html.haml

@@ -81,22 +81,7 @@
       - params = request.query_parameters
       - if Feature.enabled?(:default_merge_ref_for_diffs, @project, default_enabled: :yaml)
         - params = params.merge(diff_head: true)
-      = render "projects/merge_requests/tabs/pane", name: "diffs", id: "js-diffs-app", class: "diffs", data: { "is-locked": @merge_request.discussion_locked?,
-        endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', params),
-        endpoint_metadata: @endpoint_metadata_url,
-        endpoint_batch: diffs_batch_project_json_merge_request_path(@project, @merge_request, 'json', params),
-        endpoint_coverage: @coverage_path,
-        help_page_path: suggest_changes_help_path,
-        current_user_data: @current_user_data,
-        update_current_user_path: @update_current_user_path,
-        project_path: project_path(@merge_request.project),
-        changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg'),
-        is_fluid_layout: fluid_layout.to_s,
-        dismiss_endpoint: user_callouts_path,
-        show_suggest_popover: show_suggest_popover?.to_s,
-        show_whitespace_default: @show_whitespace_default.to_s,
-        file_by_file_default: @file_by_file_default.to_s,
-        default_suggestion_commit_message: default_suggestion_commit_message }
+      = render "projects/merge_requests/tabs/pane", name: "diffs", id: "js-diffs-app", class: "diffs", data: diffs_tab_pane_data(@project, @merge_request, params)
 
     .mr-loading-status
       .loading.hide

ee/app/assets/javascripts/diffs/components/code_quality_badge.vue

+<script>
+import { GlBadge, GlTooltipDirective } from '@gitlab/ui';
+import { __ } from '~/locale';
+
+export default {
+  components: {
+    GlBadge,
+  },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
+  i18n: {
+    badgeTitle: __('Code Quality'),
+    badgeTooltip: __(
+      'The merge request has been updated, and the number of code quality violations in this file has changed.',
+    ),
+  },
+};
+</script>
+
+<template>
+  <gl-badge
+    v-gl-tooltip
+    :title="$options.i18n.badgeTooltip"
+    icon="information"
+    class="gl-display-inline-block"
+  >
+    {{ $options.i18n.badgeTitle }}
+  </gl-badge>
+</template>

ee/app/helpers/ee/merge_requests_helper.rb

@@ -2,6 +2,8 @@
 
 module EE
   module MergeRequestsHelper
+    extend ::Gitlab::Utils::Override
+
     def render_items_list(items, separator = "and")
       items_cnt = items.size
 
@@ -15,5 +17,12 @@ module EE
         "#{items.join(", ")} #{separator} #{last_item}"
       end
     end
+
+    override :diffs_tab_pane_data
+    def diffs_tab_pane_data(project, merge_request, params)
+      super.merge(
+        endpoint_codequality: (codequality_mr_diff_reports_project_merge_request_path(@project, @merge_request, 'json') if project.licensed_feature_available?(:inline_codequality) && @merge_request.has_codequality_mr_diff_report?)
+      )
+    end
   end
 end

ee/app/models/license.rb

@@ -157,6 +157,7 @@ class License < ApplicationRecord
     group_ci_cd_analytics
     group_level_compliance_dashboard
     incident_management
+    inline_codequality
     insights
     issuable_health_status
     jira_vulnerabilities_integration

ee/spec/frontend/diffs/components/diff_file_spec.js

+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import Vuex from 'vuex';
+
+import CodeQualityBadge from 'ee/diffs/components/code_quality_badge.vue';
+import diffFileMockDataReadable from 'jest/diffs/mock_data/diff_file';
+import DiffFileComponent from '~/diffs/components/diff_file.vue';
+
+import createDiffsStore from '~/diffs/store/modules';
+
+const getReadableFile = () => JSON.parse(JSON.stringify(diffFileMockDataReadable));
+
+function createComponent({ first = false, last = false, options = {}, props = {} }) {
+  const file = getReadableFile();
+  const localVue = createLocalVue();
+
+  localVue.use(Vuex);
+
+  const store = new Vuex.Store({
+    modules: {
+      diffs: createDiffsStore(),
+    },
+  });
+
+  store.state.diffs.diffFiles = [file];
+
+  const wrapper = shallowMount(DiffFileComponent, {
+    store,
+    localVue,
+    propsData: {
+      file,
+      canCurrentUserFork: false,
+      viewDiffsFileByFile: false,
+      isFirstFile: first,
+      isLastFile: last,
+      ...props,
+    },
+    ...options,
+  });
+
+  return {
+    localVue,
+    wrapper,
+    store,
+  };
+}
+
+describe('EE DiffFile', () => {
+  let wrapper;
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('code quality badge', () => {
+    it('is shown when there is diff data for the file', () => {
+      ({ wrapper } = createComponent({
+        props: {
+          codequalityDiff: [
+            { line: 1, description: 'Unexpected alert.', severity: 'minor' },
+            {
+              line: 3,
+              description: 'Arrow function has too many statements (52). Maximum allowed is 30.',
+              severity: 'minor',
+            },
+          ],
+        },
+      }));
+
+      expect(wrapper.find(CodeQualityBadge)).toExist();
+    });
+
+    it('is not shown when there is no diff data for the file', () => {
+      ({ wrapper } = createComponent({}));
+
+      expect(wrapper.find(CodeQualityBadge)).toExist();
+    });
+  });
+});

locale/gitlab.pot

@@ -30516,6 +30516,9 @@ msgstr ""
 msgid "The merge request can now be merged."
 msgstr ""
 
+msgid "The merge request has been updated, and the number of code quality violations in this file has changed."
+msgstr ""
+
 msgid "The metric must be one of %{metrics}."
 msgstr ""
 

spec/frontend/diffs/components/app_spec.js

@@ -56,6 +56,7 @@ describe('diffs/components/app', () => {
         endpointMetadata: `${TEST_HOST}/diff/endpointMetadata`,
         endpointBatch: `${TEST_HOST}/diff/endpointBatch`,
         endpointCoverage: `${TEST_HOST}/diff/endpointCoverage`,
+        endpointCodequality: `${TEST_HOST}/diff/endpointCodequality`,
         projectPath: 'namespace/project',
         currentUser: {},
         changesEmptyStateIllustration: '',
@@ -141,6 +142,24 @@ describe('diffs/components/app', () => {
     });
   });
 
+  describe('codequality diff', () => {
+    it('fetches code quality data when endpoint is provided', () => {
+      createComponent();
+      jest.spyOn(wrapper.vm, 'fetchCodequality');
+      wrapper.vm.fetchData(false);
+
+      expect(wrapper.vm.fetchCodequality).toHaveBeenCalled();
+    });
+
+    it('does not fetch code quality data when endpoint is blank', async () => {
+      createComponent({ endpointCodequality: '' });
+      jest.spyOn(wrapper.vm, 'fetchCodequality');
+      wrapper.vm.fetchData(false);
+
+      expect(wrapper.vm.fetchCodequality).not.toHaveBeenCalled();
+    });
+  });
+
   it.each`
     props                      | state                                                              | expected
     ${{ isFluidLayout: true }} | ${{ isParallelView: false }}                                       | ${false}

spec/frontend/diffs/store/actions_spec.js

@@ -17,6 +17,9 @@ import {
   fetchDiffFilesBatch,
   fetchDiffFilesMeta,
   fetchCoverageFiles,
+  clearEtagPoll,
+  stopCodequalityPolling,
+  fetchCodequality,
   assignDiscussionsToDiff,
   removeDiscussionsFromDiff,
   startRenderDiffsQueue,
@@ -98,6 +101,7 @@ describe('DiffsStoreActions', () => {
       const endpointMetadata = '/diffs/set/endpoint/metadata';
       const endpointBatch = '/diffs/set/endpoint/batch';
       const endpointCoverage = '/diffs/set/coverage_reports';
+      const endpointCodequality = '/diffs/set/codequality_diff';
       const projectPath = '/root/project';
       const dismissEndpoint = '/-/user_callouts';
       const showSuggestPopover = false;
@@ -109,6 +113,7 @@ describe('DiffsStoreActions', () => {
           endpointBatch,
           endpointMetadata,
           endpointCoverage,
+          endpointCodequality,
           projectPath,
           dismissEndpoint,
           showSuggestPopover,
@@ -118,6 +123,7 @@ describe('DiffsStoreActions', () => {
           endpointBatch: '',
           endpointMetadata: '',
           endpointCoverage: '',
+          endpointCodequality: '',
           projectPath: '',
           dismissEndpoint: '',
           showSuggestPopover: true,
@@ -130,6 +136,7 @@ describe('DiffsStoreActions', () => {
               endpointMetadata,
               endpointBatch,
               endpointCoverage,
+              endpointCodequality,
               projectPath,
               dismissEndpoint,
               showSuggestPopover,
@@ -297,6 +304,47 @@ describe('DiffsStoreActions', () => {
     });
   });
 
+  describe('fetchCodequality', () => {
+    let mock;
+    const endpointCodequality = '/fetch';
+
+    beforeEach(() => {
+      mock = new MockAdapter(axios);
+    });
+
+    afterEach(() => {
+      stopCodequalityPolling();
+      clearEtagPoll();
+    });
+
+    it('should commit SET_CODEQUALITY_DATA with received response', (done) => {
+      const data = {
+        files: { 'app.js': [{ line: 1, description: 'Unexpected alert.', severity: 'minor' }] },
+      };
+
+      mock.onGet(endpointCodequality).reply(200, { data });
+
+      testAction(
+        fetchCodequality,
+        {},
+        { endpointCodequality },
+        [{ type: types.SET_CODEQUALITY_DATA, payload: { data } }],
+        [],
+        done,
+      );
+    });
+
+    it('should show flash on API error', (done) => {
+      mock.onGet(endpointCodequality).reply(400);
+
+      testAction(fetchCodequality, {}, { endpointCodequality }, [], [], () => {
+        expect(createFlash).toHaveBeenCalledTimes(1);
+        expect(createFlash).toHaveBeenCalledWith(expect.stringMatching('Something went wrong'));
+        done();
+      });
+    });
+  });
+
   describe('setHighlightedRow', () => {
     it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => {
       testAction(setHighlightedRow, 'ABC_123', {}, [

spec/frontend/diffs/store/getters_spec.js

@@ -376,6 +376,26 @@ describe('Diffs Module Getters', () => {
     });
   });
 
+  describe('fileCodequalityDiff', () => {
+    beforeEach(() => {
+      Object.assign(localState.codequalityDiff, {
+        files: { 'app.js': [{ line: 1, description: 'Unexpected alert.', severity: 'minor' }] },
+      });
+    });
+
+    it('returns empty array when no codequality data is available', () => {
+      Object.assign(localState.codequalityDiff, {});
+
+      expect(getters.fileCodequalityDiff(localState)('test.js')).toEqual([]);
+    });
+
+    it('returns array when codequality data is available for given file', () => {
+      expect(getters.fileCodequalityDiff(localState)('app.js')).toEqual([
+        { line: 1, description: 'Unexpected alert.', severity: 'minor' },
+      ]);
+    });
+  });
+
   describe('suggestionCommitMessage', () => {
     let rootState;
 

spec/frontend/diffs/store/mutations_spec.js

@@ -113,6 +113,19 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
+  describe('SET_CODEQUALITY_DATA', () => {
+    it('should set codequality data', () => {
+      const state = { codequalityDiff: {} };
+      const codequality = {
+        files: { 'app.js': [{ line: 1, description: 'Unexpected alert.', severity: 'minor' }] },
+      };
+
+      mutations[types.SET_CODEQUALITY_DATA](state, codequality);
+
+      expect(state.codequalityDiff).toEqual(codequality);
+    });
+  });
+
   describe('SET_DIFF_VIEW_TYPE', () => {
     it('should set diff view type properly', () => {
       const state = {};