Merge branch '36523-get-files-code-smell' into 'master'

Resolve code smells in `getFiles` in Web IDE `ide/stores/actions/tree.js`

See merge request gitlab-org/gitlab!21805

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

@@ -141,7 +141,7 @@ export const getMergeRequestVersions = (
   });
 
 export const openMergeRequest = (
-  { dispatch, state },
+  { dispatch, state, getters },
   { projectId, targetProjectId, mergeRequestId } = {},
 ) =>
   dispatch('getMergeRequestData', {
@@ -152,17 +152,18 @@ export const openMergeRequest = (
     .then(mr => {
       dispatch('setCurrentBranchId', mr.source_branch);
 
-      // 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,
-      }).then(() =>
-        dispatch('getFiles', {
+      }).then(() => {
+        const branch = getters.findBranch(projectId, mr.source_branch);
+
+        return dispatch('getFiles', {
           projectId,
           branchId: mr.source_branch,
-        }),
-      );
+          ref: branch.commit.id,
+        });
+      });
     })
     .then(() =>
       dispatch('getMergeRequestVersions', {

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

@@ -111,7 +111,7 @@ export const loadFile = ({ dispatch, state }, { basePath }) => {
   }
 };
 
-export const loadBranch = ({ dispatch }, { projectId, branchId }) =>
+export const loadBranch = ({ dispatch, getters }, { projectId, branchId }) =>
   dispatch('getBranchData', {
     projectId,
     branchId,
@@ -121,9 +121,13 @@ export const loadBranch = ({ dispatch }, { projectId, branchId }) =>
         projectId,
         branchId,
       });
+
+      const branch = getters.findBranch(projectId, branchId);
+
       return dispatch('getFiles', {
         projectId,
         branchId,
+        ref: branch.commit.id,
       });
     })
     .catch(() => {

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

@@ -46,19 +46,20 @@ export const setDirectoryData = ({ state, commit }, { projectId, branchId, treeL
   });
 };
 
-export const getFiles = ({ state, commit, dispatch, getters }, { projectId, branchId } = {}) =>
+export const getFiles = ({ state, commit, dispatch }, payload = {}) =>
   new Promise((resolve, reject) => {
+    const { projectId, branchId, ref = branchId } = payload;
+
     if (
       !state.trees[`${projectId}/${branchId}`] ||
       (state.trees[`${projectId}/${branchId}`].tree &&
         state.trees[`${projectId}/${branchId}`].tree.length === 0)
     ) {
       const selectedProject = state.projects[projectId];
-      const selectedBranch = getters.findBranch(projectId, branchId);
 
       commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
       service
-        .getFiles(selectedProject.web_url, selectedBranch.commit.id)
+        .getFiles(selectedProject.web_url, ref)
         .then(({ data }) => {
           const { entries, treeList } = decorateFiles({
             data,
@@ -77,8 +78,8 @@ export const getFiles = ({ state, commit, dispatch, getters }, { projectId, bran
         .catch(e => {
           dispatch('setErrorMessage', {
             text: __('An error occurred whilst loading all the files.'),
-            action: payload =>
-              dispatch('getFiles', payload).then(() => dispatch('setErrorMessage', null)),
+            action: actionPayload =>
+              dispatch('getFiles', actionPayload).then(() => dispatch('setErrorMessage', null)),
             actionText: __('Please try again'),
             actionPayload: { projectId, branchId },
           });

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

@@ -348,6 +348,8 @@ describe('IDE store merge request actions', () => {
     let testMergeRequest;
     let testMergeRequestChanges;
 
+    const mockGetters = { findBranch: () => ({ commit: { id: 'abcd2322' } }) };
+
     beforeEach(() => {
       testMergeRequest = {
         source_branch: 'abcbranch',
@@ -406,8 +408,8 @@ describe('IDE store merge request actions', () => {
       );
     });
 
-    it('dispatch actions for merge request data', done => {
-      openMergeRequest(store, mr)
+    it('dispatches actions for merge request data', done => {
+      openMergeRequest({ state: store.state, dispatch: store.dispatch, getters: mockGetters }, mr)
         .then(() => {
           expect(store.dispatch.calls.allArgs()).toEqual([
             ['getMergeRequestData', mr],
@@ -424,6 +426,7 @@ describe('IDE store merge request actions', () => {
               {
                 projectId: mr.projectId,
                 branchId: testMergeRequest.source_branch,
+                ref: 'abcd2322',
               },
             ],
             ['getMergeRequestVersions', mr],
@@ -449,7 +452,7 @@ describe('IDE store merge request actions', () => {
         { new_path: 'bar', path: 'bar' },
       ];
 
-      openMergeRequest(store, mr)
+      openMergeRequest({ state: store.state, dispatch: store.dispatch, getters: mockGetters }, mr)
         .then(() => {
           expect(store.dispatch).toHaveBeenCalledWith(
             'updateActivityBarView',

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

@@ -285,16 +285,21 @@ describe('IDE store project actions', () => {
   describe('loadBranch', () => {
     const projectId = 'abc/def';
     const branchId = '123-lorem';
+    const ref = 'abcd2322';
 
     it('fetches branch data', done => {
+      const mockGetters = { findBranch: () => ({ commit: { id: ref } }) };
       spyOn(store, 'dispatch').and.returnValue(Promise.resolve());
 
-      loadBranch(store, { projectId, branchId })
+      loadBranch(
+        { getters: mockGetters, state: store.state, dispatch: store.dispatch },
+        { projectId, branchId },
+      )
         .then(() => {
           expect(store.dispatch.calls.allArgs()).toEqual([
             ['getBranchData', { projectId, branchId }],
             ['getMergeRequestsForBranch', { projectId, branchId }],
-            ['getFiles', { projectId, branchId }],
+            ['getFiles', { projectId, branchId, ref }],
           ]);
         })
         .then(done)

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

@@ -17,6 +17,7 @@ describe('Multi-file store tree actions', () => {
     projectId: 'abcproject',
     branch: 'master',
     branchId: 'master',
+    ref: '12345678',
   };
 
   beforeEach(() => {
@@ -29,14 +30,6 @@ describe('Multi-file store tree actions', () => {
     store.state.currentBranchId = 'master';
     store.state.projects.abcproject = {
       web_url: '',
-      branches: {
-        master: {
-          workingReference: '12345678',
-          commit: {
-            id: '12345678',
-          },
-        },
-      },
     };
   });