Merge branch '21505-quickactions-update-pd' into 'master'

Apply quickactions when modifying comments

See merge request gitlab-org/gitlab-ce!31136

app/assets/javascripts/lib/utils/http_status.js

@@ -19,6 +19,7 @@ const httpStatusCodes = {
   UNAUTHORIZED: 401,
   FORBIDDEN: 403,
   NOT_FOUND: 404,
+  GONE: 410,
   UNPROCESSABLE_ENTITY: 422,
 };
 

app/assets/javascripts/notes/components/noteable_note.vue

@@ -14,6 +14,7 @@ import NoteBody from './note_body.vue';
 import eventHub from '../event_hub';
 import noteable from '../mixins/noteable';
 import resolvable from '../mixins/resolvable';
+import httpStatusCodes from '~/lib/utils/http_status';
 
 export default {
   name: 'NoteableNote',
@@ -122,7 +123,13 @@ export default {
   },
 
   methods: {
-    ...mapActions(['deleteNote', 'updateNote', 'toggleResolveNote', 'scrollToNoteIfNeeded']),
+    ...mapActions([
+      'deleteNote',
+      'removeNote',
+      'updateNote',
+      'toggleResolveNote',
+      'scrollToNoteIfNeeded',
+    ]),
     editHandler() {
       this.isEditing = true;
       this.$emit('handleEdit');
@@ -185,15 +192,21 @@ export default {
           this.updateSuccess();
           callback();
         })
-        .catch(() => {
-          this.isRequesting = false;
-          this.isEditing = true;
-          this.$nextTick(() => {
-            const msg = __('Something went wrong while editing your comment. Please try again.');
-            Flash(msg, 'alert', this.$el);
-            this.recoverNoteContent(noteText);
+        .catch(response => {
+          if (response.status === httpStatusCodes.GONE) {
+            this.removeNote(this.note);
+            this.updateSuccess();
             callback();
-          });
+          } else {
+            this.isRequesting = false;
+            this.isEditing = true;
+            this.$nextTick(() => {
+              const msg = __('Something went wrong while editing your comment. Please try again.');
+              Flash(msg, 'alert', this.$el);
+              this.recoverNoteContent(noteText);
+              callback();
+            });
+          }
         });
     },
     formCancelHandler(shouldConfirm, isDirty) {

app/assets/javascripts/notes/stores/actions.js

@@ -61,18 +61,22 @@ export const updateDiscussion = ({ commit, state }, discussion) => {
   return utils.findNoteObjectById(state.discussions, discussion.id);
 };
 
-export const deleteNote = ({ commit, dispatch, state }, note) =>
+export const removeNote = ({ commit, dispatch, state }, note) => {
+  const discussion = state.discussions.find(({ id }) => id === note.discussion_id);
+
+  commit(types.DELETE_NOTE, note);
+
+  dispatch('updateMergeRequestWidget');
+  dispatch('updateResolvableDiscussionsCounts');
+
+  if (isInMRPage()) {
+    dispatch('diffs/removeDiscussionsFromDiff', discussion);
+  }
+};
+
+export const deleteNote = ({ dispatch }, note) =>
   axios.delete(note.path).then(() => {
-    const discussion = state.discussions.find(({ id }) => id === note.discussion_id);
-
-    commit(types.DELETE_NOTE, note);
-
-    dispatch('updateMergeRequestWidget');
-    dispatch('updateResolvableDiscussionsCounts');
-
-    if (isInMRPage()) {
-      dispatch('diffs/removeDiscussionsFromDiff', discussion);
-    }
+    dispatch('removeNote', note);
   });
 
 export const updateNote = ({ commit, dispatch }, { endpoint, note }) =>

app/controllers/concerns/notes_actions.rb

@@ -73,6 +73,11 @@ module NotesActions
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def update
     @note = Notes::UpdateService.new(project, current_user, update_note_params).execute(note)
+    unless @note
+      head :gone
+      return
+    end
+
     prepare_notes_for_rendering([@note])
 
     respond_to do |format|

app/controllers/projects/notes_controller.rb

@@ -6,7 +6,7 @@ class Projects::NotesController < Projects::ApplicationController
   include NotesHelper
   include ToggleAwardEmoji
 
-  before_action :whitelist_query_limiting, only: [:create]
+  before_action :whitelist_query_limiting, only: [:create, :update]
   before_action :authorize_read_note!
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_resolve_note!, only: [:resolve, :unresolve]

app/services/notes/update_service.rb

@@ -8,24 +8,70 @@ module Notes
       old_mentioned_users = note.mentioned_users.to_a
 
       note.update(params.merge(updated_by: current_user))
-      note.create_new_cross_references!(current_user)
 
-      if note.previous_changes.include?('note')
-        TodoService.new.update_note(note, current_user, old_mentioned_users)
+      only_commands = false
+
+      quick_actions_service = QuickActionsService.new(project, current_user)
+      if quick_actions_service.supported?(note)
+        content, update_params, message = quick_actions_service.execute(note, {})
+
+        only_commands = content.empty?
+
+        note.note = content
       end
 
-      if note.supports_suggestion?
-        Suggestion.transaction do
-          note.suggestions.delete_all
-          Suggestions::CreateService.new(note).execute
+      unless only_commands
+        note.create_new_cross_references!(current_user)
+
+        update_todos(note, old_mentioned_users)
+
+        update_suggestions(note)
+      end
+
+      if quick_actions_service.commands_executed_count.to_i > 0
+        if update_params.present?
+          quick_actions_service.apply_updates(update_params, note)
+          note.commands_changes = update_params
         end
 
-        # We need to refresh the previous suggestions call cache
-        # in order to get the new records.
-        note.reset
+        if only_commands
+          delete_note(note, message)
+          note = nil
+        else
+          note.save
+        end
       end
 
       note
     end
+
+    private
+
+    def delete_note(note, message)
+      # We must add the error after we call #save because errors are reset
+      # when #save is called
+      note.errors.add(:commands_only, message.presence || _('Commands did not apply'))
+
+      Notes::DestroyService.new(project, current_user).execute(note)
+    end
+
+    def update_suggestions(note)
+      return unless note.supports_suggestion?
+
+      Suggestion.transaction do
+        note.suggestions.delete_all
+        Suggestions::CreateService.new(note).execute
+      end
+
+      # We need to refresh the previous suggestions call cache
+      # in order to get the new records.
+      note.reset
+    end
+
+    def update_todos(note, old_mentioned_users)
+      return unless note.previous_changes.include?('note')
+
+      TodoService.new.update_note(note, current_user, old_mentioned_users)
+    end
   end
 end

changelogs/unreleased/21505-quickactions-update-pd.yml

@@ -0,0 +1,5 @@
+---
+title: Apply quickactions when modifying comments
+merge_request: 31136
+author:
+type: added

locale/gitlab.pot

@@ -2977,6 +2977,9 @@ msgstr ""
 msgid "Commands applied"
 msgstr ""
 
+msgid "Commands did not apply"
+msgstr ""
+
 msgid "Comment"
 msgstr ""
 

spec/javascripts/notes/stores/actions_spec.js

@@ -336,7 +336,7 @@ describe('Actions Notes Store', () => {
     });
   });
 
-  describe('deleteNote', () => {
+  describe('removeNote', () => {
     const endpoint = `${TEST_HOST}/note`;
     let axiosMock;
 
@@ -357,7 +357,7 @@ describe('Actions Notes Store', () => {
       const note = { path: endpoint, id: 1 };
 
       testAction(
-        actions.deleteNote,
+        actions.removeNote,
         note,
         store.state,
         [
@@ -384,7 +384,7 @@ describe('Actions Notes Store', () => {
       $('body').attr('data-page', 'projects:merge_requests:show');
 
       testAction(
-        actions.deleteNote,
+        actions.removeNote,
         note,
         store.state,
         [
@@ -409,6 +409,45 @@ describe('Actions Notes Store', () => {
     });
   });
 
+  describe('deleteNote', () => {
+    const endpoint = `${TEST_HOST}/note`;
+    let axiosMock;
+
+    beforeEach(() => {
+      axiosMock = new AxiosMockAdapter(axios);
+      axiosMock.onDelete(endpoint).replyOnce(200, {});
+
+      $('body').attr('data-page', '');
+    });
+
+    afterEach(() => {
+      axiosMock.restore();
+
+      $('body').attr('data-page', '');
+    });
+
+    it('dispatches removeNote', done => {
+      const note = { path: endpoint, id: 1 };
+
+      testAction(
+        actions.deleteNote,
+        note,
+        {},
+        [],
+        [
+          {
+            type: 'removeNote',
+            payload: {
+              id: 1,
+              path: 'http://test.host/note',
+            },
+          },
+        ],
+        done,
+      );
+    });
+  });
+
   describe('createNewNote', () => {
     describe('success', () => {
       const res = {

spec/support/shared_examples/quick_actions/issue/move_quick_action_shared_examples.rb

@@ -89,5 +89,54 @@ shared_examples 'move quick action' do
         it_behaves_like 'applies the commands to issues in both projects, target and source'
       end
     end
+
+    context 'when editing comments' do
+      let(:target_project) { create(:project, :public) }
+
+      before do
+        target_project.add_maintainer(user)
+
+        sign_in(user)
+        visit project_issue_path(project, issue)
+        wait_for_all_requests
+      end
+
+      it 'moves the issue after quickcommand note was updated' do
+        # misspelled quick action
+        add_note("test note.\n/mvoe #{target_project.full_path}")
+
+        expect(issue.reload).not_to be_closed
+
+        edit_note("/mvoe #{target_project.full_path}", "test note.\n/move #{target_project.full_path}")
+        wait_for_all_requests
+
+        expect(page).to have_content 'test note.'
+        expect(issue.reload).to be_closed
+
+        visit project_issue_path(target_project, issue)
+        wait_for_all_requests
+
+        expect(page).to have_content 'Issues 1'
+      end
+
+      it 'deletes the note if it was updated to just contain a command' do
+        # missspelled quick action
+        add_note("test note.\n/mvoe #{target_project.full_path}")
+
+        expect(page).not_to have_content 'Commands applied'
+        expect(issue.reload).not_to be_closed
+
+        edit_note("/mvoe #{target_project.full_path}", "/move #{target_project.full_path}")
+        wait_for_all_requests
+
+        expect(page).not_to have_content "/move #{target_project.full_path}"
+        expect(issue.reload).to be_closed
+
+        visit project_issue_path(target_project, issue)
+        wait_for_all_requests
+
+        expect(page).to have_content 'Issues 1'
+      end
+    end
   end
 end