diff --git a/app/assets/javascripts/lib/utils/http_status.js b/app/assets/javascripts/lib/utils/http_status.js index 37ad1676f7a..5e5d10883a3 100644 --- a/app/assets/javascripts/lib/utils/http_status.js +++ b/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, }; diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index 2f201839d45..9019f0542b6 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/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) { diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index b7857997d42..411bd585672 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/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 }) => diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index d2a961efff7..4b7899d469b 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/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| diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 65d9b074eee..13e8453ed00 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/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] diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 384d1dd2e50..853faed9d85 100644 --- a/app/services/notes/update_service.rb +++ b/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 diff --git a/changelogs/unreleased/21505-quickactions-update-pd.yml b/changelogs/unreleased/21505-quickactions-update-pd.yml new file mode 100644 index 00000000000..243f8eda4e3 --- /dev/null +++ b/changelogs/unreleased/21505-quickactions-update-pd.yml @@ -0,0 +1,5 @@ +--- +title: Apply quickactions when modifying comments +merge_request: 31136 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 61642fbbd59..8c8574d0a48 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2977,6 +2977,9 @@ msgstr "" msgid "Commands applied" msgstr "" +msgid "Commands did not apply" +msgstr "" + msgid "Comment" msgstr "" diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index e55aa0e965a..1fd4a9a7612 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/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 = { diff --git a/spec/support/shared_examples/quick_actions/issue/move_quick_action_shared_examples.rb b/spec/support/shared_examples/quick_actions/issue/move_quick_action_shared_examples.rb index a37b2392d52..bebc8509d53 100644 --- a/spec/support/shared_examples/quick_actions/issue/move_quick_action_shared_examples.rb +++ b/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