Added DIff Image Viewertz-refactored-mr-imagediffs

Added tests for image diff viewer in MR

Added project Path to data setup, updated actions + views based on MR feedback

14 files changed, 177 insertions, 39 deletions

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 82ca10f4163..ecf54236ecf 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/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(__('Something went wrong on our end. Please try again!'));
+    });
   },
   created() {
     this.adjustView();
   },
   methods: {
-    ...mapActions(['setEndpoint', 'fetchDiffFiles']),
+    ...mapActions(['setBaseConfig', 'fetchDiffFiles']),
     setActive(filePath) {
       this.activeFile = filePath;
     },
diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue
index adcd22f7876..a831af53242 100644
--- a/app/assets/javascripts/diffs/components/diff_content.vue
+++ b/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,15 @@ 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;
+    },
   },
 };
 </script>
@@ -23,16 +34,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="diffFile.text === true">
+        <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>
diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue
index 0ed3dc7f3ad..d71c4561abc 100644
--- a/app/assets/javascripts/diffs/components/inline_diff_view.vue
+++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue
@@ -36,7 +36,8 @@ 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"
+    data-test="diff-inline-view">
     <tbody>
       <template
         v-for="(line, index) in normalizedDiffLines"
diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js
index f6840f87034..aae89109c27 100644
--- a/app/assets/javascripts/diffs/index.js
+++ b/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',
         },
       });
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index f8089b314d3..bf188a44022 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/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,
diff --git a/app/assets/javascripts/diffs/store/modules/index.js b/app/assets/javascripts/diffs/store/modules/index.js
index 882a098c977..94caa131506 100644
--- a/app/assets/javascripts/diffs/store/modules/index.js
+++ b/app/assets/javascripts/diffs/store/modules/index.js
@@ -13,6 +13,7 @@ export default {
   state: {
     isLoading: true,
     endpoint: '',
+    basePath: '',
     commit: null,
     diffFiles: [],
     mergeRequestDiffs: [],
diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js
index a65b205b8e7..63e9239dce4 100644
--- a/app/assets/javascripts/diffs/store/mutation_types.js
+++ b/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';
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index fd9ea73e33d..339a33f8b71 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/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,
diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue
index 2c47f5b9b35..8a1a38e1d7c 100644
--- a/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue
+++ b/app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue
@@ -45,11 +45,18 @@ export default {
           return DownloadDiffViewer;
       }
     },
+    basePath() {
+      // We might get the project path from rails with the relative url already setup
+      if (this.projectPath.indexOf('/') === 0) {
+        return '';
+      }
+      return `${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}`;
     },
   },
 };
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index 65add153606..787dcaab9b6 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -501,6 +501,10 @@
   border-bottom: 0;
 }
 
+.merge-request-details .preview-container .file-container .file-content img {
+  max-height: 50vh;
+}
+
 .diff-stats-summary-toggler {
   padding: 0;
   background-color: transparent;
diff --git a/app/views/projects/merge_requests/show.html.haml b/app/views/projects/merge_requests/show.html.haml
index 4fe0ae17ec5..b23baa22d8b 100644
--- a/app/views/projects/merge_requests/show.html.haml
+++ b/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
diff --git a/spec/javascripts/diffs/components/diff_content_spec.js b/spec/javascripts/diffs/components/diff_content_spec.js
index 7237274eb43..527e76c06f7 100644
--- a/spec/javascripts/diffs/components/diff_content_spec.js
+++ b/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('[data-test=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('[data-test=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('[data-test=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();
+        });
+      });
+    });
+  });
+});
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index e61780c9928..f0bd098f698 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/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,
       );
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index 5f1a6e9def7..02836fcaeea 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/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);
     });
   });