Fixed Code review Comments

app/assets/javascripts/ide/ide_router.js

@@ -108,60 +108,44 @@ router.beforeEach((to, from, next) => {
                 branchId: mr.source_branch,
               });
-              store
-                .dispatch('getFiles', {
-                  projectId: fullProjectId,
-                  branchId: mr.source_branch,
-                })
-                .then(() => {
-                  store
-                    .dispatch('getMergeRequestChanges', {
-                      projectId: fullProjectId,
-                      mergeRequestId: to.params.mrid,
-                    })
-                    .then(mrChanges => {
-                      store
-                        .dispatch('getMergeRequestVersions', {
-                          projectId: fullProjectId,
-                          mergeRequestId: to.params.mrid,
-                        })
-                        .then(() => {
-                          mrChanges.changes.forEach((change, ind) => {
-                            const changeTreeEntry = store.state.entries[change.new_path];
-
-                            if (changeTreeEntry) {
-                              store.dispatch('setFileMrChange', {
-                                file: changeTreeEntry,
-                                mrChange: change,
-                              });
-
-                              if (ind < 10) {
-                                store.dispatch('getFileData', {
-                                  path: change.new_path,
-                                  makeFileActive: ind === 0,
-                                });
-                              }
-                            }
-                          });
-                        })
-                        .catch(e => {
-                          flash(
-                            'Error while loading the merge request versions. Please try again.',
-                          );
-                          throw e;
-                        });
-                    })
-                    .catch(e => {
-                      flash('Error while loading the merge request changes. Please try again.');
-                      throw e;
+              return store.dispatch('getFiles', {
+                projectId: fullProjectId,
+                branchId: mr.source_branch,
+              });
+            })
+            .then(() =>
+              store.dispatch('getMergeRequestVersions', {
+                projectId: fullProjectId,
+                mergeRequestId: to.params.mrid,
+              }),
+            )
+            .then(() =>
+              store.dispatch('getMergeRequestChanges', {
+                projectId: fullProjectId,
+                mergeRequestId: to.params.mrid,
+              }),
+            )
+            .then(mrChanges => {
+              mrChanges.changes.forEach((change, ind) => {
+                const changeTreeEntry = store.state.entries[change.new_path];
+
+                if (changeTreeEntry) {
+                  store.dispatch('setFileMrChange', {
+                    file: changeTreeEntry,
+                    mrChange: change,
+                  });
+
+                  if (ind < 10) {
+                    store.dispatch('getFileData', {
+                      path: change.new_path,
+                      makeFileActive: ind === 0,
                     });
-                })
-                .catch(e => {
-                  flash('Error while loading the branch files. Please try again.');
-                  throw e;
-                });
+                  }
+                }
+              });
             })
             .catch(e => {
+              flash('Error while loading the merge request. Please try again.');
               throw e;
             });
         }

app/assets/javascripts/ide/lib/common/model.js

@@ -21,7 +21,6 @@ export default class Model {
         new this.monaco.Uri(null, null, this.file.path),
       )),
     );
-
     if (this.file.mrChange) {
       this.disposable.add(
         (this.baseModel = this.monaco.editor.createModel(

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

@@ -32,7 +32,7 @@ export default {
     }
     return Vue.http
-      .get(file.rawPath.replace(file.branchId, sha), {
+      .get(file.rawPath.replace(`/raw/${file.branchId}/${file.path}`, `/raw/${sha}/${file.path}`), {
         params: { format: 'json' },
       })
       .then(res => res.text());

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

@@ -53,7 +53,6 @@ export const getFileData = ({ state, commit, dispatch }, { path, makeFileActive
     .getFileData(file.url)
     .then(res => {
       const pageTitle = decodeURI(normalizeHeaders(res.headers)['PAGE-TITLE']);
-
       setPageTitle(pageTitle);
       return res.json();
@@ -61,7 +60,7 @@ export const getFileData = ({ state, commit, dispatch }, { path, makeFileActive
     .then(data => {
       commit(types.SET_FILE_DATA, { data, file });
       commit(types.TOGGLE_FILE_OPEN, path);
-      if (makeFileActive) dispatch('setFileActive', file.path);
+      if (makeFileActive) dispatch('setFileActive', path);
       commit(types.TOGGLE_LOADING, { entry: file });
     })
     .catch(() => {
@@ -71,7 +70,7 @@ export const getFileData = ({ state, commit, dispatch }, { path, makeFileActive
 };
 export const setFileMrChange = ({ state, commit }, { file, mrChange }) => {
-  commit(types.SET_FILE_MR_CHANGE, { file, mrChange });
+  commit(types.SET_FILE_MERGE_REQUEST_CHANGE, { file, mrChange });
 };
 export const getRawFileData = ({ state, commit, dispatch }, { path, baseSha }) => {

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

@@ -46,6 +46,6 @@ export const TOGGLE_FILE_CHANGED = 'TOGGLE_FILE_CHANGED';
 export const SET_CURRENT_BRANCH = 'SET_CURRENT_BRANCH';
 export const SET_ENTRIES = 'SET_ENTRIES';
 export const CREATE_TMP_ENTRY = 'CREATE_TMP_ENTRY';
-export const SET_FILE_MR_CHANGE = 'SET_FILE_MR_CHANGE';
+export const SET_FILE_MERGE_REQUEST_CHANGE = 'SET_FILE_MERGE_REQUEST_CHANGE';
 export const UPDATE_VIEWER = 'UPDATE_VIEWER';
 export const UPDATE_DELAY_VIEWER_CHANGE = 'UPDATE_DELAY_VIEWER_CHANGE';

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

@@ -66,8 +66,8 @@ export default {
       editorColumn,
     });
   },
-  [types.SET_FILE_MR_CHANGE](state, { file, mrChange }) {
-    Object.assign(file, {
+  [types.SET_FILE_MERGE_REQUEST_CHANGE](state, { file, mrChange }) {
+    Object.assign(state.entries[file.path], {
       mrChange,
     });
   },

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

@@ -7,17 +7,15 @@ export default {
     });
   },
   [types.SET_MERGE_REQUEST](state, { projectPath, mergeRequestId, mergeRequest }) {
-    // Add client side properties
-    Object.assign(mergeRequest, {
-      active: true,
-      changes: [],
-      versions: [],
-      baseCommitSha: null,
-    });
-
     Object.assign(state.projects[projectPath], {
       mergeRequests: {
-        [mergeRequestId]: mergeRequest,
+        [mergeRequestId]: {
+          ...mergeRequest,
+          active: true,
+          changes: [],
+          versions: [],
+          baseCommitSha: null,
+        },
       },
     });
   },

app/assets/javascripts/vue_merge_request_widget/components/mr_widget_header.vue

@@ -100,10 +100,9 @@ export default {
     <div v-if="mr.isOpen">
       <a
-        :disabled="mr.sourceBranchRemoved"
+        v-if="!mr.sourceBranchRemoved"
         :href="webIdePath"
         class="btn btn-sm btn-default inline js-web-ide"
-        type="button"
       >
         {{ s__("mrWidget|Web IDE") }}
       </a>

spec/javascripts/ide/components/changed_file_icon_spec.js

@@ -25,12 +25,6 @@ describe('IDE changed file icon', () => {
       expect(vm.changedIcon).toBe('file-modified');
     });
-    it('equals git-merge when not a temp file and has no changes', () => {
-      vm.file.changed = false;
-
-      expect(vm.changedIcon).toBe('git-merge');
-    });
-
     it('equals file-addition when a temp file', () => {
       vm.file.tempFile = true;
@@ -43,12 +37,6 @@ describe('IDE changed file icon', () => {
       expect(vm.changedIconClass).toContain('multi-file-modified');
     });
-    it('includes multi-git-merge when a mr changed file', () => {
-      vm.file.changed = false;
-
-      expect(vm.changedIconClass).toContain('multi-git-merge');
-    });
-
     it('includes multi-file-addition when a temp file', () => {
       vm.file.tempFile = true;

spec/javascripts/ide/components/repo_editor_spec.js

@@ -151,25 +151,25 @@ describe('RepoEditor', () => {
   describe('setup editor for merge request viewing', () => {
     beforeEach(done => {
+      // Resetting as the main test setup has already done it
       vm.$destroy();
-
       resetStore(vm.$store);
-
       Editor.editorInstance.modelManager.dispose();
-      const f = file();
+      const f = {
+        ...file(),
+        active: true,
+        tempFile: true,
+        html: 'testing',
+        mrChange: { diff: 'ABC' },
+        baseRaw: 'testing',
+        content: 'test',
+      };
       const RepoEditor = Vue.extend(repoEditor);
-
       vm = createComponentWithStore(RepoEditor, store, {
         file: f,
       });
-      f.active = true;
-      f.tempFile = true;
-      f.html = 'testing';
-      f.mrChange = { diff: 'ABC' };
-      f.baseRaw = 'testing';
-      f.content = 'test';
       vm.$store.state.openFiles.push(f);
       vm.$store.state.entries[f.path] = f;

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

@@ -125,9 +125,9 @@ describe('IDE store file mutations', () => {
     });
   });
-  describe('SET_FILE_MR_CHANGE', () => {
+  describe('SET_FILE_MERGE_REQUEST_CHANGE', () => {
     it('sets file mr change', () => {
-      mutations.SET_FILE_MR_CHANGE(localState, {
+      mutations.SET_FILE_MERGE_REQUEST_CHANGE(localState, {
         file: localFile,
         mrChange: { diff: 'ABC' },
       });