Merge branch '267612-codequality-badge-improvements' into 'master'

Improvements to the code quality badge in the MR diff file header

See merge request gitlab-org/gitlab!58833

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

@@ -184,12 +184,7 @@ export default {
       'viewDiffsFileByFile',
       'mrReviews',
     ]),
-    ...mapGetters('diffs', [
-      'whichCollapsedTypes',
-      'isParallelView',
-      'currentDiffIndex',
-      'fileCodequalityDiff',
-    ]),
+    ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     diffs() {
       if (!this.viewDiffsFileByFile) {
@@ -287,7 +282,6 @@ export default {
       endpointMetadata: this.endpointMetadata,
       endpointBatch: this.endpointBatch,
       endpointCoverage: this.endpointCoverage,
-      endpointCodequality: this.endpointCodequality,
       endpointUpdateUser: this.endpointUpdateUser,
       projectPath: this.projectPath,
       dismissEndpoint: this.dismissEndpoint,
@@ -297,6 +291,10 @@ export default {
       mrReviews: this.rehydratedMrReviews,
     });
 
+    if (this.endpointCodequality) {
+      this.setCodequalityEndpoint(this.endpointCodequality);
+    }
+
     if (this.shouldShow) {
       this.fetchData();
     }
@@ -341,6 +339,7 @@ export default {
     ...mapActions('diffs', [
       'moveToNeighboringCommit',
       'setBaseConfig',
+      'setCodequalityEndpoint',
       'fetchDiffFilesMeta',
       'fetchDiffFilesBatch',
       'fetchCoverageFiles',
@@ -532,7 +531,6 @@ 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,11 +67,6 @@ export default {
       type: Boolean,
       required: true,
     },
-    codequalityDiff: {
-      type: Array,
-      required: false,
-      default: () => [],
-    },
   },
   data() {
     return {
@@ -85,7 +80,7 @@ export default {
     genericError: GENERIC_ERROR,
   },
   computed: {
-    ...mapState('diffs', ['currentDiffFileId']),
+    ...mapState('diffs', ['currentDiffFileId', 'codequalityDiff']),
     ...mapGetters(['isNotesFetched']),
     ...mapGetters('diffs', ['getDiffFileDiscussions']),
     viewBlobHref() {
@@ -154,7 +149,9 @@ export default {
       return loggedIn && featureOn;
     },
     hasCodequalityChanges() {
-      return this.codequalityDiff.length > 0;
+      return (
+        this.codequalityDiff?.files && this.codequalityDiff?.files[this.file.file_path]?.length > 0
+      );
     },
   },
   watch: {

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';
@@ -53,15 +52,12 @@ import {
   prepareLineForRenamedFile,
 } from './utils';
 
-let eTagPoll;
-
 export const setBaseConfig = ({ commit }, options) => {
   const {
     endpoint,
     endpointMetadata,
     endpointBatch,
     endpointCoverage,
-    endpointCodequality,
     endpointUpdateUser,
     projectPath,
     dismissEndpoint,
@@ -75,7 +71,6 @@ export const setBaseConfig = ({ commit }, options) => {
     endpointMetadata,
     endpointBatch,
     endpointCoverage,
-    endpointCodequality,
     endpointUpdateUser,
     projectPath,
     dismissEndpoint,
@@ -238,48 +233,6 @@ 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,16 +135,6 @@ 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

@@ -29,7 +29,6 @@ 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/modules/index.js

-import * as actions from '../actions';
+import * as actions from 'ee_else_ce/diffs/store/actions';
+import createState from 'ee_else_ce/diffs/store/modules/diff_state';
+import mutations from 'ee_else_ce/diffs/store/mutations';
 import * as getters from '../getters';
-import mutations from '../mutations';
-import createState from './diff_state';
 
 export default () => ({
   namespaced: true,

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

@@ -11,7 +11,6 @@ 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,7 +33,6 @@ export default {
       endpointMetadata,
       endpointBatch,
       endpointCoverage,
-      endpointCodequality,
       endpointUpdateUser,
       projectPath,
       dismissEndpoint,
@@ -47,7 +46,6 @@ export default {
       endpointMetadata,
       endpointBatch,
       endpointCoverage,
-      endpointCodequality,
       endpointUpdateUser,
       projectPath,
       dismissEndpoint,
@@ -91,10 +89,6 @@ export default {
     Object.assign(state, { coverageFiles });
   },
 
-  [types.SET_CODEQUALITY_DATA](state, codequalityDiffData) {
-    Object.assign(state, { codequalityDiff: codequalityDiffData });
-  },
-
   [types.RENDER_FILE](state, file) {
     renderFile(file);
   },

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

@@ -12,7 +12,7 @@ export default {
   i18n: {
     badgeTitle: __('Code Quality'),
     badgeTooltip: __(
-      'The merge request has been updated, and the number of code quality violations in this file has changed.',
+      'The merge request has made changes to this file that affect the number of code quality violations in it.',
     ),
   },
 };

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

+import Visibility from 'visibilityjs';
+import createFlash from '~/flash';
+import axios from '~/lib/utils/axios_utils';
+import httpStatusCodes from '~/lib/utils/http_status';
+import Poll from '~/lib/utils/poll';
+import { __ } from '~/locale';
+
+import * as types from './mutation_types';
+
+export * from '~/diffs/store/actions';
+
+let codequalityPoll;
+
+export const setCodequalityEndpoint = ({ commit }, endpoint) => {
+  commit(types.SET_CODEQUALITY_ENDPOINT, endpoint);
+};
+
+export const clearCodequalityPoll = () => {
+  codequalityPoll = null;
+};
+
+export const stopCodequalityPolling = () => {
+  if (codequalityPoll) codequalityPoll.stop();
+};
+
+export const restartCodequalityPolling = () => {
+  if (codequalityPoll) codequalityPoll.restart();
+};
+
+export const fetchCodequality = ({ commit, state, dispatch }) => {
+  codequalityPoll = 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);
+
+        dispatch('stopCodequalityPolling');
+      }
+    },
+    errorCallback: () =>
+      createFlash(__('Something went wrong on our end while loading the code quality diff.')),
+  });
+
+  if (!Visibility.hidden()) {
+    codequalityPoll.makeRequest();
+  }
+
+  Visibility.change(() => {
+    if (!Visibility.hidden()) {
+      dispatch('restartCodequalityPolling');
+    } else {
+      dispatch('stopCodequalityPolling');
+    }
+  });
+};

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

+import createStateCE from '~/diffs/store/modules/diff_state';
+
+export default () => ({
+  ...createStateCE(),
+
+  endpointCodequality: '',
+  codequalityDiff: {},
+});

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

+export const SET_CODEQUALITY_ENDPOINT = 'SET_CODEQUALITY_ENDPOINT';
+export const SET_CODEQUALITY_DATA = 'SET_CODEQUALITY_DATA';

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

+import CEMutations from '~/diffs/store/mutations';
+
+import * as types from './mutation_types';
+
+export default {
+  ...CEMutations,
+
+  [types.SET_CODEQUALITY_ENDPOINT](state, endpoint) {
+    Object.assign(state, { endpointCodequality: endpoint });
+  },
+
+  [types.SET_CODEQUALITY_DATA](state, codequalityDiffData) {
+    Object.assign(state, { codequalityDiff: codequalityDiffData });
+  },
+};

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

-import { shallowMount, createLocalVue } from '@vue/test-utils';
+import { mount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
 
 import CodeQualityBadge from 'ee/diffs/components/code_quality_badge.vue';
@@ -9,7 +9,7 @@ import createDiffsStore from '~/diffs/store/modules';
 
 const getReadableFile = () => JSON.parse(JSON.stringify(diffFileMockDataReadable));
 
-function createComponent({ first = false, last = false, options = {}, props = {} }) {
+function createComponent({ withCodequality = true }) {
   const file = getReadableFile();
   const localVue = createLocalVue();
 
@@ -23,18 +23,31 @@ function createComponent({ first = false, last = false, options = {}, props = {}
 
   store.state.diffs.diffFiles = [file];
 
-  const wrapper = shallowMount(DiffFileComponent, {
+  if (withCodequality) {
+    store.state.diffs.codequalityDiff = {
+      files: {
+        [file.file_path]: [
+          { line: 1, description: 'Unexpected alert.', severity: 'minor' },
+          {
+            line: 3,
+            description: 'Arrow function has too many statements (52). Maximum allowed is 30.',
+            severity: 'minor',
+          },
+        ],
+      },
+    };
+  }
+
+  const wrapper = mount(DiffFileComponent, {
     store,
     localVue,
     propsData: {
       file,
       canCurrentUserFork: false,
       viewDiffsFileByFile: false,
-      isFirstFile: first,
-      isLastFile: last,
-      ...props,
+      isFirstFile: false,
+      isLastFile: false,
     },
-    ...options,
   });
 
   return {
@@ -52,27 +65,24 @@ describe('EE DiffFile', () => {
   });
 
   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',
-            },
-          ],
-        },
-      }));
+    describe('when there is diff data for the file', () => {
+      beforeEach(() => {
+        ({ wrapper } = createComponent({ withCodequality: true }));
+      });
 
-      expect(wrapper.find(CodeQualityBadge)).toExist();
+      it('shows the code quality badge', () => {
+        expect(wrapper.find(CodeQualityBadge).exists()).toBe(true);
+      });
     });
 
-    it('is not shown when there is no diff data for the file', () => {
-      ({ wrapper } = createComponent({}));
+    describe('when there is no diff data for the file', () => {
+      beforeEach(() => {
+        ({ wrapper } = createComponent({ withCodequality: false }));
+      });
 
-      expect(wrapper.find(CodeQualityBadge)).toExist();
+      it('does not show the code quality badge', () => {
+        expect(wrapper.find(CodeQualityBadge).exists()).toBe(false);
+      });
     });
   });
 });

ee/spec/frontend/diffs/store/actions_spec.js

+import MockAdapter from 'axios-mock-adapter';
+import {
+  setCodequalityEndpoint,
+  clearCodequalityPoll,
+  stopCodequalityPolling,
+  fetchCodequality,
+} from 'ee/diffs/store/actions';
+import * as types from 'ee/diffs/store/mutation_types';
+import testAction from 'helpers/vuex_action_helper';
+import createFlash from '~/flash';
+import axios from '~/lib/utils/axios_utils';
+
+jest.mock('~/flash');
+
+describe('EE DiffsStoreActions', () => {
+  describe('setCodequalityEndpoint', () => {
+    it('should set given endpoint', (done) => {
+      const endpoint = '/codequality_mr_diff.json';
+
+      testAction(
+        setCodequalityEndpoint,
+        { endpoint },
+        {},
+        [{ type: types.SET_CODEQUALITY_ENDPOINT, payload: { endpoint } }],
+        [],
+        done,
+      );
+    });
+  });
+
+  describe('fetchCodequality', () => {
+    let mock;
+    const endpoint = '/codequality_mr_diff.json';
+
+    beforeEach(() => {
+      mock = new MockAdapter(axios);
+    });
+
+    afterEach(() => {
+      stopCodequalityPolling();
+      clearCodequalityPoll();
+    });
+
+    it('should commit SET_CODEQUALITY_DATA with received response and stop polling', (done) => {
+      const data = {
+        files: { 'app.js': [{ line: 1, description: 'Unexpected alert.', severity: 'minor' }] },
+      };
+
+      mock.onGet(endpoint).reply(200, { data });
+
+      testAction(
+        fetchCodequality,
+        {},
+        { endpointCodequality: endpoint },
+        [{ type: types.SET_CODEQUALITY_DATA, payload: { data } }],
+        [{ type: 'stopCodequalityPolling' }],
+        done,
+      );
+    });
+
+    it('should show flash on API error', (done) => {
+      mock.onGet(endpoint).reply(400);
+
+      testAction(fetchCodequality, {}, { endpoint }, [], [], () => {
+        expect(createFlash).toHaveBeenCalledTimes(1);
+        expect(createFlash).toHaveBeenCalledWith(expect.stringMatching('Something went wrong'));
+        done();
+      });
+    });
+  });
+});

ee/spec/frontend/diffs/store/mutations_spec.js

+import * as types from 'ee/diffs/store/mutation_types';
+import mutations from 'ee/diffs/store/mutations';
+
+describe('EE DiffsStoreMutations', () => {
+  describe('SET_CODEQUALITY_ENDPOINT', () => {
+    it('sets the endpoint into state', () => {
+      const state = {};
+      const endpoint = '/codequality_mr_diff.json';
+
+      mutations[types.SET_CODEQUALITY_ENDPOINT](state, endpoint);
+
+      expect(state.endpointCodequality).toEqual(endpoint);
+    });
+  });
+
+  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);
+    });
+  });
+});

locale/gitlab.pot

@@ -29017,6 +29017,9 @@ msgstr ""
 msgid "Something went wrong on our end"
 msgstr ""
 
+msgid "Something went wrong on our end while loading the code quality diff."
+msgstr ""
+
 msgid "Something went wrong on our end."
 msgstr ""
 
@@ -30988,7 +30991,7 @@ 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."
+msgid "The merge request has made changes to this file that affect the number of code quality violations in it."
 msgstr ""
 
 msgid "The metric must be one of %{metrics}."

spec/frontend/diffs/store/actions_spec.js

@@ -17,9 +17,6 @@ import {
   fetchDiffFilesBatch,
   fetchDiffFilesMeta,
   fetchCoverageFiles,
-  clearEtagPoll,
-  stopCodequalityPolling,
-  fetchCodequality,
   assignDiscussionsToDiff,
   removeDiscussionsFromDiff,
   startRenderDiffsQueue,
@@ -101,7 +98,6 @@ 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;
@@ -113,7 +109,6 @@ describe('DiffsStoreActions', () => {
           endpointBatch,
           endpointMetadata,
           endpointCoverage,
-          endpointCodequality,
           projectPath,
           dismissEndpoint,
           showSuggestPopover,
@@ -123,7 +118,6 @@ describe('DiffsStoreActions', () => {
           endpointBatch: '',
           endpointMetadata: '',
           endpointCoverage: '',
-          endpointCodequality: '',
           projectPath: '',
           dismissEndpoint: '',
           showSuggestPopover: true,
@@ -136,7 +130,6 @@ describe('DiffsStoreActions', () => {
               endpointMetadata,
               endpointBatch,
               endpointCoverage,
-              endpointCodequality,
               projectPath,
               dismissEndpoint,
               showSuggestPopover,
@@ -306,47 +299,6 @@ 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,26 +376,6 @@ 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

@@ -115,19 +115,6 @@ 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 = {};