Merge branch 'diff-empty-state-fixes' into 'master'

Fix diff changes empty state

Closes #48635

See merge request gitlab-org/gitlab-ce!23767

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

@@ -42,6 +42,11 @@ export default {
       type: Object,
       required: true,
     },
+    changesEmptyStateIllustration: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   data() {
     return {
@@ -63,7 +68,7 @@ export default {
       plainDiffPath: state => state.diffs.plainDiffPath,
       emailPatchPath: state => state.diffs.emailPatchPath,
     }),
-    ...mapState('diffs', ['showTreeList', 'isLoading']),
+    ...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']),
     ...mapGetters('diffs', ['isParallelView']),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     targetBranch() {
@@ -79,6 +84,13 @@ export default {
     showCompareVersions() {
       return this.mergeRequestDiffs && this.mergeRequestDiff;
     },
+    renderDiffFiles() {
+      return (
+        this.diffFiles.length > 0 ||
+        (this.startVersion &&
+          this.startVersion.version_index === this.mergeRequestDiff.version_index)
+      );
+    },
   },
   watch: {
     diffViewType() {
@@ -191,7 +203,7 @@ export default {
         <div v-show="showTreeList" class="diff-tree-list"><tree-list /></div>
         <div class="diff-files-holder">
           <commit-widget v-if="commit" :commit="commit" />
-          <template v-if="diffFiles.length > 0">
+          <template v-if="renderDiffFiles">
             <diff-file
               v-for="file in diffFiles"
               :key="file.newPath"
@@ -199,7 +211,7 @@ export default {
               :can-current-user-fork="canCurrentUserFork"
             />
           </template>
-          <no-changes v-else />
+          <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" />
         </div>
       </div>
     </div>

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

 <script>
-import { mapState } from 'vuex';
-import emptyImage from '~/../../views/shared/icons/_mr_widget_empty_state.svg';
+import { mapGetters } from 'vuex';
+import _ from 'underscore';
+import { GlButton } from '@gitlab/ui';
+import { __, sprintf } from '~/locale';
 
 export default {
-  data() {
-    return {
-      emptyImage,
-    };
+  components: {
+    GlButton,
+  },
+  props: {
+    changesEmptyStateIllustration: {
+      type: String,
+      required: true,
+    },
   },
   computed: {
-    ...mapState({
-      sourceBranch: state => state.notes.noteableData.source_branch,
-      targetBranch: state => state.notes.noteableData.target_branch,
-      newBlobPath: state => state.notes.noteableData.new_blob_path,
-    }),
+    ...mapGetters(['getNoteableData']),
+    emptyStateText() {
+      return sprintf(
+        __(
+          'No changes between %{ref_start}%{source_branch}%{ref_end} and %{ref_start}%{target_branch}%{ref_end}',
+        ),
+        {
+          ref_start: '<span class="ref-name">',
+          ref_end: '</span>',
+          source_branch: _.escape(this.getNoteableData.source_branch),
+          target_branch: _.escape(this.getNoteableData.target_branch),
+        },
+        false,
+      );
+    },
   },
 };
 </script>
 
 <template>
-  <div class="row empty-state nothing-here-block">
-    <div class="col-xs-12">
-      <div class="svg-content"><span v-html="emptyImage"></span></div>
+  <div class="row empty-state">
+    <div class="col-12">
+      <div class="svg-content svg-250"><img :src="changesEmptyStateIllustration" /></div>
     </div>
-    <div class="col-xs-12">
+    <div class="col-12">
       <div class="text-content text-center">
-        No changes between <span class="ref-name">{{ sourceBranch }}</span> and
-        <span class="ref-name">{{ targetBranch }}</span>
+        <span v-html="emptyStateText"></span>
         <div class="text-center">
-          <a :href="newBlobPath" class="btn btn-success"> {{ __('Create commit') }} </a>
+          <gl-button :href="getNoteableData.new_blob_path" variant="success">{{
+            __('Create commit')
+          }}</gl-button>
         </div>
       </div>
     </div>

app/assets/javascripts/diffs/index.js

@@ -17,6 +17,7 @@ export default function initDiffsApp(store) {
         endpoint: dataset.endpoint,
         projectPath: dataset.projectPath,
         currentUser: JSON.parse(dataset.currentUserData) || {},
+        changesEmptyStateIllustration: dataset.changesEmptyStateIllustration,
       };
     },
     computed: {
@@ -31,6 +32,7 @@ export default function initDiffsApp(store) {
           currentUser: this.currentUser,
           projectPath: this.projectPath,
           shouldShow: this.activeTab === 'diffs',
+          changesEmptyStateIllustration: this.changesEmptyStateIllustration,
         },
       });
     },

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

@@ -77,7 +77,8 @@
       #js-diffs-app.diffs.tab-pane{ data: { "is-locked" => @merge_request.discussion_locked?,
         endpoint: diffs_project_merge_request_path(@project, @merge_request, 'json', request.query_parameters),
         current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json,
-        project_path: project_path(@merge_request.project)} }
+        project_path: project_path(@merge_request.project),
+        changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg') } }
 
     .mr-loading-status
       = spinner

changelogs/unreleased/diff-empty-state-fixes.yml

+---
+title: Fixed merge request diffs empty states
+merge_request:
+author:
+type: fixed

locale/gitlab.pot

@@ -4368,6 +4368,9 @@ msgstr ""
 msgid "No changes"
 msgstr ""
 
+msgid "No changes between %{ref_start}%{source_branch}%{ref_end} and %{ref_start}%{target_branch}%{ref_end}"
+msgstr ""
+
 msgid "No connection could be made to a Gitaly Server, please check your logs!"
 msgstr ""
 

spec/javascripts/diffs/components/app_spec.js

-import Vue from 'vue';
-import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import Vuex from 'vuex';
+import { shallowMount, createLocalVue } from '@vue/test-utils';
 import { TEST_HOST } from 'spec/test_constants';
 import App from '~/diffs/components/app.vue';
+import NoChanges from '~/diffs/components/no_changes.vue';
+import DiffFile from '~/diffs/components/diff_file.vue';
 import createDiffsStore from '../create_diffs_store';
 
 describe('diffs/components/app', () => {
   const oldMrTabs = window.mrTabs;
-  const Component = Vue.extend(App);
-
+  let store;
   let vm;
 
-  beforeEach(() => {
-    // setup globals (needed for component to mount :/)
-    window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']);
-    window.mrTabs.expandViewContainer = jasmine.createSpy();
-    window.location.hash = 'ABC_123';
+  function createComponent(props = {}, extendStore = () => {}) {
+    const localVue = createLocalVue();
+
+    localVue.use(Vuex);
 
-    // setup component
-    const store = createDiffsStore();
+    store = createDiffsStore();
     store.state.diffs.isLoading = false;
 
-    vm = mountComponentWithStore(Component, {
-      store,
-      props: {
+    extendStore(store);
+
+    vm = shallowMount(localVue.extend(App), {
+      localVue,
+      propsData: {
         endpoint: `${TEST_HOST}/diff/endpoint`,
         projectPath: 'namespace/project',
         currentUser: {},
+        changesEmptyStateIllustration: '',
+        ...props,
       },
+      store,
     });
+  }
+
+  beforeEach(() => {
+    // setup globals (needed for component to mount :/)
+    window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']);
+    window.mrTabs.expandViewContainer = jasmine.createSpy();
+    window.location.hash = 'ABC_123';
   });
 
   afterEach(() => {
@@ -35,21 +46,53 @@ describe('diffs/components/app', () => {
     window.mrTabs = oldMrTabs;
 
     // reset component
-    vm.$destroy();
+    vm.destroy();
   });
 
   it('does not show commit info', () => {
-    expect(vm.$el).not.toContainElement('.blob-commit-info');
+    createComponent();
+
+    expect(vm.contains('.blob-commit-info')).toBe(false);
   });
 
   it('sets highlighted row if hash exists in location object', done => {
-    vm.$props.shouldShow = true;
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$store.state.diffs.highlightedRow).toBe('ABC_123');
-      })
-      .then(done)
-      .catch(done.fail);
+    createComponent({
+      shouldShow: true,
+    });
+
+    // Component uses $nextTick so we wait until that has finished
+    setTimeout(() => {
+      expect(store.state.diffs.highlightedRow).toBe('ABC_123');
+
+      done();
+    });
+  });
+
+  describe('empty state', () => {
+    it('renders empty state when no diff files exist', () => {
+      createComponent();
+
+      expect(vm.contains(NoChanges)).toBe(true);
+    });
+
+    it('does not render empty state when diff files exist', () => {
+      createComponent({}, () => {
+        store.state.diffs.diffFiles.push({
+          id: 1,
+        });
+      });
+
+      expect(vm.contains(NoChanges)).toBe(false);
+      expect(vm.findAll(DiffFile).length).toBe(1);
+    });
+
+    it('does not render empty state when versions match', () => {
+      createComponent({}, () => {
+        store.state.diffs.startVersion = { version_index: 1 };
+        store.state.diffs.mergeRequestDiff = { version_index: 1 };
+      });
+
+      expect(vm.contains(NoChanges)).toBe(false);
+    });
   });
 });

spec/javascripts/diffs/components/no_changes_spec.js

-// TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import Vuex from 'vuex';
+import { createStore } from '~/mr_notes/stores';
+import NoChanges from '~/diffs/components/no_changes.vue';
+
+describe('Diff no changes empty state', () => {
+  let vm;
+
+  function createComponent(extendStore = () => {}) {
+    const localVue = createLocalVue();
+    localVue.use(Vuex);
+
+    const store = createStore();
+    extendStore(store);
+
+    vm = shallowMount(localVue.extend(NoChanges), {
+      localVue,
+      store,
+      propsData: {
+        changesEmptyStateIllustration: '',
+      },
+    });
+  }
+
+  afterEach(() => {
+    vm.destroy();
+  });
+
+  it('prevents XSS', () => {
+    createComponent(store => {
+      // eslint-disable-next-line no-param-reassign
+      store.state.notes.noteableData = {
+        source_branch: '<script>alert("test");</script>',
+        target_branch: '<script>alert("test");</script>',
+      };
+    });
+
+    expect(vm.contains('script')).toBe(false);
+  });
+});