summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorFilipa Lacerda <filipa@gitlab.com>2018-09-24 09:21:34 +0000
committerFilipa Lacerda <filipa@gitlab.com>2018-09-24 09:21:34 +0000
commit421108c62841592dea15e87a6eb490ccd431af25 (patch)
tree6cac837d92394b09e5349dcd589a292964350787
parente9b13d1854b7b5f3258558f10e3d9e7f8d8ebc03 (diff)
parent21f02674bef5a653d80deaf1e21153926d02a363 (diff)
downloadgitlab-ce-421108c62841592dea15e87a6eb490ccd431af25.tar.gz
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
-rw-r--r--app/assets/javascripts/diffs/components/diff_line_note_form.vue48
-rw-r--r--app/assets/javascripts/diffs/store/actions.js19
-rw-r--r--app/assets/javascripts/diffs/store/utils.js1
-rw-r--r--app/assets/javascripts/notes/stores/actions.js22
-rw-r--r--app/assets/javascripts/notes/stores/mutations.js3
-rw-r--r--app/controllers/concerns/notes_actions.rb48
-rw-r--r--changelogs/unreleased/dm-create-note-return-discussion.yml5
-rw-r--r--locale/gitlab.pot3
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb8
-rw-r--r--spec/javascripts/diffs/components/diff_line_note_form_spec.js29
-rw-r--r--spec/javascripts/diffs/store/actions_spec.js46
-rw-r--r--spec/javascripts/diffs/store/utils_spec.js2
12 files changed, 140 insertions, 94 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/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index 834a94ea42a..631e3de311e 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/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,
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index 68df63b8539..320dfa47d5a 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);
-
- resolve(discussion);
- }
- })
- .catch(() => {});
- });
+export const updateDiscussion = ({ commit, state }, discussion) => {
+ commit(types.UPDATE_DISCUSSION, discussion);
+
+ return utils.findNoteObjectById(state.discussions, discussion.id);
+};
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/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index 9422a06387b..3a45d6205ab 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/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']
diff --git a/changelogs/unreleased/dm-create-note-return-discussion.yml b/changelogs/unreleased/dm-create-note-return-discussion.yml
new file mode 100644
index 00000000000..49ab5c0ca14
--- /dev/null
+++ b/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
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index bc5d2aeac46..d0e8937fdfc 100644
--- a/locale/gitlab.pot
+++ b/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 ""
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 81badaac76b..e48c9dea976 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/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 = {
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(),
);
-
- component.handleSaveNote('note body');
-
- expect(formDataSpy).toHaveBeenCalled();
- expect(saveNoteSpy).toHaveBeenCalled();
- expect(isPromiseCalled).toEqual(true);
+ spyOnProperty(component, 'formData').and.returnValue('formData');
+
+ 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..05b39bad6ea 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).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);
+ });
+ });
});
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index 6138b9701f4..897cd1483aa 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/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,