Commit 36993a83 authored by Justin Boyson's avatar Justin Boyson Committed by David O'Regan

Check for empty position object before stringify

Updates specs to check for correct string parsing
parent da75cd82
import { isEmpty } from 'lodash';
import { deprecatedCreateFlash as flash } from '~/flash'; import { deprecatedCreateFlash as flash } from '~/flash';
import { scrollToElement } from '~/lib/utils/common_utils'; import { scrollToElement } from '~/lib/utils/common_utils';
import { __ } from '~/locale'; import { __ } from '~/locale';
...@@ -88,18 +89,23 @@ export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGet ...@@ -88,18 +89,23 @@ export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGet
export const updateDraft = ( export const updateDraft = (
{ commit, getters }, { commit, getters },
{ note, noteText, resolveDiscussion, position, callback }, { note, noteText, resolveDiscussion, position, callback },
) => ) => {
service const params = {
.update(getters.getNotesData.draftsPath, {
draftId: note.id, draftId: note.id,
note: noteText, note: noteText,
resolveDiscussion, resolveDiscussion,
position: JSON.stringify(position), };
}) // Stringifying an empty object yields `{}` which breaks graphql queries
// https://gitlab.com/gitlab-org/gitlab/-/issues/298827
if (!isEmpty(position)) params.position = JSON.stringify(position);
return service
.update(getters.getNotesData.draftsPath, params)
.then((res) => res.data) .then((res) => res.data)
.then((data) => commit(types.RECEIVE_DRAFT_UPDATE_SUCCESS, data)) .then((data) => commit(types.RECEIVE_DRAFT_UPDATE_SUCCESS, data))
.then(callback) .then(callback)
.catch(() => flash(__('An error occurred while updating the comment'))); .catch(() => flash(__('An error occurred while updating the comment')));
};
export const scrollToDraft = ({ dispatch, rootGetters }, draft) => { export const scrollToDraft = ({ dispatch, rootGetters }, draft) => {
const discussion = draft.discussion_id && rootGetters.getDiscussion(draft.discussion_id); const discussion = draft.discussion_id && rootGetters.getDiscussion(draft.discussion_id);
......
<script> <script>
import { GlSprintf, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui'; import { GlSprintf, GlSafeHtmlDirective as SafeHtml } from '@gitlab/ui';
import $ from 'jquery'; import $ from 'jquery';
import { escape } from 'lodash'; import { escape, isEmpty } from 'lodash';
import { mapGetters, mapActions } from 'vuex'; import { mapGetters, mapActions } from 'vuex';
import { INLINE_DIFF_LINES_KEY } from '~/diffs/constants'; import { INLINE_DIFF_LINES_KEY } from '~/diffs/constants';
import httpStatusCodes from '~/lib/utils/http_status'; import httpStatusCodes from '~/lib/utils/http_status';
...@@ -282,9 +282,13 @@ export default { ...@@ -282,9 +282,13 @@ export default {
note: { note: {
target_type: this.getNoteableData.targetType, target_type: this.getNoteableData.targetType,
target_id: this.note.noteable_id, target_id: this.note.noteable_id,
note: { note: noteText, position: JSON.stringify(position) }, note: { note: noteText },
}, },
}; };
// Stringifying an empty object yields `{}` which breaks graphql queries
// https://gitlab.com/gitlab-org/gitlab/-/issues/298827
if (!isEmpty(position)) data.note.note.position = JSON.stringify(position);
this.isRequesting = true; this.isRequesting = true;
this.oldContent = this.note.note_html; this.oldContent = this.note.note_html;
// eslint-disable-next-line vue/no-mutating-props // eslint-disable-next-line vue/no-mutating-props
......
---
title: fix stringify empty position object
merge_request: 56037
author:
type: fixed
import MockAdapter from 'axios-mock-adapter'; import MockAdapter from 'axios-mock-adapter';
import { TEST_HOST } from 'helpers/test_constants'; import { TEST_HOST } from 'helpers/test_constants';
import testAction from 'helpers/vuex_action_helper'; import testAction from 'helpers/vuex_action_helper';
import service from '~/batch_comments/services/drafts_service';
import * as actions from '~/batch_comments/stores/modules/batch_comments/actions'; import * as actions from '~/batch_comments/stores/modules/batch_comments/actions';
import axios from '~/lib/utils/axios_utils'; import axios from '~/lib/utils/axios_utils';
...@@ -201,6 +202,12 @@ describe('Batch comments store actions', () => { ...@@ -201,6 +202,12 @@ describe('Batch comments store actions', () => {
describe('updateDraft', () => { describe('updateDraft', () => {
let getters; let getters;
service.update = jest.fn();
service.update.mockResolvedValue({ data: { id: 1 } });
const commit = jest.fn();
let context;
let params;
beforeEach(() => { beforeEach(() => {
getters = { getters = {
...@@ -208,43 +215,43 @@ describe('Batch comments store actions', () => { ...@@ -208,43 +215,43 @@ describe('Batch comments store actions', () => {
draftsPath: TEST_HOST, draftsPath: TEST_HOST,
}, },
}; };
});
it('commits RECEIVE_DRAFT_UPDATE_SUCCESS with returned data', (done) => { context = {
const commit = jest.fn();
const context = {
getters, getters,
commit, commit,
}; };
res = { id: 1 }; res = { id: 1 };
mock.onAny().reply(200, res); mock.onAny().reply(200, res);
params = { note: { id: 1 }, noteText: 'test' };
});
actions afterEach(() => jest.clearAllMocks());
.updateDraft(context, { note: { id: 1 }, noteText: 'test', callback() {} })
.then(() => { it('commits RECEIVE_DRAFT_UPDATE_SUCCESS with returned data', () => {
return actions.updateDraft(context, { ...params, callback() {} }).then(() => {
expect(commit).toHaveBeenCalledWith('RECEIVE_DRAFT_UPDATE_SUCCESS', { id: 1 }); expect(commit).toHaveBeenCalledWith('RECEIVE_DRAFT_UPDATE_SUCCESS', { id: 1 });
}) });
.then(done)
.catch(done.fail);
}); });
it('calls passed callback', (done) => { it('calls passed callback', () => {
const commit = jest.fn();
const context = {
getters,
commit,
};
const callback = jest.fn(); const callback = jest.fn();
res = { id: 1 }; return actions.updateDraft(context, { ...params, callback }).then(() => {
mock.onAny().reply(200, res);
actions
.updateDraft(context, { note: { id: 1 }, noteText: 'test', callback })
.then(() => {
expect(callback).toHaveBeenCalled(); expect(callback).toHaveBeenCalled();
}) });
.then(done) });
.catch(done.fail);
it('does not stringify empty position', () => {
return actions.updateDraft(context, { ...params, position: {}, callback() {} }).then(() => {
expect(service.update.mock.calls[0][1].position).toBeUndefined();
});
});
it('stringifies a non-empty position', () => {
const position = { test: true };
const expectation = JSON.stringify(position);
return actions.updateDraft(context, { ...params, position, callback() {} }).then(() => {
expect(service.update.mock.calls[0][1].position).toBe(expectation);
});
}); });
}); });
......
import { mount, createLocalVue } from '@vue/test-utils'; import { mount, createLocalVue } from '@vue/test-utils';
import { escape } from 'lodash'; import { escape } from 'lodash';
import waitForPromises from 'helpers/wait_for_promises';
import NoteActions from '~/notes/components/note_actions.vue'; import NoteActions from '~/notes/components/note_actions.vue';
import NoteBody from '~/notes/components/note_body.vue'; import NoteBody from '~/notes/components/note_body.vue';
import NoteHeader from '~/notes/components/note_header.vue'; import NoteHeader from '~/notes/components/note_header.vue';
...@@ -13,7 +14,7 @@ describe('issue_note', () => { ...@@ -13,7 +14,7 @@ describe('issue_note', () => {
let wrapper; let wrapper;
const findMultilineComment = () => wrapper.find('[data-testid="multiline-comment"]'); const findMultilineComment = () => wrapper.find('[data-testid="multiline-comment"]');
beforeEach(() => { const createWrapper = (props = {}) => {
store = createStore(); store = createStore();
store.dispatch('setNoteableData', noteableDataMock); store.dispatch('setNoteableData', noteableDataMock);
store.dispatch('setNotesData', notesDataMock); store.dispatch('setNotesData', notesDataMock);
...@@ -23,6 +24,7 @@ describe('issue_note', () => { ...@@ -23,6 +24,7 @@ describe('issue_note', () => {
store, store,
propsData: { propsData: {
note, note,
...props,
}, },
localVue, localVue,
stubs: [ stubs: [
...@@ -33,14 +35,18 @@ describe('issue_note', () => { ...@@ -33,14 +35,18 @@ describe('issue_note', () => {
'multiline-comment-form', 'multiline-comment-form',
], ],
}); });
}); };
afterEach(() => { afterEach(() => {
wrapper.destroy(); wrapper.destroy();
}); });
describe('mutiline comments', () => { describe('mutiline comments', () => {
it('should render if has multiline comment', () => { beforeEach(() => {
createWrapper();
});
it('should render if has multiline comment', async () => {
const position = { const position = {
line_range: { line_range: {
start: { start: {
...@@ -69,9 +75,8 @@ describe('issue_note', () => { ...@@ -69,9 +75,8 @@ describe('issue_note', () => {
line, line,
}); });
return wrapper.vm.$nextTick().then(() => { await wrapper.vm.$nextTick();
expect(findMultilineComment().text()).toEqual('Comment on lines 1 to 2'); expect(findMultilineComment().text()).toBe('Comment on lines 1 to 2');
});
}); });
it('should only render if it has everything it needs', () => { it('should only render if it has everything it needs', () => {
...@@ -147,9 +152,14 @@ describe('issue_note', () => { ...@@ -147,9 +152,14 @@ describe('issue_note', () => {
}); });
}); });
describe('rendering', () => {
beforeEach(() => {
createWrapper();
});
it('should render user information', () => { it('should render user information', () => {
const { author } = note; const { author } = note;
const avatar = wrapper.find(UserAvatarLink); const avatar = wrapper.findComponent(UserAvatarLink);
const avatarProps = avatar.props(); const avatarProps = avatar.props();
expect(avatarProps.linkHref).toBe(author.path); expect(avatarProps.linkHref).toBe(author.path);
...@@ -159,17 +169,17 @@ describe('issue_note', () => { ...@@ -159,17 +169,17 @@ describe('issue_note', () => {
}); });
it('should render note header content', () => { it('should render note header content', () => {
const noteHeader = wrapper.find(NoteHeader); const noteHeader = wrapper.findComponent(NoteHeader);
const noteHeaderProps = noteHeader.props(); const noteHeaderProps = noteHeader.props();
expect(noteHeaderProps.author).toEqual(note.author); expect(noteHeaderProps.author).toBe(note.author);
expect(noteHeaderProps.createdAt).toEqual(note.created_at); expect(noteHeaderProps.createdAt).toBe(note.created_at);
expect(noteHeaderProps.noteId).toEqual(note.id); expect(noteHeaderProps.noteId).toBe(note.id);
}); });
it('should render note actions', () => { it('should render note actions', () => {
const { author } = note; const { author } = note;
const noteActions = wrapper.find(NoteActions); const noteActions = wrapper.findComponent(NoteActions);
const noteActionsProps = noteActions.props(); const noteActionsProps = noteActions.props();
expect(noteActionsProps.authorId).toBe(author.id); expect(noteActionsProps.authorId).toBe(author.id);
...@@ -189,66 +199,104 @@ describe('issue_note', () => { ...@@ -189,66 +199,104 @@ describe('issue_note', () => {
}); });
it('should render issue body', () => { it('should render issue body', () => {
const noteBody = wrapper.find(NoteBody); const noteBody = wrapper.findComponent(NoteBody);
const noteBodyProps = noteBody.props(); const noteBodyProps = noteBody.props();
expect(noteBodyProps.note).toEqual(note); expect(noteBodyProps.note).toBe(note);
expect(noteBodyProps.line).toBe(null); expect(noteBodyProps.line).toBe(null);
expect(noteBodyProps.canEdit).toBe(note.current_user.can_edit); expect(noteBodyProps.canEdit).toBe(note.current_user.can_edit);
expect(noteBodyProps.isEditing).toBe(false); expect(noteBodyProps.isEditing).toBe(false);
expect(noteBodyProps.helpPagePath).toBe(''); expect(noteBodyProps.helpPagePath).toBe('');
}); });
it('prevents note preview xss', (done) => { it('prevents note preview xss', async () => {
const imgSrc = 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'; const noteBody =
const noteBody = `<img src="${imgSrc}" onload="alert(1)" />`; '<img src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" onload="alert(1)" />';
const alertSpy = jest.spyOn(window, 'alert'); const alertSpy = jest.spyOn(window, 'alert').mockImplementation(() => {});
const noteBodyComponent = wrapper.findComponent(NoteBody);
store.hotUpdate({ store.hotUpdate({
actions: { actions: {
updateNote() {}, updateNote() {},
setSelectedCommentPositionHover() {}, setSelectedCommentPositionHover() {},
}, },
}); });
const noteBodyComponent = wrapper.find(NoteBody);
noteBodyComponent.vm.$emit('handleFormUpdate', noteBody, null, () => {}); noteBodyComponent.vm.$emit('handleFormUpdate', noteBody, null, () => {});
setImmediate(() => { await waitForPromises();
expect(alertSpy).not.toHaveBeenCalled(); expect(alertSpy).not.toHaveBeenCalled();
expect(wrapper.vm.note.note_html).toEqual(escape(noteBody)); expect(wrapper.vm.note.note_html).toBe(escape(noteBody));
done();
}); });
}); });
describe('cancel edit', () => { describe('cancel edit', () => {
it('restores content of updated note', (done) => { beforeEach(() => {
createWrapper();
});
it('restores content of updated note', async () => {
const updatedText = 'updated note text'; const updatedText = 'updated note text';
store.hotUpdate({ store.hotUpdate({
actions: { actions: {
updateNote() {}, updateNote() {},
}, },
}); });
const noteBody = wrapper.find(NoteBody); const noteBody = wrapper.findComponent(NoteBody);
noteBody.vm.resetAutoSave = () => {}; noteBody.vm.resetAutoSave = () => {};
noteBody.vm.$emit('handleFormUpdate', updatedText, null, () => {}); noteBody.vm.$emit('handleFormUpdate', updatedText, null, () => {});
wrapper.vm await wrapper.vm.$nextTick();
.$nextTick() let noteBodyProps = noteBody.props();
.then(() => {
const noteBodyProps = noteBody.props();
expect(noteBodyProps.note.note_html).toBe(updatedText); expect(noteBodyProps.note.note_html).toBe(updatedText);
noteBody.vm.$emit('cancelForm'); noteBody.vm.$emit('cancelForm');
}) await wrapper.vm.$nextTick();
.then(() => wrapper.vm.$nextTick())
.then(() => { noteBodyProps = noteBody.props();
const noteBodyProps = noteBody.props();
expect(noteBodyProps.note.note_html).toBe(note.note_html); expect(noteBodyProps.note.note_html).toBe(note.note_html);
}) });
.then(done) });
.catch(done.fail);
describe('formUpdateHandler', () => {
const updateNote = jest.fn();
const params = ['', null, jest.fn(), ''];
const updateActions = () => {
store.hotUpdate({
actions: {
updateNote,
setSelectedCommentPositionHover() {},
},
});
};
afterEach(() => updateNote.mockReset());
it('responds to handleFormUpdate', () => {
createWrapper();
updateActions();
wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
expect(wrapper.emitted('handleUpdateNote')).toBeTruthy();
});
it('does not stringify empty position', () => {
createWrapper();
updateActions();
wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
expect(updateNote.mock.calls[0][1].note.note.position).toBeUndefined();
});
it('stringifies populated position', () => {
const position = { test: true };
const expectation = JSON.stringify(position);
createWrapper({ note: { ...note, position } });
updateActions();
wrapper.findComponent(NoteBody).vm.$emit('handleFormUpdate', ...params);
expect(updateNote.mock.calls[0][1].note.note.position).toBe(expectation);
}); });
}); });
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment