Merge branch '29451-webide-currentsha' into 'master'

Use current commit SHA when requesting IDE file tree and contents

See merge request gitlab-org/gitlab!19348

app/assets/javascripts/ide/services/index.js

 import axios from '~/lib/utils/axios_utils';
+import { joinPaths } from '~/lib/utils/url_utility';
+import { escapeFileUrl } from '../stores/utils';
 import Api from '~/api';
 
 export default {
@@ -23,18 +25,25 @@ export default {
       .then(({ data }) => data);
   },
   getBaseRawFileData(file, sha) {
-    if (file.tempFile) {
-      return Promise.resolve(file.baseRaw);
-    }
+    if (file.tempFile || file.baseRaw) return Promise.resolve(file.baseRaw);
 
-    if (file.baseRaw) {
-      return Promise.resolve(file.baseRaw);
-    }
+    // if files are renamed, their base path has changed
+    const filePath =
+      file.mrChange && file.mrChange.renamed_file ? file.mrChange.old_path : file.path;
 
     return axios
-      .get(file.rawPath.replace(`/raw/${file.branchId}/${file.path}`, `/raw/${sha}/${file.path}`), {
+      .get(
+        joinPaths(
+          gon.relative_url_root || '/',
+          file.projectId,
+          'raw',
+          sha,
+          escapeFileUrl(filePath),
+        ),
+        {
           transformResponse: [f => f],
-      })
+        },
+      )
       .then(({ data }) => data);
   },
   getProjectData(namespace, project) {
@@ -58,8 +67,8 @@ export default {
   commit(projectId, payload) {
     return Api.commitMultiple(projectId, payload);
   },
-  getFiles(projectUrl, branchId) {
-    const url = `${projectUrl}/files/${branchId}`;
+  getFiles(projectUrl, ref) {
+    const url = `${projectUrl}/files/${ref}`;
     return axios.get(url, { params: { format: 'json' } });
   },
   lastCommitPipelines({ getters }) {

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

 import { joinPaths } from '~/lib/utils/url_utility';
-import { normalizeHeaders } from '~/lib/utils/common_utils';
 import { __ } from '~/locale';
 import eventHub from '../../eventhub';
 import service from '../../services';
 import * as types from '../mutation_types';
 import router from '../../ide_router';
-import { setPageTitle, replaceFileUrl, addFinalNewlineIfNeeded } from '../utils';
+import { escapeFileUrl, addFinalNewlineIfNeeded, setPageTitleForFile } from '../utils';
 import { viewerTypes, stageKeys } from '../../constants';
 
 export const closeFile = ({ commit, state, dispatch }, file) => {
@@ -58,7 +57,7 @@ export const setFileActive = ({ commit, state, getters, dispatch }, path) => {
 };
 
 export const getFileData = (
-  { state, commit, dispatch },
+  { state, commit, dispatch, getters },
   { path, makeFileActive = true, openFile = makeFileActive },
 ) => {
   const file = state.entries[path];
@@ -67,15 +66,18 @@ export const getFileData = (
 
   commit(types.TOGGLE_LOADING, { entry: file });
 
-  const url = file.prevPath ? replaceFileUrl(file.url, file.path, file.prevPath) : file.url;
+  const url = joinPaths(
+    gon.relative_url_root || '/',
+    state.currentProjectId,
+    file.type,
+    getters.lastCommit && getters.lastCommit.id,
+    escapeFileUrl(file.prevPath || file.path),
+  );
 
   return service
-    .getFileData(joinPaths(gon.relative_url_root || '', url.replace('/-/', '/')))
-    .then(({ data, headers }) => {
-      const normalizedHeaders = normalizeHeaders(headers);
-      let title = normalizedHeaders['PAGE-TITLE'];
-      title = file.prevPath ? title.replace(file.prevPath, file.path) : title;
-      setPageTitle(decodeURI(title));
+    .getFileData(url)
+    .then(({ data }) => {
+      setPageTitleForFile(state, file);
 
       if (data) commit(types.SET_FILE_DATA, { data, file });
       if (openFile) commit(types.TOGGLE_FILE_OPEN, path);

app/assets/javascripts/ide/stores/actions/merge_request.js

@@ -152,15 +152,17 @@ export const openMergeRequest = (
     .then(mr => {
       dispatch('setCurrentBranchId', mr.source_branch);
 
-      dispatch('getBranchData', {
+      // getFiles needs to be called after getting the branch data
+      // since files are fetched using the last commit sha of the branch
+      return dispatch('getBranchData', {
         projectId,
         branchId: mr.source_branch,
-      });
-
-      return dispatch('getFiles', {
+      }).then(() =>
+        dispatch('getFiles', {
           projectId,
           branchId: mr.source_branch,
-      });
+        }),
+      );
     })
     .then(() =>
       dispatch('getMergeRequestVersions', {

app/assets/javascripts/ide/stores/actions/tree.js

@@ -46,7 +46,7 @@ export const setDirectoryData = ({ state, commit }, { projectId, branchId, treeL
   });
 };
 
-export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } = {}) =>
+export const getFiles = ({ state, commit, dispatch, getters }, { projectId, branchId } = {}) =>
   new Promise((resolve, reject) => {
     if (
       !state.trees[`${projectId}/${branchId}`] ||
@@ -54,10 +54,11 @@ export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } =
         state.trees[`${projectId}/${branchId}`].tree.length === 0)
     ) {
       const selectedProject = state.projects[projectId];
-      commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
+      const selectedBranch = getters.findBranch(projectId, branchId);
 
+      commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
       service
-        .getFiles(selectedProject.web_url, branchId)
+        .getFiles(selectedProject.web_url, selectedBranch.commit.id)
         .then(({ data }) => {
           const { entries, treeList } = decorateFiles({
             data,

app/assets/javascripts/ide/stores/getters.js

@@ -34,7 +34,9 @@ export const currentMergeRequest = state => {
   return null;
 };
 
-export const currentProject = state => state.projects[state.currentProjectId];
+export const findProject = state => projectId => state.projects[projectId];
+
+export const currentProject = (state, getters) => getters.findProject(state.currentProjectId);
 
 export const emptyRepo = state =>
   state.projects[state.currentProjectId] && state.projects[state.currentProjectId].empty_repo;
@@ -94,8 +96,14 @@ export const lastCommit = (state, getters) => {
   return branch ? branch.commit : null;
 };
 
+export const findBranch = (state, getters) => (projectId, branchId) => {
+  const project = getters.findProject(projectId);
+
+  return project && project.branches[branchId];
+};
+
 export const currentBranch = (state, getters) =>
-  getters.currentProject && getters.currentProject.branches[state.currentBranchId];
+  getters.findBranch(state.currentProjectId, state.currentBranchId);
 
 export const branchName = (_state, getters) => getters.currentBranch && getters.currentBranch.name;
 

app/assets/javascripts/ide/stores/utils.js

@@ -113,6 +113,11 @@ export const setPageTitle = title => {
   document.title = title;
 };
 
+export const setPageTitleForFile = (state, file) => {
+  const title = [file.path, state.currentBranchId, state.currentProjectId, 'GitLab'].join(' · ');
+  setPageTitle(title);
+};
+
 export const commitActionForFile = file => {
   if (file.prevPath) {
     return commitActionTypes.move;

changelogs/unreleased/29451-webide-currentsha.yml

+---
+title: 'Resolve: Web IDE Throws Error When Viewing Diff for Renamed Files'
+merge_request: 19348
+author:
+type: fixed

changelogs/unreleased/issue-63160.yml

+---
+title: Use initial commit SHA instead of branch id to request IDE files and contents
+merge_request: 19348
+author: David Palubin
+type: fixed

spec/frontend/ide/services/index_spec.js

+import axios from 'axios';
+import MockAdapter from 'axios-mock-adapter';
 import services from '~/ide/services';
 import Api from '~/api';
+import { escapeFileUrl } from '~/ide/stores/utils';
 
 jest.mock('~/api');
 
 const TEST_PROJECT_ID = 'alice/wonderland';
 const TEST_BRANCH = 'master-patch-123';
 const TEST_COMMIT_SHA = '123456789';
+const TEST_FILE_PATH = 'README2.md';
+const TEST_FILE_OLD_PATH = 'OLD_README2.md';
+const TEST_FILE_PATH_SPECIAL = 'READM?ME/abc';
+const TEST_FILE_CONTENTS = 'raw file content';
 
 describe('IDE services', () => {
   describe('commit', () => {
@@ -28,4 +35,80 @@ describe('IDE services', () => {
       expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload);
     });
   });
+
+  describe('getBaseRawFileData', () => {
+    let file;
+    let mock;
+
+    beforeEach(() => {
+      file = {
+        mrChange: null,
+        projectId: TEST_PROJECT_ID,
+        path: TEST_FILE_PATH,
+      };
+
+      jest.spyOn(axios, 'get');
+
+      mock = new MockAdapter(axios);
+    });
+
+    afterEach(() => {
+      mock.restore();
+    });
+
+    it('gives back file.baseRaw for files with that property present', () => {
+      file.baseRaw = TEST_FILE_CONTENTS;
+
+      return services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+        expect(content).toEqual(TEST_FILE_CONTENTS);
+      });
+    });
+
+    it('gives back file.baseRaw for files for temp files', () => {
+      file.tempFile = true;
+      file.baseRaw = TEST_FILE_CONTENTS;
+
+      return services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+        expect(content).toEqual(TEST_FILE_CONTENTS);
+      });
+    });
+
+    describe.each`
+      relativeUrlRoot | filePath                  | isRenamed
+      ${''}           | ${TEST_FILE_PATH}         | ${false}
+      ${''}           | ${TEST_FILE_OLD_PATH}     | ${true}
+      ${''}           | ${TEST_FILE_PATH_SPECIAL} | ${false}
+      ${''}           | ${TEST_FILE_PATH_SPECIAL} | ${true}
+      ${'gitlab'}     | ${TEST_FILE_OLD_PATH}     | ${true}
+    `(
+      'with relativeUrlRoot ($relativeUrlRoot) and filePath ($filePath) and isRenamed ($isRenamed)',
+      ({ relativeUrlRoot, filePath, isRenamed }) => {
+        beforeEach(() => {
+          if (isRenamed) {
+            file.mrChange = {
+              renamed_file: true,
+              old_path: filePath,
+            };
+          } else {
+            file.path = filePath;
+          }
+
+          gon.relative_url_root = relativeUrlRoot;
+
+          mock
+            .onGet(
+              `${relativeUrlRoot}/${TEST_PROJECT_ID}/raw/${TEST_COMMIT_SHA}/${escapeFileUrl(
+                filePath,
+              )}`,
+            )
+            .reply(200, TEST_FILE_CONTENTS);
+        });
+
+        it('fetches file content', () =>
+          services.getBaseRawFileData(file, TEST_COMMIT_SHA).then(content => {
+            expect(content).toEqual(TEST_FILE_CONTENTS);
+          }));
+      },
+    );
+  });
 });

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

@@ -182,13 +182,25 @@ describe('IDE store file actions', () => {
       spyOn(service, 'getFileData').and.callThrough();
 
       localFile = file(`newCreate-${Math.random()}`);
-      localFile.url = `project/getFileDataURL`;
       store.state.entries[localFile.path] = localFile;
+
+      store.state.currentProjectId = 'test/test';
+      store.state.currentBranchId = 'master';
+
+      store.state.projects['test/test'] = {
+        branches: {
+          master: {
+            commit: {
+              id: '7297abc',
+            },
+          },
+        },
+      };
     });
 
     describe('success', () => {
       beforeEach(() => {
-        mock.onGet(`${RELATIVE_URL_ROOT}/project/getFileDataURL`).replyOnce(
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`).replyOnce(
           200,
           {
             blame_path: 'blame_path',
@@ -210,7 +222,7 @@ describe('IDE store file actions', () => {
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
             expect(service.getFileData).toHaveBeenCalledWith(
-              `${RELATIVE_URL_ROOT}/project/getFileDataURL`,
+              `${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`,
             );
 
             done();
@@ -229,12 +241,11 @@ describe('IDE store file actions', () => {
           .catch(done.fail);
       });
 
-      it('sets document title', done => {
+      it('sets document title with the branchId', done => {
         store
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
-            expect(document.title).toBe('testing getFileData');
-
+            expect(document.title).toBe(`${localFile.path} · master · test/test · GitLab`);
             done();
           })
           .catch(done.fail);
@@ -283,7 +294,7 @@ describe('IDE store file actions', () => {
         localFile.path = 'new-shiny-file';
         store.state.entries[localFile.path] = localFile;
 
-        mock.onGet(`${RELATIVE_URL_ROOT}/project/getFileDataURL`).replyOnce(
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/old-dull-file`).replyOnce(
           200,
           {
             blame_path: 'blame_path',
@@ -304,7 +315,7 @@ describe('IDE store file actions', () => {
         store
           .dispatch('getFileData', { path: localFile.path })
           .then(() => {
-            expect(document.title).toBe('testing new-shiny-file');
+            expect(document.title).toBe(`new-shiny-file · master · test/test · GitLab`);
 
             done();
           })
@@ -314,14 +325,17 @@ describe('IDE store file actions', () => {
 
     describe('error', () => {
       beforeEach(() => {
-        mock.onGet(`project/getFileDataURL`).networkError();
+        mock.onGet(`${RELATIVE_URL_ROOT}/test/test/7297abc/${localFile.path}`).networkError();
       });
 
       it('dispatches error action', done => {
         const dispatch = jasmine.createSpy('dispatch');
 
         actions
-          .getFileData({ state: store.state, commit() {}, dispatch }, { path: localFile.path })
+          .getFileData(
+            { state: store.state, commit() {}, dispatch, getters: store.getters },
+            { path: localFile.path },
+          )
           .then(() => {
             expect(dispatch).toHaveBeenCalledWith('setErrorMessage', {
               text: 'An error occurred whilst loading the file.',

spec/javascripts/ide/stores/actions/merge_request_spec.js

@@ -356,8 +356,30 @@ describe('IDE store merge request actions', () => {
         changes: [],
       };
       store.state.entries = {
-        foo: {},
-        bar: {},
+        foo: {
+          type: 'blob',
+        },
+        bar: {
+          type: 'blob',
+        },
+      };
+
+      store.state.currentProjectId = 'test/test';
+      store.state.currentBranchId = 'master';
+
+      store.state.projects['test/test'] = {
+        branches: {
+          master: {
+            commit: {
+              id: '7297abc',
+            },
+          },
+          abcbranch: {
+            commit: {
+              id: '29020fc',
+            },
+          },
+        },
       };
 
       const originalDispatch = store.dispatch;
@@ -415,9 +437,11 @@ describe('IDE store merge request actions', () => {
     it('updates activity bar view and gets file data, if changes are found', done => {
       store.state.entries.foo = {
         url: 'test',
+        type: 'blob',
       };
       store.state.entries.bar = {
         url: 'test',
+        type: 'blob',
       };
 
       testMergeRequestChanges.changes = [

spec/javascripts/ide/stores/actions/tree_spec.js

@@ -31,7 +31,10 @@ describe('Multi-file store tree actions', () => {
       web_url: '',
       branches: {
         master: {
-          workingReference: '1',
+          workingReference: '12345678',
+          commit: {
+            id: '12345678',
+          },
         },
       },
     };
@@ -61,7 +64,7 @@ describe('Multi-file store tree actions', () => {
         store
           .dispatch('getFiles', basicCallParameters)
           .then(() => {
-            expect(service.getFiles).toHaveBeenCalledWith('', 'master');
+            expect(service.getFiles).toHaveBeenCalledWith('', '12345678');
 
             done();
           })
@@ -99,7 +102,17 @@ describe('Multi-file store tree actions', () => {
         store.state.projects = {
           'abc/def': {
             web_url: `${gl.TEST_HOST}/files`,
+            branches: {
+              'master-testing': {
+                commit: {
+                  id: '12345',
+                },
               },
+            },
+          },
+        };
+        const getters = {
+          findBranch: () => store.state.projects['abc/def'].branches['master-testing'],
         };
 
         mock.onGet(/(.*)/).replyOnce(500);
@@ -109,6 +122,7 @@ describe('Multi-file store tree actions', () => {
             commit() {},
             dispatch,
             state: store.state,
+            getters,
           },
           {
             projectId: 'abc/def',

spec/javascripts/ide/stores/getters_spec.js

@@ -163,21 +163,58 @@ describe('IDE store getters', () => {
 
   describe('currentBranch', () => {
     it('returns current projects branch', () => {
-      const localGetters = {
-        currentProject: {
+      localState.currentProjectId = 'abcproject';
+      localState.currentBranchId = 'master';
+      localState.projects.abcproject = {
+        name: 'abcproject',
         branches: {
           master: {
             name: 'master',
           },
         },
-        },
       };
-      localState.currentBranchId = 'master';
+      const localGetters = {
+        findBranch: jasmine.createSpy('findBranchSpy'),
+      };
+      getters.currentBranch(localState, localGetters);
 
-      expect(getters.currentBranch(localState, localGetters)).toEqual({
-        name: 'master',
+      expect(localGetters.findBranch).toHaveBeenCalledWith('abcproject', 'master');
+    });
+  });
+
+  describe('findProject', () => {
+    it('returns the project matching the id', () => {
+      localState.currentProjectId = 'abcproject';
+      localState.projects.abcproject = {
+        name: 'abcproject',
+      };
+
+      expect(getters.findProject(localState)('abcproject').name).toBe('abcproject');
     });
   });
+
+  describe('findBranch', () => {
+    let result;
+
+    it('returns the selected branch from a project', () => {
+      localState.currentProjectId = 'abcproject';
+      localState.currentBranchId = 'master';
+      localState.projects.abcproject = {
+        name: 'abcproject',
+        branches: {
+          master: {
+            name: 'master',
+          },
+        },
+      };
+      const localGetters = {
+        findProject: () => localState.projects.abcproject,
+      };
+
+      result = getters.findBranch(localState, localGetters)('abcproject', 'master');
+
+      expect(result.name).toBe('master');
+    });
   });
 
   describe('isOnDefaultBranch', () => {

spec/javascripts/ide/stores/utils_spec.js

@@ -11,6 +11,23 @@ describe('Multi-file store utils', () => {
     });
   });
 
+  describe('setPageTitleForFile', () => {
+    it('sets the document page title for the file passed', () => {
+      const f = {
+        path: 'README.md',
+      };
+
+      const state = {
+        currentBranchId: 'master',
+        currentProjectId: 'test/test',
+      };
+
+      utils.setPageTitleForFile(state, f);
+
+      expect(document.title).toBe('README.md · master · test/test · GitLab');
+    });
+  });
+
   describe('findIndexOfFile', () => {
     let localState;