Add reply to notes to turn into discussions

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

app/assets/javascripts/notes/components/note_actions/reply_button.vue

+<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>

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

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"

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

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

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';

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

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

app/models/concerns/noteable.rb

 # 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

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

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

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

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

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

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 ""

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

spec/javascripts/notes/components/note_actions/reply_button_spec.js

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

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

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