Merge branch 'ide-improve-error-messages' into 'master'

Improve branch 404 error in Web IDE

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

app/assets/javascripts/api.js

@@ -243,6 +243,15 @@ const Api = {
     });
   },
 
+  createBranch(id, { ref, branch }) {
+    const url = Api.buildUrl(this.createBranchPath).replace(':id', encodeURIComponent(id));
+
+    return axios.post(url, {
+      ref,
+      branch,
+    });
+  },
+
   buildUrl(url) {
     let urlRoot = '';
     if (gon.relative_url_root != null) {

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

+<script>
+import { mapActions } from 'vuex';
+import LoadingIcon from '../../vue_shared/components/loading_icon.vue';
+
+export default {
+  components: {
+    LoadingIcon,
+  },
+  props: {
+    message: {
+      type: Object,
+      required: true,
+    },
+  },
+  data() {
+    return {
+      isLoading: false,
+    };
+  },
+  methods: {
+    ...mapActions(['setErrorMessage']),
+    clickAction() {
+      if (this.isLoading) return;
+
+      this.isLoading = true;
+
+      this.$store
+        .dispatch(this.message.action, this.message.actionPayload)
+        .then(() => {
+          this.isLoading = false;
+        })
+        .catch(() => {
+          this.isLoading = false;
+        });
+    },
+    clickFlash() {
+      if (!this.message.action) {
+        this.setErrorMessage(null);
+      }
+    },
+  },
+};
+</script>
+
+<template>
+  <div
+    class="flash-container flash-container-page"
+    @click="clickFlash"
+  >
+    <div class="flash-alert">
+      <span
+        v-html="message.text"
+      >
+      </span>
+      <button
+        v-if="message.action"
+        type="button"
+        class="flash-action text-white p-0 border-top-0 border-right-0 border-left-0 bg-transparent"
+        @click.stop.prevent="clickAction"
+      >
+        {{ message.actionText }}
+        <loading-icon
+          v-show="isLoading"
+          inline
+        />
+      </button>
+    </div>
+  </div>
+</template>

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

@@ -7,6 +7,7 @@ import IdeStatusBar from './ide_status_bar.vue';
 import RepoEditor from './repo_editor.vue';
 import FindFile from './file_finder/index.vue';
 import RightPane from './panes/right.vue';
+import ErrorMessage from './error_message.vue';
 
 const originalStopCallback = Mousetrap.stopCallback;
 
@@ -18,6 +19,7 @@ export default {
     RepoEditor,
     FindFile,
     RightPane,
+    ErrorMessage,
   },
   computed: {
     ...mapState([
@@ -28,6 +30,7 @@ export default {
       'fileFindVisible',
       'emptyStateSvgPath',
       'currentProjectId',
+      'errorMessage',
     ]),
     ...mapGetters(['activeFile', 'hasChanges']),
   },
@@ -72,6 +75,10 @@ export default {
 
 <template>
   <article class="ide">
+    <error-message
+      v-if="errorMessage"
+      :message="errorMessage"
+    />
     <div
       class="ide-view"
     >

app/assets/javascripts/ide/ide_router.js

@@ -95,14 +95,6 @@ router.beforeEach((to, from, next) => {
               }
             })
             .catch(e => {
-              flash(
-                'Error while loading the branch files. Please try again.',
-                'alert',
-                document,
-                null,
-                false,
-                true,
-              );
               throw e;
             });
         } else if (to.params.mrid) {

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

 import Vue from 'vue';
 import VueResource from 'vue-resource';
+import axios from '~/lib/utils/axios_utils';
 import Api from '~/api';
 
 Vue.use(VueResource);
@@ -69,11 +70,7 @@ export default {
   },
   getFiles(projectUrl, branchId) {
     const url = `${projectUrl}/files/${branchId}`;
-    return Vue.http.get(url, {
-      params: {
-        format: 'json',
-      },
-    });
+    return axios.get(url, { params: { format: 'json' } });
   },
   lastCommitPipelines({ getters }) {
     const commitSha = getters.lastCommit.id;

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

@@ -175,6 +175,9 @@ export const setRightPane = ({ commit }, view) => {
 
 export const setLinks = ({ commit }, links) => commit(types.SET_LINKS, links);
 
+export const setErrorMessage = ({ commit }, errorMessage) =>
+  commit(types.SET_ERROR_MESSAGE, errorMessage);
+
 export * from './actions/tree';
 export * from './actions/file';
 export * from './actions/project';

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

+import _ from 'underscore';
 import flash from '~/flash';
-import { __ } from '~/locale';
+import { __, sprintf } from '~/locale';
 import service from '../../services';
+import api from '../../../api';
 import * as types from '../mutation_types';
+import router from '../../ide_router';
 
 export const getProjectData = ({ commit, state }, { namespace, projectId, force = false } = {}) =>
   new Promise((resolve, reject) => {
@@ -32,7 +35,10 @@ export const getProjectData = ({ commit, state }, { namespace, projectId, force
     }
   });
 
-export const getBranchData = ({ commit, state }, { projectId, branchId, force = false } = {}) =>
+export const getBranchData = (
+  { commit, dispatch, state },
+  { projectId, branchId, force = false } = {},
+) =>
   new Promise((resolve, reject) => {
     if (
       typeof state.projects[`${projectId}`] === 'undefined' ||
@@ -51,7 +57,10 @@ export const getBranchData = ({ commit, state }, { projectId, branchId, force =
           commit(types.SET_BRANCH_WORKING_REFERENCE, { projectId, branchId, reference: id });
           resolve(data);
         })
-        .catch(() => {
+        .catch(e => {
+          if (e.response.status === 404) {
+            dispatch('showBranchNotFoundError', branchId);
+          } else {
             flash(
               __('Error loading branch data. Please try again.'),
               'alert',
@@ -60,6 +69,7 @@ export const getBranchData = ({ commit, state }, { projectId, branchId, force =
               false,
               true,
             );
+          }
           reject(new Error(`Branch not loaded - ${projectId}/${branchId}`));
         });
     } else {
@@ -80,3 +90,37 @@ export const refreshLastCommitData = ({ commit }, { projectId, branchId } = {})
     .catch(() => {
       flash(__('Error loading last commit.'), 'alert', document, null, false, true);
     });
+
+export const createNewBranchFromDefault = ({ state, dispatch, getters }, branch) =>
+  api
+    .createBranch(state.currentProjectId, {
+      ref: getters.currentProject.default_branch,
+      branch,
+    })
+    .then(() => {
+      dispatch('setErrorMessage', null);
+      router.push(`${router.currentRoute.path}?${Date.now()}`);
+    })
+    .catch(() => {
+      dispatch('setErrorMessage', {
+        text: __('An error occured creating the new branch.'),
+        action: 'createNewBranchFromDefault',
+        actionText: __('Please try again'),
+        actionPayload: branch,
+      });
+    });
+
+export const showBranchNotFoundError = ({ dispatch }, branchId) => {
+  dispatch('setErrorMessage', {
+    text: sprintf(
+      __("Branch %{branchName} was not found in this project's repository."),
+      {
+        branchName: `<strong>${_.escape(branchId)}</strong>`,
+      },
+      false,
+    ),
+    action: 'createNewBranchFromDefault',
+    actionText: __('Create branch'),
+    actionPayload: branchId,
+  });
+};

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

 import { normalizeHeaders } from '~/lib/utils/common_utils';
 import flash from '~/flash';
+import { __ } from '../../../locale';
 import service from '../../services';
 import * as types from '../mutation_types';
 import { findEntry } from '../utils';
@@ -62,16 +63,19 @@ export const getLastCommitData = ({ state, commit, dispatch }, tree = state) =>
     .catch(() => flash('Error fetching log data.', 'alert', document, null, false, true));
 };
 
-export const getFiles = ({ state, commit }, { projectId, branchId } = {}) =>
+export const getFiles = ({ state, commit, dispatch }, { projectId, branchId } = {}) =>
   new Promise((resolve, reject) => {
-    if (!state.trees[`${projectId}/${branchId}`]) {
+    if (
+      !state.trees[`${projectId}/${branchId}`] ||
+      (state.trees[`${projectId}/${branchId}`].tree &&
+        state.trees[`${projectId}/${branchId}`].tree.length === 0)
+    ) {
       const selectedProject = state.projects[projectId];
       commit(types.CREATE_TREE, { treePath: `${projectId}/${branchId}` });
 
       service
         .getFiles(selectedProject.web_url, branchId)
-        .then(res => res.json())
-        .then(data => {
+        .then(({ data }) => {
           const worker = new FilesDecoratorWorker();
           worker.addEventListener('message', e => {
             const { entries, treeList } = e.data;
@@ -99,7 +103,18 @@ export const getFiles = ({ state, commit }, { projectId, branchId } = {}) =>
           });
         })
         .catch(e => {
-          flash('Error loading tree data. Please try again.', 'alert', document, null, false, true);
+          if (e.response.status === 404) {
+            dispatch('showBranchNotFoundError', branchId);
+          } else {
+            flash(
+              __('Error loading tree data. Please try again.'),
+              'alert',
+              document,
+              null,
+              false,
+              true,
+            );
+          }
           reject(e);
         });
     } else {

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

@@ -72,3 +72,5 @@ export const SET_RIGHT_PANE = 'SET_RIGHT_PANE';
 
 export const CLEAR_PROJECTS = 'CLEAR_PROJECTS';
 export const RESET_OPEN_FILES = 'RESET_OPEN_FILES';
+
+export const SET_ERROR_MESSAGE = 'SET_ERROR_MESSAGE';

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

@@ -163,6 +163,9 @@ export default {
   [types.RESET_OPEN_FILES](state) {
     Object.assign(state, { openFiles: [] });
   },
+  [types.SET_ERROR_MESSAGE](state, errorMessage) {
+    Object.assign(state, { errorMessage });
+  },
   ...projectMutations,
   ...mergeRequestMutation,
   ...fileMutations,

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

@@ -25,4 +25,5 @@ export default () => ({
   fileFindVisible: false,
   rightPane: null,
   links: {},
+  errorMessage: null,
 });

app/assets/stylesheets/framework/flash.scss

@@ -42,7 +42,7 @@
       display: inline-block;
     }
 
-    a.flash-action {
+    .flash-action {
       margin-left: 5px;
       text-decoration: none;
       font-weight: $gl-font-weight-normal;

spec/javascripts/api_spec.js

@@ -362,4 +362,29 @@ describe('Api', () => {
         .catch(done.fail);
     });
   });
+
+  describe('createBranch', () => {
+    it('creates new branch', done => {
+      const ref = 'master';
+      const branch = 'new-branch-name';
+      const dummyProjectPath = 'gitlab-org/gitlab-ce';
+      const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects/${encodeURIComponent(
+        dummyProjectPath,
+      )}/repository/branches`;
+
+      spyOn(axios, 'post').and.callThrough();
+
+      mock.onPost(expectedUrl).replyOnce(200, {
+        name: branch,
+      });
+
+      Api.createBranch(dummyProjectPath, { ref, branch })
+        .then(({ data }) => {
+          expect(data.name).toBe(branch);
+          expect(axios.post).toHaveBeenCalledWith(expectedUrl, { ref, branch });
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
 });

spec/javascripts/ide/components/error_message_spec.js

+import Vue from 'vue';
+import store from '~/ide/stores';
+import ErrorMessage from '~/ide/components/error_message.vue';
+import { createComponentWithStore } from '../../helpers/vue_mount_component_helper';
+import { resetStore } from '../helpers';
+
+describe('IDE error message component', () => {
+  const Component = Vue.extend(ErrorMessage);
+  let vm;
+
+  beforeEach(() => {
+    vm = createComponentWithStore(Component, store, {
+      message: {
+        text: 'error message',
+        action: null,
+        actionText: null,
+      },
+    }).$mount();
+  });
+
+  afterEach(() => {
+    vm.$destroy();
+    resetStore(vm.$store);
+  });
+
+  it('renders error message', () => {
+    expect(vm.$el.textContent).toContain('error message');
+  });
+
+  it('clears error message on click', () => {
+    spyOn(vm, 'setErrorMessage');
+
+    vm.$el.click();
+
+    expect(vm.setErrorMessage).toHaveBeenCalledWith(null);
+  });
+
+  describe('with action', () => {
+    beforeEach(done => {
+      vm.message.action = 'testAction';
+      vm.message.actionText = 'test action';
+      vm.message.actionPayload = 'testActionPayload';
+
+      spyOn(vm.$store, 'dispatch').and.returnValue(Promise.resolve());
+
+      vm.$nextTick(done);
+    });
+
+    it('renders action button', () => {
+      expect(vm.$el.querySelector('.flash-action')).not.toBe(null);
+      expect(vm.$el.textContent).toContain('test action');
+    });
+
+    it('does not clear error message on click', () => {
+      spyOn(vm, 'setErrorMessage');
+
+      vm.$el.click();
+
+      expect(vm.setErrorMessage).not.toHaveBeenCalled();
+    });
+
+    it('dispatches action', done => {
+      vm.$el.querySelector('.flash-action').click();
+
+      vm.$nextTick(() => {
+        expect(vm.$store.dispatch).toHaveBeenCalledWith('testAction', 'testActionPayload');
+
+        done();
+      });
+    });
+
+    it('does not dispatch action when already loading', () => {
+      vm.isLoading = true;
+
+      vm.$el.querySelector('.flash-action').click();
+
+      expect(vm.$store.dispatch).not.toHaveBeenCalledWith();
+    });
+
+    it('resets isLoading after click', done => {
+      vm.$el.querySelector('.flash-action').click();
+
+      expect(vm.isLoading).toBe(true);
+
+      vm.$nextTick(() => {
+        expect(vm.isLoading).toBe(false);
+
+        done();
+      });
+    });
+
+    it('shows loading icon when isLoading is true', done => {
+      expect(vm.$el.querySelector('.loading-container').style.display).not.toBe('');
+
+      vm.isLoading = true;
+
+      vm.$nextTick(() => {
+        expect(vm.$el.querySelector('.loading-container').style.display).toBe('');
+
+        done();
+      });
+    });
+  });
+});

spec/javascripts/ide/components/ide_spec.js

@@ -114,4 +114,18 @@ describe('ide component', () => {
       expect(vm.mousetrapStopCallback(null, document.querySelector('.inputarea'), 't')).toBe(true);
     });
   });
+
+  it('shows error message when set', done => {
+    expect(vm.$el.querySelector('.flash-container')).toBe(null);
+
+    vm.$store.state.errorMessage = {
+      text: 'error',
+    };
+
+    vm.$nextTick(() => {
+      expect(vm.$el.querySelector('.flash-container')).not.toBe(null);
+
+      done();
+    });
+  });
 });

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

-import { refreshLastCommitData } from '~/ide/stores/actions';
+import MockAdapter from 'axios-mock-adapter';
+import axios from '~/lib/utils/axios_utils';
+import {
+  refreshLastCommitData,
+  showBranchNotFoundError,
+  createNewBranchFromDefault,
+  getBranchData,
+} from '~/ide/stores/actions';
 import store from '~/ide/stores';
 import service from '~/ide/services';
+import api from '~/api';
+import router from '~/ide/ide_router';
 import { resetStore } from '../../helpers';
 import testAction from '../../../helpers/vuex_action_helper';
 
 describe('IDE store project actions', () => {
+  let mock;
+
   beforeEach(() => {
-    store.state.projects['abc/def'] = {};
+    mock = new MockAdapter(axios);
+
+    store.state.projects['abc/def'] = {
+      branches: {},
+    };
   });
 
   afterEach(() => {
+    mock.restore();
+
     resetStore(store);
   });
 
@@ -80,4 +97,138 @@ describe('IDE store project actions', () => {
       );
     });
   });
+
+  describe('showBranchNotFoundError', () => {
+    it('dispatches setErrorMessage', done => {
+      testAction(
+        showBranchNotFoundError,
+        'master',
+        null,
+        [],
+        [
+          {
+            type: 'setErrorMessage',
+            payload: {
+              text: "Branch <strong>master</strong> was not found in this project's repository.",
+              action: 'createNewBranchFromDefault',
+              actionText: 'Create branch',
+              actionPayload: 'master',
+            },
+          },
+        ],
+        done,
+      );
+    });
+  });
+
+  describe('createNewBranchFromDefault', () => {
+    it('calls API', done => {
+      spyOn(api, 'createBranch').and.returnValue(Promise.resolve());
+      spyOn(router, 'push');
+
+      createNewBranchFromDefault(
+        {
+          state: {
+            currentProjectId: 'project-path',
+          },
+          getters: {
+            currentProject: {
+              default_branch: 'master',
+            },
+          },
+          dispatch() {},
+        },
+        'new-branch-name',
+      )
+        .then(() => {
+          expect(api.createBranch).toHaveBeenCalledWith('project-path', {
+            ref: 'master',
+            branch: 'new-branch-name',
+          });
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('clears error message', done => {
+      const dispatchSpy = jasmine.createSpy('dispatch');
+      spyOn(api, 'createBranch').and.returnValue(Promise.resolve());
+      spyOn(router, 'push');
+
+      createNewBranchFromDefault(
+        {
+          state: {
+            currentProjectId: 'project-path',
+          },
+          getters: {
+            currentProject: {
+              default_branch: 'master',
+            },
+          },
+          dispatch: dispatchSpy,
+        },
+        'new-branch-name',
+      )
+        .then(() => {
+          expect(dispatchSpy).toHaveBeenCalledWith('setErrorMessage', null);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('reloads window', done => {
+      spyOn(api, 'createBranch').and.returnValue(Promise.resolve());
+      spyOn(router, 'push');
+
+      createNewBranchFromDefault(
+        {
+          state: {
+            currentProjectId: 'project-path',
+          },
+          getters: {
+            currentProject: {
+              default_branch: 'master',
+            },
+          },
+          dispatch() {},
+        },
+        'new-branch-name',
+      )
+        .then(() => {
+          expect(router.push).toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
+
+  describe('getBranchData', () => {
+    describe('error', () => {
+      it('dispatches branch not found action when response is 404', done => {
+        const dispatch = jasmine.createSpy('dispatchSpy');
+
+        mock.onGet(/(.*)/).replyOnce(404);
+
+        getBranchData(
+          {
+            commit() {},
+            dispatch,
+            state: store.state,
+          },
+          {
+            projectId: 'abc/def',
+            branchId: 'master-testing',
+          },
+        )
+          .then(done.fail)
+          .catch(() => {
+            expect(dispatch.calls.argsFor(0)).toEqual([
+              'showBranchNotFoundError',
+              'master-testing',
+            ]);
+            done();
+          });
+      });
+    });
+  });
 });

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

+import MockAdapter from 'axios-mock-adapter';
 import Vue from 'vue';
 import testAction from 'spec/helpers/vuex_action_helper';
-import { showTreeEntry } from '~/ide/stores/actions/tree';
+import { showTreeEntry, getFiles } from '~/ide/stores/actions/tree';
 import * as types from '~/ide/stores/mutation_types';
+import axios from '~/lib/utils/axios_utils';
 import store from '~/ide/stores';
 import service from '~/ide/services';
 import router from '~/ide/ide_router';
@@ -9,6 +11,7 @@ import { file, resetStore, createEntriesFromPaths } from '../../helpers';
 
 describe('Multi-file store tree actions', () => {
   let projectTree;
+  let mock;
 
   const basicCallParameters = {
     endpoint: 'rootEndpoint',
@@ -20,6 +23,8 @@ describe('Multi-file store tree actions', () => {
   beforeEach(() => {
     spyOn(router, 'push');
 
+    mock = new MockAdapter(axios);
+
     store.state.currentProjectId = 'abcproject';
     store.state.currentBranchId = 'master';
     store.state.projects.abcproject = {
@@ -33,21 +38,22 @@ describe('Multi-file store tree actions', () => {
   });
 
   afterEach(() => {
+    mock.restore();
     resetStore(store);
   });
 
   describe('getFiles', () => {
+    describe('success', () => {
       beforeEach(() => {
-      spyOn(service, 'getFiles').and.returnValue(
-        Promise.resolve({
-          json: () =>
-            Promise.resolve([
+        spyOn(service, 'getFiles').and.callThrough();
+
+        mock
+          .onGet(/(.*)/)
+          .replyOnce(200, [
             'file.txt',
             'folder/fileinfolder.js',
             'folder/subfolder/fileinsubfolder.js',
-            ]),
-        }),
-      );
+          ]);
       });
 
       it('calls service getFiles', done => {
@@ -79,6 +85,41 @@ describe('Multi-file store tree actions', () => {
       });
     });
 
+    describe('error', () => {
+      it('dispatches branch not found actions when response is 404', done => {
+        const dispatch = jasmine.createSpy('dispatchSpy');
+
+        store.state.projects = {
+          'abc/def': {
+            web_url: `${gl.TEST_HOST}/files`,
+          },
+        };
+
+        mock.onGet(/(.*)/).replyOnce(404);
+
+        getFiles(
+          {
+            commit() {},
+            dispatch,
+            state: store.state,
+          },
+          {
+            projectId: 'abc/def',
+            branchId: 'master-testing',
+          },
+        )
+          .then(done.fail)
+          .catch(() => {
+            expect(dispatch.calls.argsFor(0)).toEqual([
+              'showBranchNotFoundError',
+              'master-testing',
+            ]);
+            done();
+          });
+      });
+    });
+  });
+
   describe('toggleTreeOpen', () => {
     let tree;
 
@@ -122,9 +163,7 @@ describe('Multi-file store tree actions', () => {
           { type: types.SET_TREE_OPEN, payload: 'grandparent/parent' },
           { type: types.SET_TREE_OPEN, payload: 'grandparent' },
         ],
-        [
-          { type: 'showTreeEntry' },
-        ],
+        [{ type: 'showTreeEntry' }],
         done,
       );
     });

spec/javascripts/ide/stores/actions_spec.js

@@ -6,6 +6,7 @@ import actions, {
   setEmptyStateSvgs,
   updateActivityBarView,
   updateTempFlagForEntry,
+  setErrorMessage,
 } from '~/ide/stores/actions';
 import store from '~/ide/stores';
 import * as types from '~/ide/stores/mutation_types';
@@ -443,4 +444,17 @@ describe('Multi-file store actions', () => {
       );
     });
   });
+
+  describe('setErrorMessage', () => {
+    it('commis error messsage', done => {
+      testAction(
+        setErrorMessage,
+        'error',
+        null,
+        [{ type: types.SET_ERROR_MESSAGE, payload: 'error' }],
+        [],
+        done,
+      );
+    });
+  });
 });

spec/javascripts/ide/stores/mutations_spec.js

@@ -148,4 +148,12 @@ describe('Multi-file store mutations', () => {
       expect(localState.unusedSeal).toBe(false);
     });
   });
+
+  describe('SET_ERROR_MESSAGE', () => {
+    it('updates error message', () => {
+      mutations.SET_ERROR_MESSAGE(localState, 'error');
+
+      expect(localState.errorMessage).toBe('error');
+    });
+  });
 });