Commit 880afe22 authored by Phil Hughes's avatar Phil Hughes

Merge branch '30299-discussion-from-individual-note' into 'master'

Add reply to notes to turn into discussions

See merge request gitlab-org/gitlab-ce!24480
parents c5f1b834 a04d9ba9
...@@ -2,11 +2,13 @@ ...@@ -2,11 +2,13 @@
import { mapGetters } from 'vuex'; import { mapGetters } from 'vuex';
import Icon from '~/vue_shared/components/icon.vue'; import Icon from '~/vue_shared/components/icon.vue';
import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui';
import ReplyButton from './note_actions/reply_button.vue';
export default { export default {
name: 'NoteActions', name: 'NoteActions',
components: { components: {
Icon, Icon,
ReplyButton,
GlLoadingIcon, GlLoadingIcon,
}, },
directives: { directives: {
...@@ -21,6 +23,11 @@ export default { ...@@ -21,6 +23,11 @@ export default {
type: [String, Number], type: [String, Number],
required: true, required: true,
}, },
discussionId: {
type: String,
required: false,
default: '',
},
noteUrl: { noteUrl: {
type: String, type: String,
required: false, required: false,
...@@ -36,6 +43,10 @@ export default { ...@@ -36,6 +43,10 @@ export default {
required: false, required: false,
default: null, default: null,
}, },
showReply: {
type: Boolean,
required: true,
},
canEdit: { canEdit: {
type: Boolean, type: Boolean,
required: true, required: true,
...@@ -80,6 +91,9 @@ export default { ...@@ -80,6 +91,9 @@ export default {
}, },
computed: { computed: {
...mapGetters(['getUserDataByProp']), ...mapGetters(['getUserDataByProp']),
showReplyButton() {
return gon.features && gon.features.replyToIndividualNotes && this.showReply;
},
shouldShowActionsDropdown() { shouldShowActionsDropdown() {
return this.currentUserId && (this.canEdit || this.canReportAsAbuse); return this.currentUserId && (this.canEdit || this.canReportAsAbuse);
}, },
...@@ -153,6 +167,12 @@ export default { ...@@ -153,6 +167,12 @@ export default {
<icon css-classes="link-highlight award-control-icon-super-positive" name="emoji_smiley" /> <icon css-classes="link-highlight award-control-icon-super-positive" name="emoji_smiley" />
</a> </a>
</div> </div>
<reply-button
v-if="showReplyButton"
ref="replyButton"
class="js-reply-button"
:note-id="discussionId"
/>
<div v-if="canEdit" class="note-actions-item"> <div v-if="canEdit" class="note-actions-item">
<button <button
v-gl-tooltip.bottom v-gl-tooltip.bottom
......
<script>
import { mapActions } from 'vuex';
import { GlTooltipDirective, GlButton } from '@gitlab/ui';
import Icon from '~/vue_shared/components/icon.vue';
export default {
name: 'ReplyButton',
components: {
Icon,
GlButton,
},
directives: {
GlTooltip: GlTooltipDirective,
},
props: {
noteId: {
type: String,
required: true,
},
},
methods: {
...mapActions(['convertToDiscussion']),
},
};
</script>
<template>
<div class="note-actions-item">
<gl-button
ref="button"
v-gl-tooltip.bottom
class="note-action-button"
variant="transparent"
:title="__('Reply to comment')"
@click="convertToDiscussion(noteId)"
>
<icon name="comment" css-classes="link-highlight" />
</gl-button>
</div>
</template>
...@@ -398,6 +398,7 @@ Please check your network connection and try again.`; ...@@ -398,6 +398,7 @@ Please check your network connection and try again.`;
:line="line" :line="line"
:commit="commit" :commit="commit"
:help-page-path="helpPagePath" :help-page-path="helpPagePath"
:show-reply-button="canReply"
@handleDeleteNote="deleteNoteHandler" @handleDeleteNote="deleteNoteHandler"
> >
<note-edited-text <note-edited-text
......
...@@ -29,6 +29,11 @@ export default { ...@@ -29,6 +29,11 @@ export default {
type: Object, type: Object,
required: true, required: true,
}, },
discussion: {
type: Object,
required: false,
default: null,
},
line: { line: {
type: Object, type: Object,
required: false, required: false,
...@@ -54,7 +59,7 @@ export default { ...@@ -54,7 +59,7 @@ export default {
}; };
}, },
computed: { computed: {
...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData']), ...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData', 'commentsDisabled']),
author() { author() {
return this.note.author; return this.note.author;
}, },
...@@ -80,6 +85,19 @@ export default { ...@@ -80,6 +85,19 @@ export default {
isTarget() { isTarget() {
return this.targetNoteHash === this.noteAnchorId; return this.targetNoteHash === this.noteAnchorId;
}, },
discussionId() {
if (this.discussion) {
return this.discussion.id;
}
return '';
},
showReplyButton() {
if (!this.discussion || !this.getNoteableData.current_user.can_create_note) {
return false;
}
return this.discussion.individual_note && !this.commentsDisabled;
},
actionText() { actionText() {
if (!this.commit) { if (!this.commit) {
return ''; return '';
...@@ -231,6 +249,7 @@ export default { ...@@ -231,6 +249,7 @@ export default {
:note-id="note.id" :note-id="note.id"
:note-url="note.noteable_note_url" :note-url="note.noteable_note_url"
:access-level="note.human_access" :access-level="note.human_access"
:show-reply="showReplyButton"
:can-edit="note.current_user.can_edit" :can-edit="note.current_user.can_edit"
:can-award-emoji="note.current_user.can_award_emoji" :can-award-emoji="note.current_user.can_award_emoji"
:can-delete="note.current_user.can_edit" :can-delete="note.current_user.can_edit"
...@@ -241,6 +260,7 @@ export default { ...@@ -241,6 +260,7 @@ export default {
:is-resolved="note.resolved" :is-resolved="note.resolved"
:is-resolving="isResolving" :is-resolving="isResolving"
:resolved-by="note.resolved_by" :resolved-by="note.resolved_by"
:discussion-id="discussionId"
@handleEdit="editHandler" @handleEdit="editHandler"
@handleDelete="deleteHandler" @handleDelete="deleteHandler"
@handleResolve="resolveHandler" @handleResolve="resolveHandler"
......
...@@ -199,7 +199,12 @@ export default { ...@@ -199,7 +199,12 @@ export default {
:key="discussion.id" :key="discussion.id"
:note="discussion.notes[0]" :note="discussion.notes[0]"
/> />
<noteable-note v-else :key="discussion.id" :note="discussion.notes[0]" /> <noteable-note
v-else
:key="discussion.id"
:note="discussion.notes[0]"
:discussion="discussion"
/>
</template> </template>
<noteable-discussion <noteable-discussion
v-else v-else
......
...@@ -426,5 +426,8 @@ export const submitSuggestion = ( ...@@ -426,5 +426,8 @@ export const submitSuggestion = (
}); });
}; };
export const convertToDiscussion = ({ commit }, noteId) =>
commit(types.CONVERT_TO_DISCUSSION, noteId);
// prevent babel-plugin-rewire from generating an invalid default during karma tests // prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {}; export default () => {};
...@@ -17,6 +17,7 @@ export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; ...@@ -17,6 +17,7 @@ export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE'; export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
export const DISABLE_COMMENTS = 'DISABLE_COMMENTS'; export const DISABLE_COMMENTS = 'DISABLE_COMMENTS';
export const APPLY_SUGGESTION = 'APPLY_SUGGESTION'; export const APPLY_SUGGESTION = 'APPLY_SUGGESTION';
export const CONVERT_TO_DISCUSSION = 'CONVERT_TO_DISCUSSION';
// DISCUSSION // DISCUSSION
export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
......
...@@ -264,4 +264,9 @@ export default { ...@@ -264,4 +264,9 @@ export default {
).length; ).length;
state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1; state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1;
}, },
[types.CONVERT_TO_DISCUSSION](state, discussionId) {
const discussion = utils.findNoteObjectById(state.discussions, discussionId);
Object.assign(discussion, { individual_note: false });
},
}; };
...@@ -7,6 +7,9 @@ module IssuableActions ...@@ -7,6 +7,9 @@ module IssuableActions
included do included do
before_action :authorize_destroy_issuable!, only: :destroy before_action :authorize_destroy_issuable!, only: :destroy
before_action :authorize_admin_issuable!, only: :bulk_update before_action :authorize_admin_issuable!, only: :bulk_update
before_action only: :show do
push_frontend_feature_flag(:reply_to_individual_notes)
end
end end
def permitted_keys def permitted_keys
......
# frozen_string_literal: true # frozen_string_literal: true
module Noteable module Noteable
# Names of all implementers of `Noteable` that support resolvable notes. extend ActiveSupport::Concern
# `Noteable` class names that support resolvable notes.
RESOLVABLE_TYPES = %w(MergeRequest).freeze RESOLVABLE_TYPES = %w(MergeRequest).freeze
class_methods do
# `Noteable` class names that support replying to individual notes.
def replyable_types
%w(Issue MergeRequest)
end
end
def base_class_name def base_class_name
self.class.base_class.name self.class.base_class.name
end end
...@@ -26,6 +35,10 @@ module Noteable ...@@ -26,6 +35,10 @@ module Noteable
DiscussionNote.noteable_types.include?(base_class_name) DiscussionNote.noteable_types.include?(base_class_name)
end end
def supports_replying_to_individual_notes?
supports_discussions? && self.class.replyable_types.include?(base_class_name)
end
def supports_suggestion? def supports_suggestion?
false false
end end
......
...@@ -17,6 +17,8 @@ class Discussion ...@@ -17,6 +17,8 @@ class Discussion
:for_commit?, :for_commit?,
:for_merge_request?, :for_merge_request?,
:save,
to: :first_note to: :first_note
def project_id def project_id
...@@ -116,6 +118,10 @@ class Discussion ...@@ -116,6 +118,10 @@ class Discussion
false false
end end
def can_convert_to_discussion?
false
end
def new_discussion? def new_discussion?
notes.length == 1 notes.length == 1
end end
......
...@@ -13,6 +13,14 @@ class IndividualNoteDiscussion < Discussion ...@@ -13,6 +13,14 @@ class IndividualNoteDiscussion < Discussion
true true
end end
def can_convert_to_discussion?
noteable.supports_replying_to_individual_notes? && Feature.enabled?(:reply_to_individual_notes)
end
def convert_to_discussion!
first_note.becomes!(Discussion.note_class).to_discussion
end
def reply_attributes def reply_attributes
super.tap { |attrs| attrs.delete(:discussion_id) } super.tap { |attrs| attrs.delete(:discussion_id) }
end end
......
...@@ -48,7 +48,7 @@ class SentNotification < ActiveRecord::Base ...@@ -48,7 +48,7 @@ class SentNotification < ActiveRecord::Base
end end
def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {}) def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {})
attrs[:in_reply_to_discussion_id] = note.discussion_id attrs[:in_reply_to_discussion_id] = note.discussion_id if note.part_of_discussion?
record(note.noteable, recipient_id, reply_key, attrs) record(note.noteable, recipient_id, reply_key, attrs)
end end
...@@ -99,29 +99,12 @@ class SentNotification < ActiveRecord::Base ...@@ -99,29 +99,12 @@ class SentNotification < ActiveRecord::Base
private private
def reply_params def reply_params
attrs = { {
noteable_type: self.noteable_type, noteable_type: self.noteable_type,
noteable_id: self.noteable_id, noteable_id: self.noteable_id,
commit_id: self.commit_id commit_id: self.commit_id,
in_reply_to_discussion_id: self.in_reply_to_discussion_id
} }
if self.in_reply_to_discussion_id.present?
attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id
else
# Remove in GitLab 10.0, when we will not support replying to SentNotifications
# that don't have `in_reply_to_discussion_id` anymore.
attrs.merge!(
type: self.note_type,
# LegacyDiffNote
line_code: self.line_code,
# DiffNote
position: self.position.to_json
)
end
attrs
end end
def note_valid def note_valid
......
...@@ -15,6 +15,8 @@ module Notes ...@@ -15,6 +15,8 @@ module Notes
return note return note
end end
discussion = discussion.convert_to_discussion! if discussion.can_convert_to_discussion?
params.merge!(discussion.reply_attributes) params.merge!(discussion.reply_attributes)
should_resolve = discussion.resolved? should_resolve = discussion.resolved?
end end
......
...@@ -34,6 +34,10 @@ module Notes ...@@ -34,6 +34,10 @@ module Notes
end end
if !only_commands && note.save if !only_commands && note.save
if note.part_of_discussion? && note.discussion.can_convert_to_discussion?
note.discussion.convert_to_discussion!.save(touch: false)
end
todo_service.new_note(note, current_user) todo_service.new_note(note, current_user)
clear_noteable_diffs_cache(note) clear_noteable_diffs_cache(note)
Suggestions::CreateService.new(note).execute Suggestions::CreateService.new(note).execute
......
...@@ -5959,6 +5959,9 @@ msgstr "" ...@@ -5959,6 +5959,9 @@ msgstr ""
msgid "Reopen milestone" msgid "Reopen milestone"
msgstr "" msgstr ""
msgid "Reply to comment"
msgstr ""
msgid "Reply to this email directly or %{view_it_on_gitlab}." msgid "Reply to this email directly or %{view_it_on_gitlab}."
msgstr "" msgstr ""
......
...@@ -66,6 +66,38 @@ describe 'Merge request > User posts notes', :js do ...@@ -66,6 +66,38 @@ describe 'Merge request > User posts notes', :js do
is_expected.to have_css('.js-note-text', visible: true) is_expected.to have_css('.js-note-text', visible: true)
end end
end end
describe 'when reply_to_individual_notes feature flag is not set' do
before do
stub_feature_flags(reply_to_individual_notes: false)
visit project_merge_request_path(project, merge_request)
end
it 'does not show a reply button' do
expect(page).to have_no_selector('.js-reply-button')
end
end
describe 'when reply_to_individual_notes feature flag is set' do
before do
stub_feature_flags(reply_to_individual_notes: true)
visit project_merge_request_path(project, merge_request)
end
it 'shows a reply button' do
reply_button = find('.js-reply-button', match: :first)
expect(reply_button).to have_selector('.ic-comment')
end
it 'shows reply placeholder when clicking reply button' do
reply_button = find('.js-reply-button', match: :first)
reply_button.click
expect(page).to have_selector('.discussion-reply-holder')
end
end
end end
describe 'when previewing a note' do describe 'when previewing a note' do
......
import Vuex from 'vuex';
import { createLocalVue, mount } from '@vue/test-utils';
import ReplyButton from '~/notes/components/note_actions/reply_button.vue';
describe('ReplyButton', () => {
const noteId = 'dummy-note-id';
let wrapper;
let convertToDiscussion;
beforeEach(() => {
const localVue = createLocalVue();
convertToDiscussion = jasmine.createSpy('convertToDiscussion');
localVue.use(Vuex);
const store = new Vuex.Store({
actions: {
convertToDiscussion,
},
});
wrapper = mount(ReplyButton, {
propsData: {
noteId,
},
store,
sync: false,
localVue,
});
});
afterEach(() => {
wrapper.destroy();
});
it('dispatches convertToDiscussion with note ID on click', () => {
const button = wrapper.find({ ref: 'button' });
button.trigger('click');
expect(convertToDiscussion).toHaveBeenCalledTimes(1);
const [, payload] = convertToDiscussion.calls.argsFor(0);
expect(payload).toBe(noteId);
});
});
...@@ -2,24 +2,26 @@ import Vue from 'vue'; ...@@ -2,24 +2,26 @@ import Vue from 'vue';
import { shallowMount, createLocalVue } from '@vue/test-utils'; import { shallowMount, createLocalVue } from '@vue/test-utils';
import createStore from '~/notes/stores'; import createStore from '~/notes/stores';
import noteActions from '~/notes/components/note_actions.vue'; import noteActions from '~/notes/components/note_actions.vue';
import { TEST_HOST } from 'spec/test_constants';
import { userDataMock } from '../mock_data'; import { userDataMock } from '../mock_data';
describe('noteActions', () => { describe('noteActions', () => {
let wrapper; let wrapper;
let store; let store;
let props;
beforeEach(() => { const createWrapper = propsData => {
store = createStore(); const localVue = createLocalVue();
}); return shallowMount(noteActions, {
store,
afterEach(() => { propsData,
wrapper.destroy(); localVue,
sync: false,
}); });
};
describe('user is logged in', () => {
let props;
beforeEach(() => { beforeEach(() => {
store = createStore();
props = { props = {
accessLevel: 'Maintainer', accessLevel: 'Maintainer',
authorId: 26, authorId: 26,
...@@ -28,20 +30,21 @@ describe('noteActions', () => { ...@@ -28,20 +30,21 @@ describe('noteActions', () => {
canAwardEmoji: true, canAwardEmoji: true,
canReportAsAbuse: true, canReportAsAbuse: true,
noteId: '539', noteId: '539',
noteUrl: 'https://localhost:3000/group/project/merge_requests/1#note_1', noteUrl: `${TEST_HOST}/group/project/merge_requests/1#note_1`,
reportAbusePath: reportAbusePath: `${TEST_HOST}/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26`,
'/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26', showReply: false,
}; };
});
afterEach(() => {
wrapper.destroy();
});
describe('user is logged in', () => {
beforeEach(() => {
store.dispatch('setUserData', userDataMock); store.dispatch('setUserData', userDataMock);
const localVue = createLocalVue(); wrapper = createWrapper(props);
wrapper = shallowMount(noteActions, {
store,
propsData: props,
localVue,
sync: false,
});
}); });
it('should render access level badge', () => { it('should render access level badge', () => {
...@@ -91,28 +94,14 @@ describe('noteActions', () => { ...@@ -91,28 +94,14 @@ describe('noteActions', () => {
}); });
describe('user is not logged in', () => { describe('user is not logged in', () => {
let props;
beforeEach(() => { beforeEach(() => {
store.dispatch('setUserData', {}); store.dispatch('setUserData', {});
props = { wrapper = createWrapper({
accessLevel: 'Maintainer', ...props,
authorId: 26,
canDelete: false, canDelete: false,
canEdit: false, canEdit: false,
canAwardEmoji: false, canAwardEmoji: false,
canReportAsAbuse: false, canReportAsAbuse: false,
noteId: '539',
noteUrl: 'https://localhost:3000/group/project/merge_requests/1#note_1',
reportAbusePath:
'/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26',
};
const localVue = createLocalVue();
wrapper = shallowMount(noteActions, {
store,
propsData: props,
localVue,
sync: false,
}); });
}); });
...@@ -124,4 +113,88 @@ describe('noteActions', () => { ...@@ -124,4 +113,88 @@ describe('noteActions', () => {
expect(wrapper.find('.more-actions').exists()).toBe(false); expect(wrapper.find('.more-actions').exists()).toBe(false);
}); });
}); });
describe('with feature flag replyToIndividualNotes enabled', () => {
beforeEach(() => {
gon.features = {
replyToIndividualNotes: true,
};
});
afterEach(() => {
gon.features = {};
});
describe('for showReply = true', () => {
beforeEach(() => {
wrapper = createWrapper({
...props,
showReply: true,
});
});
it('shows a reply button', () => {
const replyButton = wrapper.find({ ref: 'replyButton' });
expect(replyButton.exists()).toBe(true);
});
});
describe('for showReply = false', () => {
beforeEach(() => {
wrapper = createWrapper({
...props,
showReply: false,
});
});
it('does not show a reply button', () => {
const replyButton = wrapper.find({ ref: 'replyButton' });
expect(replyButton.exists()).toBe(false);
});
});
});
describe('with feature flag replyToIndividualNotes disabled', () => {
beforeEach(() => {
gon.features = {
replyToIndividualNotes: false,
};
});
afterEach(() => {
gon.features = {};
});
describe('for showReply = true', () => {
beforeEach(() => {
wrapper = createWrapper({
...props,
showReply: true,
});
});
it('does not show a reply button', () => {
const replyButton = wrapper.find({ ref: 'replyButton' });
expect(replyButton.exists()).toBe(false);
});
});
describe('for showReply = false', () => {
beforeEach(() => {
wrapper = createWrapper({
...props,
showReply: false,
});
});
it('does not show a reply button', () => {
const replyButton = wrapper.find({ ref: 'replyButton' });
expect(replyButton.exists()).toBe(false);
});
});
});
}); });
...@@ -585,4 +585,18 @@ describe('Actions Notes Store', () => { ...@@ -585,4 +585,18 @@ describe('Actions Notes Store', () => {
); );
}); });
}); });
describe('convertToDiscussion', () => {
it('commits CONVERT_TO_DISCUSSION with noteId', done => {
const noteId = 'dummy-note-id';
testAction(
actions.convertToDiscussion,
noteId,
{},
[{ type: 'CONVERT_TO_DISCUSSION', payload: noteId }],
[],
done,
);
});
});
}); });
...@@ -517,4 +517,27 @@ describe('Notes Store mutations', () => { ...@@ -517,4 +517,27 @@ describe('Notes Store mutations', () => {
); );
}); });
}); });
describe('CONVERT_TO_DISCUSSION', () => {
let discussion;
let state;
beforeEach(() => {
discussion = {
id: 42,
individual_note: true,
};
state = { discussions: [discussion] };
});
it('toggles individual_note', () => {
mutations.CONVERT_TO_DISCUSSION(state, discussion.id);
expect(discussion.individual_note).toBe(false);
});
it('throws if discussion was not found', () => {
expect(() => mutations.CONVERT_TO_DISCUSSION(state, 99)).toThrow();
});
});
}); });
...@@ -155,11 +155,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -155,11 +155,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
it_behaves_like "checks permissions on noteable" it_behaves_like "checks permissions on noteable"
end end
context "when everything is fine" do shared_examples 'a reply to existing comment' do
before do
setup_attachment
end
it "creates a comment" do it "creates a comment" do
expect { receiver.execute }.to change { noteable.notes.count }.by(1) expect { receiver.execute }.to change { noteable.notes.count }.by(1)
new_note = noteable.notes.last new_note = noteable.notes.last
...@@ -168,8 +164,22 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -168,8 +164,22 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
expect(new_note.position).to eq(note.position) expect(new_note.position).to eq(note.position)
expect(new_note.note).to include("I could not disagree more.") expect(new_note.note).to include("I could not disagree more.")
expect(new_note.in_reply_to?(note)).to be_truthy expect(new_note.in_reply_to?(note)).to be_truthy
if note.part_of_discussion?
expect(new_note.discussion_id).to eq(note.discussion_id)
else
expect(new_note.discussion_id).not_to eq(note.discussion_id)
end
end
end end
context "when everything is fine" do
before do
setup_attachment
end
it_behaves_like 'a reply to existing comment'
it "adds all attachments" do it "adds all attachments" do
receiver.execute receiver.execute
...@@ -207,4 +217,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -207,4 +217,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
end end
end end
end end
context "when note is not a discussion" do
let(:note) { create(:note_on_merge_request, project: project) }
it_behaves_like 'a reply to existing comment'
end
end end
...@@ -36,19 +36,41 @@ describe SentNotification do ...@@ -36,19 +36,41 @@ describe SentNotification do
end end
end end
shared_examples 'a successful sent notification' do
it 'creates a new SentNotification' do
expect { subject }.to change { described_class.count }.by(1)
end
end
describe '.record' do describe '.record' do
let(:issue) { create(:issue) } let(:issue) { create(:issue) }
it 'creates a new SentNotification' do subject { described_class.record(issue, user.id) }
expect { described_class.record(issue, user.id) }.to change { described_class.count }.by(1)
end it_behaves_like 'a successful sent notification'
end end
describe '.record_note' do describe '.record_note' do
subject { described_class.record_note(note, note.author.id) }
context 'for a discussion note' do
let(:note) { create(:diff_note_on_merge_request) } let(:note) { create(:diff_note_on_merge_request) }
it 'creates a new SentNotification' do it_behaves_like 'a successful sent notification'
expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1)
it 'sets in_reply_to_discussion_id' do
expect(subject.in_reply_to_discussion_id).to eq(note.discussion_id)
end
end
context 'for an individual note' do
let(:note) { create(:note_on_merge_request) }
it_behaves_like 'a successful sent notification'
it 'does not set in_reply_to_discussion_id' do
expect(subject.in_reply_to_discussion_id).to be_nil
end
end end
end end
......
...@@ -123,6 +123,46 @@ describe Notes::BuildService do ...@@ -123,6 +123,46 @@ describe Notes::BuildService do
end end
end end
context 'when replying to individual note' do
let(:note) { create(:note_on_issue) }
subject { described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute }
shared_examples 'an individual note reply' do
it 'builds another individual note' do
expect(subject).to be_valid
expect(subject).to be_a(Note)
expect(subject.discussion_id).not_to eq(note.discussion_id)
end
end
context 'when reply_to_individual_notes is disabled' do
before do
stub_feature_flags(reply_to_individual_notes: false)
end
it_behaves_like 'an individual note reply'
end
context 'when reply_to_individual_notes is enabled' do
before do
stub_feature_flags(reply_to_individual_notes: true)
end
it 'sets the note up to be in reply to that note' do
expect(subject).to be_valid
expect(subject).to be_a(DiscussionNote)
expect(subject.discussion_id).to eq(note.discussion_id)
end
context 'when noteable does not support replies' do
let(:note) { create(:note_on_commit) }
it_behaves_like 'an individual note reply'
end
end
end
it 'builds a note without saving it' do it 'builds a note without saving it' do
new_note = described_class.new(project, new_note = described_class.new(project,
author, author,
......
...@@ -278,5 +278,42 @@ describe Notes::CreateService do ...@@ -278,5 +278,42 @@ describe Notes::CreateService do
expect(note.note).to eq(':smile:') expect(note.note).to eq(':smile:')
end end
end end
context 'reply to individual note' do
let(:existing_note) { create(:note_on_issue, noteable: issue, project: project) }
let(:reply_opts) { opts.merge(in_reply_to_discussion_id: existing_note.discussion_id) }
subject { described_class.new(project, user, reply_opts).execute }
context 'when reply_to_individual_notes is disabled' do
before do
stub_feature_flags(reply_to_individual_notes: false)
end
it 'creates an individual note' do
expect(subject.type).to eq(nil)
expect(subject.discussion_id).not_to eq(existing_note.discussion_id)
end
it 'does not convert existing note' do
expect { subject }.not_to change { existing_note.reload.type }
end
end
context 'when reply_to_individual_notes is enabled' do
before do
stub_feature_flags(reply_to_individual_notes: true)
end
it 'creates a DiscussionNote in reply to existing note' do
expect(subject).to be_a(DiscussionNote)
expect(subject.discussion_id).to eq(existing_note.discussion_id)
end
it 'converts existing note to DiscussionNote' do
expect { subject }.to change { existing_note.reload.type }.from(nil).to('DiscussionNote')
end
end
end
end end
end end
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