Merge branch 'dm-create-note-return-discussion' into 'master'

Increase performance when creating discussion on diff

Closes #49002

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

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(),
+      );
     },
   },
 };

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 () => {};

app/assets/javascripts/diffs/store/utils.js

@@ -55,6 +55,7 @@ export function getNoteFormData(params) {
     note_project_id: '',
     target_type: noteableData.targetType,
     target_id: noteableData.id,
+    return_discussion: true,
     note: {
       note,
       position,

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, state }, discussion) => {
+  commit(types.UPDATE_DISCUSSION, discussion);
 
-          resolve(discussion);
-        }
-      })
-      .catch(() => {});
-  });
+  return utils.findNoteObjectById(state.discussions, discussion.id);
+};
 
 export const deleteNote = ({ commit, dispatch }, note) =>
   service.deleteNote(note.path).then(() => {

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;

app/controllers/concerns/notes_actions.rb

@@ -43,12 +43,26 @@ module NotesActions
 
     @note = Notes::CreateService.new(note_project, current_user, create_params).execute
 
-    if @note.is_a?(Note)
-      prepare_notes_for_rendering([@note], noteable)
-    end
-
     respond_to do |format|
-      format.json { render json: note_json(@note) }
+      format.json do
+        json = {
+          commands_changes: @note.commands_changes
+        }
+
+        if @note.persisted? && return_discussion?
+          json[:valid] = true
+
+          discussion = @note.discussion
+          prepare_notes_for_rendering(discussion.notes)
+          json[:discussion] = discussion_serializer.represent(discussion, context: self)
+        else
+          prepare_notes_for_rendering([@note])
+
+          json.merge!(note_json(@note))
+        end
+
+        render json: json
+      end
       format.html { redirect_back_or_default }
     end
   end
@@ -57,10 +71,7 @@ module NotesActions
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def update
     @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
-
-    if @note.is_a?(Note)
-      prepare_notes_for_rendering([@note])
-    end
+    prepare_notes_for_rendering([@note])
 
     respond_to do |format|
       format.json { render json: note_json(@note) }
@@ -91,14 +102,17 @@ module NotesActions
   end
 
   def note_json(note)
-    attrs = {
-      commands_changes: note.commands_changes
-    }
+    attrs = {}
 
     if note.persisted?
       attrs[:valid] = true
 
-      if use_note_serializer?
+      if return_discussion?
+        discussion = note.discussion
+        prepare_notes_for_rendering(discussion.notes)
+
+        attrs[:discussion] = discussion_serializer.represent(discussion, context: self)
+      elsif use_note_serializer?
         attrs.merge!(note_serializer.represent(note))
       else
         attrs.merge!(
@@ -218,6 +232,10 @@ module NotesActions
     ProjectNoteSerializer.new(project: project, noteable: noteable, current_user: current_user)
   end
 
+  def discussion_serializer
+    DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: ProjectNoteEntity)
+  end
+
   def note_project
     strong_memoize(:note_project) do
       next nil unless project
@@ -237,6 +255,10 @@ module NotesActions
     end
   end
 
+  def return_discussion?
+    Gitlab::Utils.to_boolean(params[:return_discussion])
+  end
+
   def use_note_serializer?
     return false if params['html']
 

changelogs/unreleased/dm-create-note-return-discussion.yml

@@ -0,0 +1,5 @@
+---
+title: Increase performance when creating discussions on diff
+merge_request:
+author:
+type: performance

locale/gitlab.pot

@@ -3690,9 +3690,6 @@ msgstr ""
 msgid "MergeRequests|Toggle comments for this file"
 msgstr ""
 
-msgid "MergeRequests|Updating discussions failed"
-msgstr ""
-
 msgid "MergeRequests|View file @ %{commitId}"
 msgstr ""
 

spec/controllers/projects/notes_controller_spec.rb

@@ -207,6 +207,14 @@ describe Projects::NotesController do
       expect(response).to have_gitlab_http_status(200)
     end
 
+    it 'returns discussion JSON when the return_discussion param is set' do
+      post :create, request_params.merge(format: :json, return_discussion: 'true')
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response).to have_key 'discussion'
+      expect(json_response['discussion']['notes'][0]['note']).to eq(request_params[:note][:note])
+    end
+
     context 'when merge_request_diff_head_sha present' do
       before do
         service_params = {

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);
       });
     });
   });

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).toBe(true);
+      expect(state.diffFiles[1].renderIt).toBe(true);
     });
   });
 
@@ -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);
+    });
+  });
 });

spec/javascripts/diffs/store/utils_spec.js

@@ -136,6 +136,7 @@ describe('DiffsStoreUtils', () => {
         note_project_id: '',
         target_type: options.noteableType,
         target_id: options.noteableData.id,
+        return_discussion: true,
         note: {
           noteable_type: options.noteableType,
           noteable_id: options.noteableData.id,
@@ -194,6 +195,7 @@ describe('DiffsStoreUtils', () => {
         note_project_id: '',
         target_type: options.noteableType,
         target_id: options.noteableData.id,
+        return_discussion: true,
         note: {
           noteable_type: options.noteableType,
           noteable_id: options.noteableData.id,