From 2497c29ef0d1e178c8b3aa96e8304ee6765bae4e Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Fri, 14 Sep 2018 16:51:25 +0100
Subject: [PATCH] Use returned discussion from API

We now use the returned discussion instead of re-fetching
all of the discussions and filtering out the ones we don't need.
This speeds up the process of creating a diff discussions
by saving us another API request before we can render the discussion
---
 .../diffs/components/diff_line_note_form.vue  | 48 ++++++-------------
 app/assets/javascripts/diffs/store/actions.js | 19 +++++++-
 .../javascripts/notes/stores/actions.js       | 20 ++------
 .../javascripts/notes/stores/mutations.js     |  3 +-
 .../components/diff_line_note_form_spec.js    | 27 +++++------
 spec/javascripts/diffs/store/actions_spec.js  | 46 ++++++++++++++----
 6 files changed, 87 insertions(+), 76 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
index 0fa14615532..bb9bb821de3 100644
--- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue
+++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue
@@ -1,12 +1,9 @@
 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
-import createFlash from '~/flash';
 import { s__ } from '~/locale';
 import noteForm from '../../notes/components/note_form.vue';
-import { getNoteFormData } from '../store/utils';
 import autosave from '../../notes/mixins/autosave';
 import { DIFF_NOTE_TYPE } from '../constants';
-import { reduceDiscussionsToLineCodes } from '../../notes/stores/utils';
 
 export default {
   components: {
@@ -39,6 +36,16 @@ export default {
     }),
     ...mapGetters('diffs', ['getDiffFileByHash']),
     ...mapGetters(['isLoggedIn', 'noteableType', 'getNoteableData', 'getNotesDataByProp']),
+    formData() {
+      return {
+        noteableData: this.noteableData,
+        noteableType: this.noteableType,
+        noteTargetLine: this.noteTargetLine,
+        diffViewType: this.diffViewType,
+        diffFile: this.getDiffFileByHash(this.diffFileHash),
+        linePosition: this.linePosition,
+      };
+    },
   },
   mounted() {
     if (this.isLoggedIn) {
@@ -53,8 +60,7 @@ export default {
     }
   },
   methods: {
-    ...mapActions('diffs', ['cancelCommentForm', 'assignDiscussionsToDiff']),
-    ...mapActions(['saveNote', 'refetchDiscussionById']),
+    ...mapActions('diffs', ['cancelCommentForm', 'assignDiscussionsToDiff', 'saveDiffDiscussion']),
     handleCancelCommentForm(shouldConfirm, isDirty) {
       if (shouldConfirm && isDirty) {
         const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
@@ -73,35 +79,9 @@ export default {
       });
     },
     handleSaveNote(note) {
-      const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash);
-      const postData = getNoteFormData({
-        note,
-        noteableData: this.noteableData,
-        noteableType: this.noteableType,
-        noteTargetLine: this.noteTargetLine,
-        diffViewType: this.diffViewType,
-        diffFile: selectedDiffFile,
-        linePosition: this.linePosition,
-      });
-
-      this.saveNote(postData)
-        .then(result => {
-          const endpoint = this.getNotesDataByProp('discussionsPath');
-
-          this.refetchDiscussionById({ path: endpoint, discussionId: result.discussion_id })
-            .then(selectedDiscussion => {
-              const lineCodeDiscussions = reduceDiscussionsToLineCodes([selectedDiscussion]);
-              this.assignDiscussionsToDiff(lineCodeDiscussions);
-
-              this.handleCancelCommentForm();
-            })
-            .catch(() => {
-              createFlash(s__('MergeRequests|Updating discussions failed'));
-            });
-        })
-        .catch(() => {
-          createFlash(s__('MergeRequests|Saving the comment failed'));
-        });
+      return this.saveDiffDiscussion({ note, formData: this.formData }).then(() =>
+        this.handleCancelCommentForm(),
+      );
     },
   },
 };
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index e60bb9dd7e3..98d8d5943f9 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -1,9 +1,12 @@
 import Vue from 'vue';
 import axios from '~/lib/utils/axios_utils';
 import Cookies from 'js-cookie';
+import createFlash from '~/flash';
+import { s__ } from '~/locale';
 import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
 import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
-import { getDiffPositionByLineCode } from './utils';
+import { reduceDiscussionsToLineCodes } from '../../notes/stores/utils';
+import { getDiffPositionByLineCode, getNoteFormData } from './utils';
 import * as types from './mutation_types';
 import {
   PARALLEL_DIFF_VIEW_TYPE,
@@ -178,5 +181,19 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
   });
 };
 
+export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => {
+  const postData = getNoteFormData({
+    note,
+    ...formData,
+  });
+
+  return dispatch('saveNote', postData, { root: true })
+    .then(result => dispatch('updateDiscussion', result.discussion, { root: true }))
+    .then(discussion =>
+      dispatch('assignDiscussionsToDiff', reduceDiscussionsToLineCodes([discussion])),
+    )
+    .catch(() => createFlash(s__('MergeRequests|Saving the comment failed')));
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index 68df63b8539..62b36f300ff 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -44,23 +44,11 @@ export const fetchDiscussions = ({ commit }, path) =>
       commit(types.SET_INITIAL_DISCUSSIONS, discussions);
     });
 
-export const refetchDiscussionById = ({ commit, state }, { path, discussionId }) =>
-  new Promise(resolve => {
-    service
-      .fetchDiscussions(path)
-      .then(res => res.json())
-      .then(discussions => {
-        const selectedDiscussion = discussions.find(discussion => discussion.id === discussionId);
-        if (selectedDiscussion) {
-          commit(types.UPDATE_DISCUSSION, selectedDiscussion);
-          // We need to refetch as it is now the transformed one in state
-          const discussion = utils.findNoteObjectById(state.discussions, discussionId);
+export const updateDiscussion = ({ commit }, discussion) => {
+  commit(types.UPDATE_DISCUSSION, discussion);
 
-          resolve(discussion);
-        }
-      })
-      .catch(() => {});
-  });
+  return discussion;
+};
 
 export const deleteNote = ({ commit, dispatch }, note) =>
   service.deleteNote(note.path).then(() => {
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index f1242a0d8be..73e55705f39 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -4,7 +4,8 @@ import * as constants from '../constants';
 import { isInMRPage } from '../../lib/utils/common_utils';
 
 export default {
-  [types.ADD_NEW_NOTE](state, note) {
+  [types.ADD_NEW_NOTE](state, data) {
+    const note = data.discussion ? data.discussion.notes[0] : data;
     const { discussion_id, type } = note;
     const [exists] = state.discussions.filter(n => n.id === note.discussion_id);
     const isDiscussion = type === constants.DISCUSSION_NOTE || type === constants.DIFF_NOTE;
diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
index 6fe5fdaf7f9..f31fc1f0e2b 100644
--- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js
+++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
@@ -69,22 +69,21 @@ describe('DiffLineNoteForm', () => {
 
     describe('saveNoteForm', () => {
       it('should call saveNote action with proper params', done => {
-        let isPromiseCalled = false;
-        const formDataSpy = spyOnDependency(DiffLineNoteForm, 'getNoteFormData').and.returnValue({
-          postData: 1,
-        });
-        const saveNoteSpy = spyOn(component, 'saveNote').and.returnValue(
-          new Promise(() => {
-            isPromiseCalled = true;
-            done();
-          }),
+        const saveDiffDiscussionSpy = spyOn(component, 'saveDiffDiscussion').and.returnValue(
+          Promise.resolve(),
         );
+        spyOnProperty(component, 'formData').and.returnValue('formData');
 
-        component.handleSaveNote('note body');
-
-        expect(formDataSpy).toHaveBeenCalled();
-        expect(saveNoteSpy).toHaveBeenCalled();
-        expect(isPromiseCalled).toEqual(true);
+        component
+          .handleSaveNote('note body')
+          .then(() => {
+            expect(saveDiffDiscussionSpy).toHaveBeenCalledWith({
+              note: 'note body',
+              formData: 'formData',
+            });
+          })
+          .then(done)
+          .catch(done.fail);
       });
     });
   });
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index 4c662fac231..5b1a0b2f392 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -21,6 +21,7 @@ import actions, {
   loadCollapsedDiff,
   expandAllFiles,
   toggleFileDiscussions,
+  saveDiffDiscussion,
 } from '~/diffs/store/actions';
 import * as types from '~/diffs/store/mutation_types';
 import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils';
@@ -245,7 +246,7 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('startRenderDiffsQueue', () => {
-    it('should set all files to RENDER_FILE', done => {
+    it('should set all files to RENDER_FILE', () => {
       const state = {
         diffFiles: [
           {
@@ -268,16 +269,10 @@ describe('DiffsStoreActions', () => {
         });
       };
 
-      startRenderDiffsQueue({ state, commit: pseudoCommit })
-        .then(() => {
-          expect(state.diffFiles[0].renderIt).toBeTruthy();
-          expect(state.diffFiles[1].renderIt).toBeTruthy();
+      startRenderDiffsQueue({ state, commit: pseudoCommit });
 
-          done();
-        })
-        .catch(() => {
-          done.fail();
-        });
+      expect(state.diffFiles[0].renderIt).toBeTruthy();
+      expect(state.diffFiles[1].renderIt).toBeTruthy();
     });
   });
 
@@ -582,4 +577,35 @@ describe('DiffsStoreActions', () => {
       expect(handleLocationHashSpy).toHaveBeenCalledTimes(1);
     });
   });
+
+  describe('saveDiffDiscussion', () => {
+    beforeEach(() => {
+      spyOnDependency(actions, 'getNoteFormData').and.returnValue('testData');
+      spyOnDependency(actions, 'reduceDiscussionsToLineCodes').and.returnValue('discussions');
+    });
+
+    it('dispatches actions', done => {
+      const dispatch = jasmine.createSpy('dispatch').and.callFake(name => {
+        switch (name) {
+          case 'saveNote':
+            return Promise.resolve({
+              discussion: 'test',
+            });
+          case 'updateDiscussion':
+            return Promise.resolve('discussion');
+          default:
+            return Promise.resolve({});
+        }
+      });
+
+      saveDiffDiscussion({ dispatch }, { note: {}, formData: {} })
+        .then(() => {
+          expect(dispatch.calls.argsFor(0)).toEqual(['saveNote', 'testData', { root: true }]);
+          expect(dispatch.calls.argsFor(1)).toEqual(['updateDiscussion', 'test', { root: true }]);
+          expect(dispatch.calls.argsFor(2)).toEqual(['assignDiscussionsToDiff', 'discussions']);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+  });
 });