summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPhil Hughes <me@iamphill.com>2019-02-06 10:31:46 +0000
committerPhil Hughes <me@iamphill.com>2019-02-06 10:31:46 +0000
commit880afe2246440739fa71fc80d6446679074fc325 (patch)
tree4a8889592e2290160926ede70aa0cf37bd93519f
parentc5f1b8346860aa764369a3d156047c9e2c7317ca (diff)
parenta04d9ba90c0d1df02f613b7aa01ef598e1ac5b28 (diff)
downloadgitlab-ce-880afe2246440739fa71fc80d6446679074fc325.tar.gz
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
-rw-r--r--app/assets/javascripts/notes/components/note_actions.vue20
-rw-r--r--app/assets/javascripts/notes/components/note_actions/reply_button.vue40
-rw-r--r--app/assets/javascripts/notes/components/noteable_discussion.vue1
-rw-r--r--app/assets/javascripts/notes/components/noteable_note.vue22
-rw-r--r--app/assets/javascripts/notes/components/notes_app.vue7
-rw-r--r--app/assets/javascripts/notes/stores/actions.js3
-rw-r--r--app/assets/javascripts/notes/stores/mutation_types.js1
-rw-r--r--app/assets/javascripts/notes/stores/mutations.js5
-rw-r--r--app/controllers/concerns/issuable_actions.rb3
-rw-r--r--app/models/concerns/noteable.rb15
-rw-r--r--app/models/discussion.rb6
-rw-r--r--app/models/individual_note_discussion.rb8
-rw-r--r--app/models/sent_notification.rb25
-rw-r--r--app/services/notes/build_service.rb2
-rw-r--r--app/services/notes/create_service.rb4
-rw-r--r--locale/gitlab.pot3
-rw-r--r--spec/features/merge_request/user_posts_notes_spec.rb32
-rw-r--r--spec/javascripts/notes/components/note_actions/reply_button_spec.js46
-rw-r--r--spec/javascripts/notes/components/note_actions_spec.js149
-rw-r--r--spec/javascripts/notes/stores/actions_spec.js14
-rw-r--r--spec/javascripts/notes/stores/mutation_spec.js23
-rw-r--r--spec/lib/gitlab/email/handler/create_note_handler_spec.rb26
-rw-r--r--spec/models/sent_notification_spec.rb34
-rw-r--r--spec/services/notes/build_service_spec.rb40
-rw-r--r--spec/services/notes/create_service_spec.rb37
25 files changed, 493 insertions, 73 deletions
diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue
index d99694b06e9..394f2a80a67 100644
--- a/app/assets/javascripts/notes/components/note_actions.vue
+++ b/app/assets/javascripts/notes/components/note_actions.vue
@@ -2,11 +2,13 @@
import { mapGetters } from 'vuex';
import Icon from '~/vue_shared/components/icon.vue';
import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui';
+import ReplyButton from './note_actions/reply_button.vue';
export default {
name: 'NoteActions',
components: {
Icon,
+ ReplyButton,
GlLoadingIcon,
},
directives: {
@@ -21,6 +23,11 @@ export default {
type: [String, Number],
required: true,
},
+ discussionId: {
+ type: String,
+ required: false,
+ default: '',
+ },
noteUrl: {
type: String,
required: false,
@@ -36,6 +43,10 @@ export default {
required: false,
default: null,
},
+ showReply: {
+ type: Boolean,
+ required: true,
+ },
canEdit: {
type: Boolean,
required: true,
@@ -80,6 +91,9 @@ export default {
},
computed: {
...mapGetters(['getUserDataByProp']),
+ showReplyButton() {
+ return gon.features && gon.features.replyToIndividualNotes && this.showReply;
+ },
shouldShowActionsDropdown() {
return this.currentUserId && (this.canEdit || this.canReportAsAbuse);
},
@@ -153,6 +167,12 @@ export default {
<icon css-classes="link-highlight award-control-icon-super-positive" name="emoji_smiley" />
</a>
</div>
+ <reply-button
+ v-if="showReplyButton"
+ ref="replyButton"
+ class="js-reply-button"
+ :note-id="discussionId"
+ />
<div v-if="canEdit" class="note-actions-item">
<button
v-gl-tooltip.bottom
diff --git a/app/assets/javascripts/notes/components/note_actions/reply_button.vue b/app/assets/javascripts/notes/components/note_actions/reply_button.vue
new file mode 100644
index 00000000000..b2f9d7f128a
--- /dev/null
+++ b/app/assets/javascripts/notes/components/note_actions/reply_button.vue
@@ -0,0 +1,40 @@
+<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>
diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index e26cce1c47f..b7e9f7c2028 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -398,6 +398,7 @@ Please check your network connection and try again.`;
:line="line"
:commit="commit"
:help-page-path="helpPagePath"
+ :show-reply-button="canReply"
@handleDeleteNote="deleteNoteHandler"
>
<note-edited-text
diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue
index 3c48d81ed05..56108a58010 100644
--- a/app/assets/javascripts/notes/components/noteable_note.vue
+++ b/app/assets/javascripts/notes/components/noteable_note.vue
@@ -29,6 +29,11 @@ export default {
type: Object,
required: true,
},
+ discussion: {
+ type: Object,
+ required: false,
+ default: null,
+ },
line: {
type: Object,
required: false,
@@ -54,7 +59,7 @@ export default {
};
},
computed: {
- ...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData']),
+ ...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData', 'commentsDisabled']),
author() {
return this.note.author;
},
@@ -80,6 +85,19 @@ export default {
isTarget() {
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() {
if (!this.commit) {
return '';
@@ -231,6 +249,7 @@ export default {
:note-id="note.id"
:note-url="note.noteable_note_url"
:access-level="note.human_access"
+ :show-reply="showReplyButton"
:can-edit="note.current_user.can_edit"
:can-award-emoji="note.current_user.can_award_emoji"
:can-delete="note.current_user.can_edit"
@@ -241,6 +260,7 @@ export default {
:is-resolved="note.resolved"
:is-resolving="isResolving"
:resolved-by="note.resolved_by"
+ :discussion-id="discussionId"
@handleEdit="editHandler"
@handleDelete="deleteHandler"
@handleResolve="resolveHandler"
diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue
index 5edceea043c..6d72b72e628 100644
--- a/app/assets/javascripts/notes/components/notes_app.vue
+++ b/app/assets/javascripts/notes/components/notes_app.vue
@@ -199,7 +199,12 @@ export default {
:key="discussion.id"
: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>
<noteable-discussion
v-else
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index 2105a62cecb..ff65f14d529 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -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
export default () => {};
diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js
index df943c155f4..2bffedad336 100644
--- a/app/assets/javascripts/notes/stores/mutation_types.js
+++ b/app/assets/javascripts/notes/stores/mutation_types.js
@@ -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 DISABLE_COMMENTS = 'DISABLE_COMMENTS';
export const APPLY_SUGGESTION = 'APPLY_SUGGESTION';
+export const CONVERT_TO_DISCUSSION = 'CONVERT_TO_DISCUSSION';
// DISCUSSION
export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index 33d39ad2ec9..d167f8ef421 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -264,4 +264,9 @@ export default {
).length;
state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1;
},
+
+ [types.CONVERT_TO_DISCUSSION](state, discussionId) {
+ const discussion = utils.findNoteObjectById(state.discussions, discussionId);
+ Object.assign(discussion, { individual_note: false });
+ },
};
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 8ef3b6502df..cd3fa641e89 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -7,6 +7,9 @@ module IssuableActions
included do
before_action :authorize_destroy_issuable!, only: :destroy
before_action :authorize_admin_issuable!, only: :bulk_update
+ before_action only: :show do
+ push_frontend_feature_flag(:reply_to_individual_notes)
+ end
end
def permitted_keys
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index 29476654bf7..3c74034b527 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -1,9 +1,18 @@
# frozen_string_literal: true
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
+ class_methods do
+ # `Noteable` class names that support replying to individual notes.
+ def replyable_types
+ %w(Issue MergeRequest)
+ end
+ end
+
def base_class_name
self.class.base_class.name
end
@@ -26,6 +35,10 @@ module Noteable
DiscussionNote.noteable_types.include?(base_class_name)
end
+ def supports_replying_to_individual_notes?
+ supports_discussions? && self.class.replyable_types.include?(base_class_name)
+ end
+
def supports_suggestion?
false
end
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index dbc7b6e67be..f2678e0597d 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -17,6 +17,8 @@ class Discussion
:for_commit?,
:for_merge_request?,
+ :save,
+
to: :first_note
def project_id
@@ -116,6 +118,10 @@ class Discussion
false
end
+ def can_convert_to_discussion?
+ false
+ end
+
def new_discussion?
notes.length == 1
end
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index 07ee7470ea2..aab0ff93468 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -13,6 +13,14 @@ class IndividualNoteDiscussion < Discussion
true
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
super.tap { |attrs| attrs.delete(:discussion_id) }
end
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index e65b3df0fb6..6caab24143b 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -48,7 +48,7 @@ class SentNotification < ActiveRecord::Base
end
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)
end
@@ -99,29 +99,12 @@ class SentNotification < ActiveRecord::Base
private
def reply_params
- attrs = {
+ {
noteable_type: self.noteable_type,
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
def note_valid
diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb
index bae98ede561..541f3e0d23c 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -15,6 +15,8 @@ module Notes
return note
end
+ discussion = discussion.convert_to_discussion! if discussion.can_convert_to_discussion?
+
params.merge!(discussion.reply_attributes)
should_resolve = discussion.resolved?
end
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index c4546f30235..b975c3a8cb6 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -34,6 +34,10 @@ module Notes
end
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)
clear_noteable_diffs_cache(note)
Suggestions::CreateService.new(note).execute
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index bdb2ed1e26e..1caf263b9f3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -5959,6 +5959,9 @@ msgstr ""
msgid "Reopen milestone"
msgstr ""
+msgid "Reply to comment"
+msgstr ""
+
msgid "Reply to this email directly or %{view_it_on_gitlab}."
msgstr ""
diff --git a/spec/features/merge_request/user_posts_notes_spec.rb b/spec/features/merge_request/user_posts_notes_spec.rb
index ee5f5377ca6..1bbcf455ac7 100644
--- a/spec/features/merge_request/user_posts_notes_spec.rb
+++ b/spec/features/merge_request/user_posts_notes_spec.rb
@@ -66,6 +66,38 @@ describe 'Merge request > User posts notes', :js do
is_expected.to have_css('.js-note-text', visible: true)
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
describe 'when previewing a note' do
diff --git a/spec/javascripts/notes/components/note_actions/reply_button_spec.js b/spec/javascripts/notes/components/note_actions/reply_button_spec.js
new file mode 100644
index 00000000000..11e1664a3f4
--- /dev/null
+++ b/spec/javascripts/notes/components/note_actions/reply_button_spec.js
@@ -0,0 +1,46 @@
+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);
+ });
+});
diff --git a/spec/javascripts/notes/components/note_actions_spec.js b/spec/javascripts/notes/components/note_actions_spec.js
index b102b7aecf7..0c1962912b4 100644
--- a/spec/javascripts/notes/components/note_actions_spec.js
+++ b/spec/javascripts/notes/components/note_actions_spec.js
@@ -2,14 +2,38 @@ import Vue from 'vue';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import createStore from '~/notes/stores';
import noteActions from '~/notes/components/note_actions.vue';
+import { TEST_HOST } from 'spec/test_constants';
import { userDataMock } from '../mock_data';
describe('noteActions', () => {
let wrapper;
let store;
+ let props;
+
+ const createWrapper = propsData => {
+ const localVue = createLocalVue();
+ return shallowMount(noteActions, {
+ store,
+ propsData,
+ localVue,
+ sync: false,
+ });
+ };
beforeEach(() => {
store = createStore();
+ props = {
+ accessLevel: 'Maintainer',
+ authorId: 26,
+ canDelete: true,
+ canEdit: true,
+ canAwardEmoji: true,
+ canReportAsAbuse: true,
+ noteId: '539',
+ noteUrl: `${TEST_HOST}/group/project/merge_requests/1#note_1`,
+ reportAbusePath: `${TEST_HOST}/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26`,
+ showReply: false,
+ };
});
afterEach(() => {
@@ -17,31 +41,10 @@ describe('noteActions', () => {
});
describe('user is logged in', () => {
- let props;
-
beforeEach(() => {
- props = {
- accessLevel: 'Maintainer',
- authorId: 26,
- canDelete: true,
- canEdit: true,
- canAwardEmoji: true,
- canReportAsAbuse: true,
- 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',
- };
-
store.dispatch('setUserData', userDataMock);
- const localVue = createLocalVue();
- wrapper = shallowMount(noteActions, {
- store,
- propsData: props,
- localVue,
- sync: false,
- });
+ wrapper = createWrapper(props);
});
it('should render access level badge', () => {
@@ -91,28 +94,14 @@ describe('noteActions', () => {
});
describe('user is not logged in', () => {
- let props;
-
beforeEach(() => {
store.dispatch('setUserData', {});
- props = {
- accessLevel: 'Maintainer',
- authorId: 26,
+ wrapper = createWrapper({
+ ...props,
canDelete: false,
canEdit: false,
canAwardEmoji: 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', () => {
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);
+ });
+ });
+ });
});
diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js
index 2e3cd5e8f36..73f960dd21e 100644
--- a/spec/javascripts/notes/stores/actions_spec.js
+++ b/spec/javascripts/notes/stores/actions_spec.js
@@ -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,
+ );
+ });
+ });
});
diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js
index b6b2c7d60a5..4f8d3069bb5 100644
--- a/spec/javascripts/notes/stores/mutation_spec.js
+++ b/spec/javascripts/notes/stores/mutation_spec.js
@@ -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();
+ });
+ });
});
diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
index e5420ea6bea..50e473c459e 100644
--- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
@@ -155,11 +155,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
it_behaves_like "checks permissions on noteable"
end
- context "when everything is fine" do
- before do
- setup_attachment
- end
-
+ shared_examples 'a reply to existing comment' do
it "creates a comment" do
expect { receiver.execute }.to change { noteable.notes.count }.by(1)
new_note = noteable.notes.last
@@ -168,7 +164,21 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
expect(new_note.position).to eq(note.position)
expect(new_note.note).to include("I could not disagree more.")
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
+
+ context "when everything is fine" do
+ before do
+ setup_attachment
+ end
+
+ it_behaves_like 'a reply to existing comment'
it "adds all attachments" do
receiver.execute
@@ -207,4 +217,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
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
diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb
index 677613b7980..6c35ed8f649 100644
--- a/spec/models/sent_notification_spec.rb
+++ b/spec/models/sent_notification_spec.rb
@@ -36,19 +36,41 @@ describe SentNotification do
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
let(:issue) { create(:issue) }
- it 'creates a new SentNotification' do
- expect { described_class.record(issue, user.id) }.to change { described_class.count }.by(1)
- end
+ subject { described_class.record(issue, user.id) }
+
+ it_behaves_like 'a successful sent notification'
end
describe '.record_note' do
- let(:note) { create(:diff_note_on_merge_request) }
+ subject { described_class.record_note(note, note.author.id) }
- it 'creates a new SentNotification' do
- expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1)
+ context 'for a discussion note' do
+ let(:note) { create(:diff_note_on_merge_request) }
+
+ it_behaves_like 'a successful sent notification'
+
+ 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
diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb
index 9aaccb4bffe..af4daff336b 100644
--- a/spec/services/notes/build_service_spec.rb
+++ b/spec/services/notes/build_service_spec.rb
@@ -123,6 +123,46 @@ describe Notes::BuildService do
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
new_note = described_class.new(project,
author,
diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb
index 1b9ba42cfd6..48f1d696ff6 100644
--- a/spec/services/notes/create_service_spec.rb
+++ b/spec/services/notes/create_service_spec.rb
@@ -278,5 +278,42 @@ describe Notes::CreateService do
expect(note.note).to eq(':smile:')
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