Merge branch 'tz-mr-vue-imagediff' into 'master'

Added Diff Viewer to new VUE based MR page

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

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

@@ -26,6 +26,10 @@ export default {
       type: String,
       required: true,
     },
+    projectPath: {
+      type: String,
+      required: true,
+    },
     shouldShow: {
       type: Boolean,
       required: false,
@@ -94,18 +98,16 @@ export default {
     },
   },
   mounted() {
-    this.setEndpoint(this.endpoint);
-    this
-      .fetchDiffFiles()
-      .catch(() => {
-        createFlash(__('Something went wrong on our end. Please try again!'));
-      });
+    this.setBaseConfig({ endpoint: this.endpoint, projectPath: this.projectPath });
+    this.fetchDiffFiles().catch(() => {
+      createFlash(__('Fetching diff files failed. Please reload the page to try again!'));
+    });
   },
   created() {
     this.adjustView();
   },
   methods: {
-    ...mapActions(['setEndpoint', 'fetchDiffFiles']),
+    ...mapActions(['setBaseConfig', 'fetchDiffFiles']),
     setActive(filePath) {
       this.activeFile = filePath;
     },

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

@@ -1,5 +1,7 @@
 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapState } from 'vuex';
+import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue';
+import { diffModes } from '~/ide/constants';
 import InlineDiffView from './inline_diff_view.vue';
 import ParallelDiffView from './parallel_diff_view.vue';
 
@@ -7,6 +9,7 @@ export default {
   components: {
     InlineDiffView,
     ParallelDiffView,
+    DiffViewer,
   },
   props: {
     diffFile: {
@@ -15,7 +18,18 @@ export default {
     },
   },
   computed: {
+    ...mapState({
+      projectPath: state => state.diffs.projectPath,
+      endpoint: state => state.diffs.endpoint,
+    }),
     ...mapGetters(['isInlineView', 'isParallelView']),
+    diffMode() {
+      const diffModeKey = Object.keys(diffModes).find(key => this.diffFile[`${key}File`]);
+      return diffModes[diffModeKey] || diffModes.replaced;
+    },
+    isTextFile() {
+      return this.diffFile.text;
+    },
   },
 };
 </script>
@@ -23,16 +37,26 @@ export default {
 <template>
   <div class="diff-content">
     <div class="diff-viewer">
-      <inline-diff-view
-        v-if="isInlineView"
-        :diff-file="diffFile"
-        :diff-lines="diffFile.highlightedDiffLines || []"
-      />
-      <parallel-diff-view
-        v-if="isParallelView"
-        :diff-file="diffFile"
-        :diff-lines="diffFile.parallelDiffLines || []"
-      />
+      <template v-if="isTextFile">
+        <inline-diff-view
+          v-if="isInlineView"
+          :diff-file="diffFile"
+          :diff-lines="diffFile.highlightedDiffLines || []"
+        />
+        <parallel-diff-view
+          v-else-if="isParallelView"
+          :diff-file="diffFile"
+          :diff-lines="diffFile.parallelDiffLines || []"
+        />
+      </template>
+      <diff-viewer
+        v-else
+        :diff-mode="diffMode"
+        :new-path="diffFile.newPath"
+        :new-sha="diffFile.diffRefs.headSha"
+        :old-path="diffFile.oldPath"
+        :old-sha="diffFile.diffRefs.baseSha"
+        :project-path="projectPath"/>
     </div>
   </div>
 </template>

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

@@ -36,7 +36,7 @@ export default {
   <table
     :class="userColorScheme"
     :data-commit-id="commitId"
-    class="code diff-wrap-lines js-syntax-highlight text-file">
+    class="code diff-wrap-lines js-syntax-highlight text-file js-diff-inline-view">
     <tbody>
       <template
         v-for="(line, index) in normalizedDiffLines"

app/assets/javascripts/diffs/index.js

@@ -16,6 +16,7 @@ export default function initDiffsApp(store) {
 
       return {
         endpoint: dataset.endpoint,
+        projectPath: dataset.projectPath,
         currentUser: convertObjectPropsToCamelCase(JSON.parse(dataset.currentUserData), {
           deep: true,
         }),
@@ -31,6 +32,7 @@ export default function initDiffsApp(store) {
         props: {
           endpoint: this.endpoint,
           currentUser: this.currentUser,
+          projectPath: this.projectPath,
           shouldShow: this.activeTab === 'diffs',
         },
       });

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

@@ -10,8 +10,9 @@ import {
   DIFF_VIEW_COOKIE_NAME,
 } from '../constants';
 
-export const setEndpoint = ({ commit }, endpoint) => {
-  commit(types.SET_ENDPOINT, endpoint);
+export const setBaseConfig = ({ commit }, options) => {
+  const { endpoint, projectPath } = options;
+  commit(types.SET_BASE_CONFIG, { endpoint, projectPath });
 };
 
 export const setLoadingState = ({ commit }, state) => {
@@ -86,7 +87,7 @@ export const expandAllFiles = ({ commit }) => {
 };
 
 export default {
-  setEndpoint,
+  setBaseConfig,
   setLoadingState,
   fetchDiffFiles,
   setInlineDiffViewType,

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

@@ -13,6 +13,7 @@ export default {
   state: {
     isLoading: true,
     endpoint: '',
+    basePath: '',
     commit: null,
     diffFiles: [],
     mergeRequestDiffs: [],

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

@@ -1,4 +1,4 @@
-export const SET_ENDPOINT = 'SET_ENDPOINT';
+export const SET_BASE_CONFIG = 'SET_BASE_CONFIG';
 export const SET_LOADING = 'SET_LOADING';
 export const SET_DIFF_DATA = 'SET_DIFF_DATA';
 export const SET_DIFF_FILES = 'SET_DIFF_FILES';

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

@@ -5,8 +5,9 @@ import { findDiffFile, addLineReferences, removeMatchLine, addContextLines } fro
 import * as types from './mutation_types';
 
 export default {
-  [types.SET_ENDPOINT](state, endpoint) {
-    Object.assign(state, { endpoint });
+  [types.SET_BASE_CONFIG](state, options) {
+    const { endpoint, projectPath } = options;
+    Object.assign(state, { endpoint, projectPath });
   },
 
   [types.SET_LOADING](state, isLoading) {
@@ -73,7 +74,7 @@ export default {
   [types.EXPAND_ALL_FILES](state) {
     const diffFiles = [];
 
-    state.diffFiles.forEach((file) => {
+    state.diffFiles.forEach(file => {
       diffFiles.push({
         ...file,
         collapsed: false,

app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue

@@ -45,11 +45,15 @@ export default {
           return DownloadDiffViewer;
       }
     },
+    basePath() {
+      // We might get the project path from rails with the relative url already setup
+      return this.projectPath.indexOf('/') === 0 ? '' : `${gon.relative_url_root}/`;
+    },
     fullOldPath() {
-      return `${gon.relative_url_root}/${this.projectPath}/raw/${this.oldSha}/${this.oldPath}`;
+      return `${this.basePath}${this.projectPath}/raw/${this.oldSha}/${this.oldPath}`;
     },
     fullNewPath() {
-      return `${gon.relative_url_root}/${this.projectPath}/raw/${this.newSha}/${this.newPath}`;
+      return `${this.basePath}${this.projectPath}/raw/${this.newSha}/${this.newPath}`;
     },
   },
 };

app/assets/stylesheets/pages/diff.scss

@@ -502,6 +502,10 @@
   border-bottom: 0;
 }
 
+.merge-request-details .file-content.image_file img {
+  max-height: 50vh;
+}
+
 .diff-stats-summary-toggler {
   padding: 0;
   background-color: transparent;

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

@@ -73,7 +73,8 @@
           = render 'projects/commit/pipelines_list', disable_initialization: true, endpoint: pipelines_project_merge_request_path(@project, @merge_request)
       #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 } }
+        current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json,
+        project_path: project_path(@merge_request.project)} }
 
     .mr-loading-status
       = spinner

spec/javascripts/diffs/components/diff_content_spec.js

@@ -1 +1,95 @@
-// TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034
+import Vue from 'vue';
+import DiffContentComponent from '~/diffs/components/diff_content.vue';
+import store from '~/mr_notes/stores';
+import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
+import { GREEN_BOX_IMAGE_URL, RED_BOX_IMAGE_URL } from 'spec/test_constants';
+import diffFileMockData from '../mock_data/diff_file';
+
+describe('DiffContent', () => {
+  const Component = Vue.extend(DiffContentComponent);
+  let vm;
+  const getDiffFileMock = () => Object.assign({}, diffFileMockData);
+
+  beforeEach(() => {
+    vm = mountComponentWithStore(Component, {
+      store,
+      props: {
+        diffFile: getDiffFileMock(),
+      },
+    });
+  });
+
+  describe('text based files', () => {
+    it('should render diff inline view', done => {
+      vm.$store.state.diffs.diffViewType = 'inline';
+
+      vm.$nextTick(() => {
+        expect(vm.$el.querySelectorAll('.js-diff-inline-view').length).toEqual(1);
+
+        done();
+      });
+    });
+
+    it('should render diff parallel view', done => {
+      vm.$store.state.diffs.diffViewType = 'parallel';
+
+      vm.$nextTick(() => {
+        expect(vm.$el.querySelectorAll('.parallel').length).toEqual(18);
+
+        done();
+      });
+    });
+  });
+
+  describe('Non-Text diffs', () => {
+    beforeEach(() => {
+      vm.diffFile.text = false;
+    });
+
+    describe('image diff', () => {
+      beforeEach(() => {
+        vm.diffFile.newPath = GREEN_BOX_IMAGE_URL;
+        vm.diffFile.newSha = 'DEF';
+        vm.diffFile.oldPath = RED_BOX_IMAGE_URL;
+        vm.diffFile.oldSha = 'ABC';
+        vm.diffFile.viewPath = '';
+      });
+
+      it('should have image diff view in place', done => {
+        vm.$nextTick(() => {
+          expect(vm.$el.querySelectorAll('.js-diff-inline-view').length).toEqual(0);
+
+          expect(vm.$el.querySelectorAll('.diff-viewer .image').length).toEqual(1);
+
+          done();
+        });
+      });
+    });
+
+    describe('file diff', () => {
+      it('should have download buttons in place', done => {
+        const el = vm.$el;
+        vm.diffFile.newPath = 'test.abc';
+        vm.diffFile.newSha = 'DEF';
+        vm.diffFile.oldPath = 'test.abc';
+        vm.diffFile.oldSha = 'ABC';
+
+        vm.$nextTick(() => {
+          expect(el.querySelectorAll('.js-diff-inline-view').length).toEqual(0);
+
+          expect(el.querySelector('.deleted .file-info').textContent.trim()).toContain('test.abc');
+          expect(el.querySelector('.deleted .btn.btn-default').textContent.trim()).toContain(
+            'Download',
+          );
+
+          expect(el.querySelector('.added .file-info').textContent.trim()).toContain('test.abc');
+          expect(el.querySelector('.added .btn.btn-default').textContent.trim()).toContain(
+            'Download',
+          );
+
+          done();
+        });
+      });
+    });
+  });
+});

spec/javascripts/diffs/store/actions_spec.js

@@ -12,15 +12,16 @@ import axios from '~/lib/utils/axios_utils';
 import testAction from '../../helpers/vuex_action_helper';
 
 describe('DiffsStoreActions', () => {
-  describe('setEndpoint', () => {
-    it('should set given endpoint', done => {
+  describe('setBaseConfig', () => {
+    it('should set given endpoint and project path', done => {
       const endpoint = '/diffs/set/endpoint';
+      const projectPath = '/root/project';
 
       testAction(
-        actions.setEndpoint,
-        endpoint,
-        { endpoint: '' },
-        [{ type: types.SET_ENDPOINT, payload: endpoint }],
+        actions.setBaseConfig,
+        { endpoint, projectPath },
+        { endpoint: '', projectPath: '' },
+        [{ type: types.SET_BASE_CONFIG, payload: { endpoint, projectPath } }],
         [],
         done,
       );

spec/javascripts/diffs/store/mutations_spec.js

@@ -3,13 +3,15 @@ import * as types from '~/diffs/store/mutation_types';
 import { INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants';
 
 describe('DiffsStoreMutations', () => {
-  describe('SET_ENDPOINT', () => {
-    it('should set endpoint', () => {
+  describe('SET_BASE_CONFIG', () => {
+    it('should set endpoint and project path', () => {
       const state = {};
       const endpoint = '/diffs/endpoint';
+      const projectPath = '/root/project';
 
-      mutations[types.SET_ENDPOINT](state, endpoint);
+      mutations[types.SET_BASE_CONFIG](state, { endpoint, projectPath });
       expect(state.endpoint).toEqual(endpoint);
+      expect(state.projectPath).toEqual(projectPath);
     });
   });
 

spec/javascripts/helpers/init_vue_mr_page_helper.js

@@ -6,6 +6,7 @@ import diffFileMockData from '../diffs/mock_data/diff_file';
 
 export default function initVueMRPage() {
   const diffsAppEndpoint = '/diffs/app/endpoint';
+  const diffsAppProjectPath = 'testproject';
   const mrEl = document.createElement('div');
   mrEl.className = 'merge-request fixture-mr';
   mrEl.setAttribute('data-mr-action', 'diffs');
@@ -26,6 +27,7 @@ export default function initVueMRPage() {
   const diffsAppEl = document.createElement('div');
   diffsAppEl.id = 'js-diffs-app';
   diffsAppEl.setAttribute('data-endpoint', diffsAppEndpoint);
+  diffsAppEl.setAttribute('data-project-path', diffsAppProjectPath);
   diffsAppEl.setAttribute('data-current-user-data', JSON.stringify(userDataMock));
   document.body.appendChild(diffsAppEl);