Merge branch '60856-deleting-binary-file' into 'master'

Resolve "Removing an image via Web IDE shows binary data"

Closes #60856

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

app/assets/javascripts/ide/components/repo_editor.vue

@@ -144,7 +144,9 @@ export default {
       'triggerFilesChange',
     ]),
     initEditor() {
-      if (this.shouldHideEditor) return;
+      if (this.shouldHideEditor && (this.file.content || this.file.raw)) {
+        return;
+      }
 
       this.editor.clearEditor();
 

app/assets/javascripts/ide/lib/files.js

@@ -77,6 +77,7 @@ export const decorateFiles = ({
     const fileFolder = parent && insertParent(parent);
 
     if (name) {
+      const previewMode = viewerInformationForPath(name);
       parentPath = fileFolder && fileFolder.path;
 
       file = decorateData({
@@ -92,9 +93,9 @@ export const decorateFiles = ({
         changed: tempFile,
         content,
         base64,
-        binary,
+        binary: (previewMode && previewMode.binary) || binary,
         rawPath,
-        previewMode: viewerInformationForPath(name),
+        previewMode,
         parentPath,
       });
 

app/assets/javascripts/ide/stores/mutations/file.js

 import * as types from '../mutation_types';
 import { sortTree } from '../utils';
 import { diffModes } from '../../constants';
+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 
 export default {
   [types.SET_FILE_ACTIVE](state, { path, active }) {
@@ -35,19 +36,18 @@ export default {
     }
   },
   [types.SET_FILE_DATA](state, { data, file }) {
-    Object.assign(state.entries[file.path], {
-      id: data.id,
-      blamePath: data.blame_path,
-      commitsPath: data.commits_path,
-      permalink: data.permalink,
-      rawPath: data.raw_path,
-      binary: data.binary,
-      renderError: data.render_error,
-      raw: (state.entries[file.path] && state.entries[file.path].raw) || null,
-      baseRaw: null,
-      html: data.html,
-      size: data.size,
-      lastCommitSha: data.last_commit_sha,
+    const stateEntry = state.entries[file.path];
+    const stagedFile = state.stagedFiles.find(f => f.path === file.path);
+    const openFile = state.openFiles.find(f => f.path === file.path);
+    const changedFile = state.changedFiles.find(f => f.path === file.path);
+
+    [stateEntry, stagedFile, openFile, changedFile].forEach(f => {
+      if (f) {
+        Object.assign(f, convertObjectPropsToCamelCase(data, { dropKeys: ['raw', 'baseRaw'] }), {
+          raw: (stateEntry && stateEntry.raw) || null,
+          baseRaw: null,
+        });
+      }
     });
   },
   [types.SET_FILE_RAW_DATA](state, { file, raw }) {

app/assets/javascripts/vue_shared/components/content_viewer/lib/viewer_utils.js

@@ -3,6 +3,7 @@ import { __ } from '~/locale';
 const viewers = {
   image: {
     id: 'image',
+    binary: true,
   },
   markdown: {
     id: 'markdown',

changelogs/unreleased/60856-deleting-binary-file.yml

+---
+title: Removing an image should not output binary data
+merge_request: 30314
+author:
+type: fixed

spec/frontend/ide/lib/files_spec.js

@@ -12,6 +12,7 @@ const createEntries = paths => {
 
     const { name, parent } = splitParent(path);
     const parentEntry = acc[parent];
+    const previewMode = viewerInformationForPath(name);
 
     acc[path] = {
       ...decorateData({
@@ -22,7 +23,8 @@ const createEntries = paths => {
         path,
         url: createUrl(`/${TEST_PROJECT_ID}/${type}/${TEST_BRANCH_ID}/-/${escapeFileUrl(path)}`),
         type,
-        previewMode: viewerInformationForPath(path),
+        previewMode,
+        binary: (previewMode && previewMode.binary) || false,
         parentPath: parent,
         parentTreeUrl: parentEntry
           ? parentEntry.url

spec/javascripts/ide/components/repo_editor_spec.js

@@ -30,6 +30,7 @@ describe('RepoEditor', () => {
     Vue.set(vm.$store.state.entries, f.path, f);
 
     spyOn(vm, 'getFileData').and.returnValue(Promise.resolve());
+    spyOn(vm, 'getRawFileData').and.returnValue(Promise.resolve());
 
     vm.$mount();
 
@@ -407,6 +408,44 @@ describe('RepoEditor', () => {
     });
   });
 
+  describe('initEditor', () => {
+    beforeEach(() => {
+      spyOn(vm.editor, 'createInstance');
+      spyOnProperty(vm, 'shouldHideEditor').and.returnValue(true);
+    });
+
+    it('is being initialised for files without content even if shouldHideEditor is `true`', done => {
+      vm.file.content = '';
+      vm.file.raw = '';
+
+      vm.initEditor();
+      vm.$nextTick()
+        .then(() => {
+          expect(vm.getFileData).toHaveBeenCalled();
+          expect(vm.getRawFileData).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('does not initialize editor for files already with content', done => {
+      expect(vm.getFileData.calls.count()).toEqual(1);
+      expect(vm.getRawFileData.calls.count()).toEqual(1);
+
+      vm.file.content = 'foo';
+
+      vm.initEditor();
+      vm.$nextTick()
+        .then(() => {
+          expect(vm.getFileData.calls.count()).toEqual(1);
+          expect(vm.getRawFileData.calls.count()).toEqual(1);
+          expect(vm.editor.createInstance).not.toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
   it('calls removePendingTab when old file is pending', done => {
     spyOnProperty(vm, 'shouldHideEditor').and.returnValue(true);
     spyOn(vm, 'removePendingTab');
@@ -416,6 +455,7 @@ describe('RepoEditor', () => {
     vm.$nextTick()
       .then(() => {
         vm.file = file('testing');
+        vm.file.content = 'foo'; // need to prevent full cycle of initEditor
 
         return vm.$nextTick();
       })

spec/javascripts/ide/stores/mutations/file_spec.js

@@ -83,6 +83,26 @@ describe('IDE store file mutations', () => {
       expect(localFile.raw).toBeNull();
       expect(localFile.baseRaw).toBeNull();
     });
+
+    it('sets extra file data to all arrays concerned', () => {
+      localState.stagedFiles = [localFile];
+      localState.changedFiles = [localFile];
+      localState.openFiles = [localFile];
+
+      const rawPath = 'foo/bar/blah.md';
+
+      mutations.SET_FILE_DATA(localState, {
+        data: {
+          raw_path: rawPath,
+        },
+        file: localFile,
+      });
+
+      expect(localState.stagedFiles[0].rawPath).toEqual(rawPath);
+      expect(localState.changedFiles[0].rawPath).toEqual(rawPath);
+      expect(localState.openFiles[0].rawPath).toEqual(rawPath);
+      expect(localFile.rawPath).toEqual(rawPath);
+    });
   });
 
   describe('SET_FILE_RAW_DATA', () => {