From 1889ea9efb6b6b00b971fe08c39d9f9d28164c03 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 7 Nov 2024 23:16:57 -0800 Subject: [PATCH 1/4] Refactor notes quick action handling Previously whenever a quick action was processed in a note, `NotesController#create` and `NotesController#update` stored the message and commands inside ActiveModel error attributes, `commands_only` and `command_names`. The returned JSON looked something like: ``` { "commands_changes": {}, "valid": false, "errors": { "commands_only": [ "Commands applied" ], "command_names": ["/close"] } } ``` This is quite confusing and a bit of hack because the `errors` section suggests there was an actual error, but in the example above the quick action succeeded. The frontend distinguishes between a failed and successful attempt by looking at the HTTP code (422 vs. 200). To determine which code to set, previously the controller looked at all the attributes stored on the `Note` ActiveModel record: * If only `commands_only` and `command_names` were present, consider the operation a success and return 200. * If there are other error attributes (e.g. `validation`), then the operation is deemed a failure and return 422. In the failure case, the controller previously only sent whatever message was stored in the `commands_only` attribute. However, if a quick action fails, we may want to display the details as to why the failure occurred. To do that, we should stop using the ActiveModel errors as a placeholder for information and only use it when actual failures occur. This message refactors the quick action handling between the frontend and backend. Now a JSON return payload looks more like: ``` { "commands_changes": {}, "valid": false, "quick_actions_status": { "messages": [ "Commands applied." ], "command_names": ["/close"], "commands_only": true, "error": false } } ``` Changelog: changed --- app/assets/javascripts/deprecated_notes.js | 4 +- .../notes/store/legacy_notes/actions.js | 9 +-- .../javascripts/notes/stores/actions.js | 10 +-- app/assets/javascripts/notes/utils.js | 10 +-- app/controllers/concerns/notes_actions.rb | 18 ++--- app/models/note.rb | 4 +- app/services/notes/create_service.rb | 19 ++--- app/services/notes/quick_actions_status.rb | 41 +++++++++++ app/services/notes/update_service.rb | 14 ++-- lib/api/entities/note.rb | 2 +- lib/api/notes.rb | 6 +- lib/gitlab/email/handler/reply_processing.rb | 2 +- .../projects/notes_controller_spec.rb | 69 ++++++++++++++++--- .../notes/components/comment_form_spec.js | 7 +- .../notes/store/legacy_notes/actions_spec.js | 5 +- spec/frontend/notes/stores/actions_spec.js | 5 +- spec/frontend/notes/utils_spec.js | 6 +- spec/requests/api/notes_spec.rb | 40 +++++++++++ spec/services/notes/create_service_spec.rb | 13 ++-- spec/services/notes/update_service_spec.rb | 18 ++++- 20 files changed, 231 insertions(+), 71 deletions(-) create mode 100644 app/services/notes/quick_actions_status.rb diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 19d4b363bf5feb..d3886da1133b1b 100644 --- a/app/assets/javascripts/deprecated_notes.js +++ b/app/assets/javascripts/deprecated_notes.js @@ -429,12 +429,12 @@ export default class Notes { } if (!noteEntity.valid) { - if (noteEntity.errors && noteEntity.errors.commands_only) { + if (noteEntity?.quick_actions_status?.messages) { if (noteEntity.commands_changes && Object.keys(noteEntity.commands_changes).length > 0) { $notesList.find('.system-note.being-posted').remove(); } this.addAlert({ - message: noteEntity.errors.commands_only, + message: noteEntity.quick_actions_status.messages, variant: VARIANT_INFO, parent: this.parentTimeline.get(0), }); diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index c75bda99ca2809..8fa35d6496cbf3 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -510,13 +510,8 @@ export function saveNote(noteData) { const processQuickActions = (res) => { const { - errors: { commands_only: commandsOnly } = { - commands_only: null, - command_names: [], - }, - command_names: commandNames, + quick_actions_status: { messages: message = null, command_names: commandNames = [] } = {}, } = res; - const message = commandsOnly; if (commandNames?.indexOf('submit_review') >= 0) { useBatchComments().clearDrafts(); @@ -525,7 +520,7 @@ export function saveNote(noteData) { /* The following reply means that quick actions have been successfully applied: - {"commands_changes":{},"valid":false,"errors":{"commands_only":["Commands applied"]}} + {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true,"error":false}} */ if (hasQuickActions && message) { // synchronizing the quick action with the sidebar widget diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index c3261257dddddb..96af4cce697e99 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -493,14 +493,8 @@ export const saveNote = ({ commit, dispatch }, noteData) => { const processQuickActions = (res) => { const { - errors: { commands_only: commandsOnly } = { - commands_only: null, - command_names: [], - }, - command_names: commandNames, + quick_actions_status: { messages: message = null, command_names: commandNames = [] } = {}, } = res; - const message = commandsOnly; - if (commandNames?.indexOf('submit_review') >= 0) { dispatch('batchComments/clearDrafts'); } @@ -508,7 +502,7 @@ export const saveNote = ({ commit, dispatch }, noteData) => { /* The following reply means that quick actions have been successfully applied: - {"commands_changes":{},"valid":false,"errors":{"commands_only":["Commands applied"]}} + {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true,"error":false}} */ if (hasQuickActions && message) { // synchronizing the quick action with the sidebar widget diff --git a/app/assets/javascripts/notes/utils.js b/app/assets/javascripts/notes/utils.js index 4ccf7cae5d47ee..0790f01ac433c2 100644 --- a/app/assets/javascripts/notes/utils.js +++ b/app/assets/javascripts/notes/utils.js @@ -26,12 +26,14 @@ export const renderMarkdown = (rawMarkdown) => { export const createNoteErrorMessages = (data, status) => { const errors = data?.errors; - if (errors && status === HTTP_STATUS_UNPROCESSABLE_ENTITY) { - if (errors.commands_only?.length) { - return errors.commands_only; + if (status === HTTP_STATUS_UNPROCESSABLE_ENTITY) { + if (data.quick_actions_status?.messages?.length) { + return data.quick_actions_status.messages; } - return [sprintf(COMMENT_FORM.error, { reason: errors.toLowerCase() }, false)]; + if (errors) { + return [sprintf(COMMENT_FORM.error, { reason: errors.toLowerCase() }, false)]; + } } return [COMMENT_FORM.GENERIC_UNSUBMITTABLE_NETWORK]; diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 0a0e614717b3f9..22a0a3886d8696 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -43,8 +43,7 @@ def create respond_to do |format| format.json do json = { - commands_changes: @note.commands_changes&.slice(:emoji_award, :time_estimate, :spend_time), - command_names: @note.command_names + commands_changes: @note.commands_changes&.slice(:emoji_award, :time_estimate, :spend_time) } if @note.persisted? && return_discussion? @@ -59,8 +58,13 @@ def create json.merge!(note_json(@note)) end - if @note.errors.present? && @note.errors.attribute_names != [:commands_only, :command_names] - render json: { errors: errors_on_create(@note.errors) }, status: :unprocessable_entity + quick_actions = @note.quick_actions_status + json[:quick_actions_status] = quick_actions.to_h if quick_actions + + if @note.errors.present? + render json: { errors: errors_on_create(@note) }, status: :unprocessable_entity + elsif quick_actions&.error? + render json: { quick_actions_status: quick_actions.to_h }, status: :unprocessable_entity else render json: json end @@ -321,10 +325,8 @@ def use_note_serializer? noteable.discussions_rendered_on_frontend? end - def errors_on_create(errors) - return { commands_only: errors.messages[:commands_only] } if errors.key?(:commands_only) - - errors.full_messages.to_sentence + def errors_on_create(note) + note.errors.full_messages.to_sentence end end diff --git a/app/models/note.rb b/app/models/note.rb index 91659615f76c69..9b7fe1a4da1855 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -56,8 +56,8 @@ class Note < ApplicationRecord # Attribute used to store the attributes that have been changed by quick actions. attr_writer :commands_changes - # Attribute used to store the quick action command names. - attr_accessor :command_names + # Attribute used to store the status of quick actions. + attr_accessor :quick_actions_status # Attribute used to determine whether keep_around_commits will be skipped for diff notes. attr_accessor :skip_keep_around_commits diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index ee4a600bde3604..9dc543e78494c2 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -78,7 +78,6 @@ def execute_quick_actions(note) content, update_params, message, command_names = quick_actions_service.execute(note, quick_action_options) only_commands = content.empty? note.note = content - note.command_names = command_names yield(only_commands) @@ -123,21 +122,23 @@ def when_saved( end def do_commands(note, update_params, message, command_names, only_commands) + status = ::Notes::QuickActionsStatus.new( + message: message, + command_names: command_names&.flatten, + commands_only: only_commands) + note.quick_actions_status = status + return if quick_actions_service.commands_executed_count.to_i == 0 update_error = quick_actions_update_errors(note, update_params) if update_error note.errors.add(:validation, update_error) - message = update_error + status.message = update_error end - # We must add the error after we call #save because errors are reset - # when #save is called - if only_commands - note.errors.add(:commands_only, message.presence || _('Failed to apply commands.')) - note.errors.add(:command_names, command_names.flatten) - # Allow consumers to detect problems applying commands - note.errors.add(:commands, _('Failed to apply commands.')) unless message.present? + if only_commands && message.blank? + status.error = true + status.message = _('Failed to apply commands.') end end diff --git a/app/services/notes/quick_actions_status.rb b/app/services/notes/quick_actions_status.rb new file mode 100644 index 00000000000000..566c389ca8cd53 --- /dev/null +++ b/app/services/notes/quick_actions_status.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Notes + class QuickActionsStatus + attr_accessor :message, :commands_only, :command_names, :error + + def initialize(message:, command_names:, commands_only:, error: false) + @message = message + @command_names = command_names + @commands_only = commands_only + @error = error + end + + def commands_only? + commands_only + end + + def success? + !error + end + + def error? + error + end + + def to_h + { + messages: messages, + command_names: command_names, + commands_only: commands_only, + error: error + } + end + + def messages + return unless message.presence + + [message] + end + end +end diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 4916a5f2691ea6..c9484464159293 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -24,11 +24,14 @@ def execute(note) quick_actions_service = QuickActionsService.new(project, current_user) if quick_actions_service.supported?(note) - content, update_params, message = quick_actions_service.execute(note, {}) + content, update_params, message, command_names = quick_actions_service.execute(note, {}) only_commands = content.empty? note.note = content + status = ::Notes::QuickActionsStatus.new( + message: message, command_names: command_names, commands_only: only_commands) + note.quick_actions_status = status end update_note(note, only_commands) @@ -71,11 +74,10 @@ def update_note(note, only_commands) end 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')) - # Allow consumers to detect problems applying commands - note.errors.add(:commands, _('Commands did not apply')) unless message.present? + if message.blank? + note.quick_actions_status.error = true + note.quick_actions_status.message = _('Commands did not apply') + end Notes::DestroyService.new(project, current_user).execute(note) end diff --git a/lib/api/entities/note.rb b/lib/api/entities/note.rb index d40358185722bb..0610e66dc950eb 100644 --- a/lib/api/entities/note.rb +++ b/lib/api/entities/note.rb @@ -44,7 +44,7 @@ class Note < Grape::Entity # To be returned if the note was command-only class NoteCommands < Grape::Entity expose(:commands_changes) { |note| note.commands_changes || {} } - expose(:summary) { |note| note.errors[:commands_only] } + expose(:summary) { |note| [note.quick_actions_status&.message] } end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 9596a4adea3d1c..12127477d6efba 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -99,14 +99,14 @@ class Notes < ::API::Base } note = create_note(noteable, opts) + quick_action_status = note.quick_actions_status - if note.errors.attribute_names == [:commands_only, :command_names] + if quick_action_status&.commands_only? && quick_action_status.success? status 202 present note, with: Entities::NoteCommands elsif note.persisted? present note, with: Entities.const_get(note.class.name, false) - else - note.errors.delete(:commands_only) if note.errors.has_key?(:commands) + elsif note.errors.present? bad_request!("Note #{note.errors.messages}") end end diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index 6404a3a54f984b..9fe10fe7294018 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -84,7 +84,7 @@ def validate_permission!(permission) def verify_record!(record:, invalid_exception:, record_name:) return if record.persisted? - return if record.errors.key?(:commands_only) + return if record.is_a?(Note) && record.quick_actions_status&.commands_only? error_title = "The #{record_name} could not be created for the following reasons:" diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 2f848125b10008..9c5a9634f281b0 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -551,7 +551,48 @@ def create! create! expect(response).to have_gitlab_http_status(:ok) - expect(json_response['command_names']).to include('react', 'estimate', 'spend') + expect(json_response['quick_actions_status']['command_names']).to include('react', 'estimate', 'spend') + expect(json_response['quick_actions_status']['commands_only']).to be(true) + end + end + + context 'with a mix of text and an invalid command' do + let(:note_text) { "hello world\n/spend asdf" } + let(:extra_request_params) { { format: :json } } + let(:expected) do + { + "messages" => nil, + "command_names" => %w[spend], + "commands_only" => false, + "error" => false + } + end + + it 'returns expected status' do + create! + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['quick_actions_status']).to eq(expected) + end + end + + context 'with a mix of text and commands that return changes and errors' do + let(:note_text) { "hello world\n/estimate 1d\n/spend asdf" } + let(:extra_request_params) { { format: :json } } + let(:expected) do + { + "messages" => ["Set time estimate to 1d."], + "command_names" => %w[estimate spend], + "commands_only" => false, + "error" => false + } + end + + it 'returns expected status' do + create! + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['quick_actions_status']).to eq(expected) end end @@ -576,20 +617,30 @@ def create! create! expect(response).to have_gitlab_http_status(:ok) - expect(json_response['command_names']).to include('move', 'title') + expect(json_response['quick_actions_status']['command_names']).to include('move', 'title') + expect(json_response['quick_actions_status']['commands_only']).to be(true) end end context 'with commands that return an error' do let(:extra_request_params) { { format: :json } } + let(:expected) do + { + 'quick_actions_status' => { + "messages" => ["Failed to apply commands."], + "command_names" => ["label"], + "commands_only" => true, + "error" => true + } + } + end before do - errors = ActiveModel::Errors.new(note) - errors.add(:commands_only, 'Failed to apply commands.') - errors.add(:command_names, ['label']) - errors.add(:commands, 'Failed to apply commands.') - - allow(note).to receive(:errors).and_return(errors) + note.quick_actions_status = ::Notes::QuickActionsStatus.new( + message: 'Failed to apply commands.', + command_names: ['label'], + commands_only: true, + error: true) allow_next_instance_of(Notes::CreateService) do |service| allow(service).to receive(:execute).and_return(note) @@ -600,7 +651,7 @@ def create! create! expect(response).to have_gitlab_http_status(:unprocessable_entity) - expect(response.body).to eq('{"errors":{"commands_only":["Failed to apply commands."]}}') + expect(response.parsed_body).to eq(expected) end end end diff --git a/spec/frontend/notes/components/comment_form_spec.js b/spec/frontend/notes/components/comment_form_spec.js index 77ac1f8a39f75a..b4a52bddf6ad48 100644 --- a/spec/frontend/notes/components/comment_form_spec.js +++ b/spec/frontend/notes/components/comment_form_spec.js @@ -196,7 +196,10 @@ describe('issue_comment_form component', () => { const store = createStore({ actions: { saveNote: jest.fn().mockRejectedValue({ - response: { status: httpStatus, data: { errors: { commands_only: errors } } }, + response: { + status: httpStatus, + data: { quick_actions_status: { messages: errors } }, + }, }), }, }); @@ -258,7 +261,7 @@ describe('issue_comment_form component', () => { saveNote: jest.fn().mockRejectedValue({ response: { status: HTTP_STATUS_UNPROCESSABLE_ENTITY, - data: { errors: { commands_only: [...commandErrors] } }, + data: { quick_actions_status: { messages: [...commandErrors] } }, }, }), }, diff --git a/spec/frontend/notes/store/legacy_notes/actions_spec.js b/spec/frontend/notes/store/legacy_notes/actions_spec.js index 3553169f8a2103..f07fc3a530b5df 100644 --- a/spec/frontend/notes/store/legacy_notes/actions_spec.js +++ b/spec/frontend/notes/store/legacy_notes/actions_spec.js @@ -853,7 +853,10 @@ describe('Actions Notes Store', () => { it('dispatches clearDrafts is command names contains submit_review', async () => { const spy = jest.spyOn(useBatchComments(), 'clearDrafts'); - const response = { command_names: ['submit_review'], valid: true }; + const response = { + quick_actions_status: { command_names: ['submit_review'] }, + valid: true, + }; axiosMock.onAny().reply(HTTP_STATUS_OK, response); await store.saveNote(payload); diff --git a/spec/frontend/notes/stores/actions_spec.js b/spec/frontend/notes/stores/actions_spec.js index 59ca7eea60be0a..40b62100904725 100644 --- a/spec/frontend/notes/stores/actions_spec.js +++ b/spec/frontend/notes/stores/actions_spec.js @@ -821,7 +821,10 @@ describe('Actions Notes Store', () => { }); it('dispatches clearDrafts is command names contains submit_review', async () => { - const response = { command_names: ['submit_review'], valid: true }; + const response = { + quick_actions_status: { command_names: ['submit_review'] }, + valid: true, + }; dispatch = jest.fn().mockResolvedValue(response); await actions.saveNote( { diff --git a/spec/frontend/notes/utils_spec.js b/spec/frontend/notes/utils_spec.js index 3607c3c546c99c..1a7bd97e7e6694 100644 --- a/spec/frontend/notes/utils_spec.js +++ b/spec/frontend/notes/utils_spec.js @@ -32,9 +32,13 @@ describe('createNoteErrorMessages', () => { const errorMessages = createNoteErrorMessages( { errors: { - commands_only: ['commands_only error 1', 'commands_only error 2'], base: ['base error 1'], }, + quick_actions_status: { + error: true, + commands_only: true, + messages: ['commands_only error 1', 'commands_only error 2'], + }, }, HTTP_STATUS_UNPROCESSABLE_ENTITY, ); diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 50c63a70a33a20..cadfe9e6c70578 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -312,6 +312,46 @@ subject { post api(request_path, user), params: params } + context 'a note with both text and invalid command' do + let(:request_body) { "hello world\n/spend hello" } + + before do + project.add_developer(user) + end + + it 'returns 200 status' do + subject + + expect(response).to have_gitlab_http_status(:created) + end + + it 'creates a new note' do + expect { subject }.to change { Note.where(system: false).count }.by(1) + end + + it 'does not create a system note about the change', :sidekiq_inline do + expect { subject }.not_to change { Note.system.count } + end + + it 'does not apply the commands' do + expect { subject }.not_to change { merge_request.reset.total_time_spent } + end + end + + context 'a blank note' do + let(:request_body) { "" } + + before do + project.add_developer(user) + end + + it 'returns a 400 and does not create a note' do + expect { subject }.not_to change { Note.where(system: false).count } + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + context 'a command only note' do context '/spend' do let(:request_body) { "/spend 1h" } diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index c0393d1e434dc0..5860b25fc78ec1 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -513,14 +513,14 @@ note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.errors[:commands_only]).to be_present + expect(note.quick_actions_status.message).to be_present end it 'adds commands failed message to note errors' do note_text = %(/reopen) note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.errors[:commands_only]).to contain_exactly('Could not apply reopen command.') + expect(note.quick_actions_status.message).to eq('Could not apply reopen command.') end it 'generates success and failed error messages' do @@ -531,7 +531,9 @@ note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.errors[:commands_only]).to contain_exactly('Closed this issue. Could not apply reopen command.') + expect(note.quick_actions_status.error?).to be(false) + expect(note.quick_actions_status.command_names).to eq(%w[close reopen]) + expect(note.quick_actions_status.message).to eq('Closed this issue. Could not apply reopen command.') end it 'does not check for spam' do @@ -553,7 +555,10 @@ end note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.errors[:commands_only]).to contain_exactly('Confidential an error occurred') + + expect(note.quick_actions_status.error?).to be(false) + expect(note.quick_actions_status.command_names).to eq(['confidential']) + expect(note.quick_actions_status.message).to eq('Confidential an error occurred') end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index abb64b3a62e2b7..4d5d8568031ca9 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -47,6 +47,18 @@ def update_note(opts) end end + context 'when the note is an invalid command' do + let(:edit_note_text) { { note: '/spend asdf' } } + + it 'deletes the note and reports command errors' do + updated_note = described_class.new(project, user, edit_note_text).execute(note) + + expect(updated_note.destroyed?).to eq(true) + expect(updated_note.quick_actions_status.error).to be(true) + expect(updated_note.quick_actions_status.message).to eq('Commands did not apply') + end + end + context 'when the note is invalid' do let(:edit_note_text) { { note: 'new text' } } @@ -130,9 +142,11 @@ def update_note(opts) expect(updated_note.destroyed?).to eq(true) expect(updated_note.errors).to match_array([ - "Note can't be blank", - "Commands only Closed this issue." + "Note can't be blank" ]) + expect(updated_note.quick_actions_status.error?).to be(false) + expect(updated_note.quick_actions_status.command_names).to eq(['close']) + expect(updated_note.quick_actions_status.message).to eq('Closed this issue.') end end -- GitLab From 478e89b42ea402e37f253f6df5c472d8ab67a247 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 12 Nov 2024 14:53:38 -0800 Subject: [PATCH 2/4] Add quickActionsStatus field to create/update note GraphQL mutations Previously the `errors` field held the results of the quick action, but this made legitimate messages appear to fail. Now `errors` is only populated if the quick action failed, and details are in the `quickActionsStatus` field. Note that some `errors` will still be blank when calling actions such as `/unapprove` twice in a row. That's because the quick action service doesn't not yet have a way to distinguish between some failures. Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/346557 Changelog: fixed --- app/graphql/mutations/notes/base.rb | 6 +++++ app/graphql/mutations/notes/create/base.rb | 6 ++++- app/graphql/mutations/notes/update/base.rb | 3 ++- .../types/notes/quick_actions_status_type.rb | 27 +++++++++++++++++++ doc/api/graphql/reference/index.md | 20 ++++++++++++++ .../mutations/notes/create/note_spec.rb | 12 ++++++--- ..._actions_for_work_items_shared_examples.rb | 5 ++-- 7 files changed, 71 insertions(+), 8 deletions(-) create mode 100644 app/graphql/types/notes/quick_actions_status_type.rb diff --git a/app/graphql/mutations/notes/base.rb b/app/graphql/mutations/notes/base.rb index 58c075cd77129a..9d5f4f0efc2349 100644 --- a/app/graphql/mutations/notes/base.rb +++ b/app/graphql/mutations/notes/base.rb @@ -13,6 +13,12 @@ class Base < BaseMutation Types::Notes::NoteType, null: true, description: 'Note after mutation.' + + field :quick_actions_status, + Types::Notes::QuickActionsStatusType, + null: true, + description: 'Status of quick actions after mutation.', + skip_type_authorization: [:read_note] end end end diff --git a/app/graphql/mutations/notes/create/base.rb b/app/graphql/mutations/notes/create/base.rb index 22cccbce91a600..636129f8dc3a95 100644 --- a/app/graphql/mutations/notes/create/base.rb +++ b/app/graphql/mutations/notes/create/base.rb @@ -33,10 +33,14 @@ def resolve(args) create_note_params(noteable, args) ).execute - { + data = { note: (note if note.persisted?), errors: errors_on_object(note) } + + data.tap do |payload| + payload[:quick_actions_status] = note.quick_actions_status.to_h if note.quick_actions_status + end end private diff --git a/app/graphql/mutations/notes/update/base.rb b/app/graphql/mutations/notes/update/base.rb index 5edb4fb023be12..16a16f43f39d25 100644 --- a/app/graphql/mutations/notes/update/base.rb +++ b/app/graphql/mutations/notes/update/base.rb @@ -26,7 +26,8 @@ def resolve(args) { note: updated_note.destroyed? ? nil : updated_note.reset, - errors: updated_note.destroyed? ? [] : errors_on_object(updated_note) + errors: updated_note.destroyed? ? [] : errors_on_object(updated_note), + quick_actions_status: updated_note.destroyed? ? nil : updated_note.quick_actions_status&.to_h } end diff --git a/app/graphql/types/notes/quick_actions_status_type.rb b/app/graphql/types/notes/quick_actions_status_type.rb new file mode 100644 index 00000000000000..046cd517c80d7b --- /dev/null +++ b/app/graphql/types/notes/quick_actions_status_type.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Types + module Notes + class QuickActionsStatusType < BaseObject + graphql_name 'QuickActionsStatus' + + authorize :read_note + + field :messages, [GraphQL::Types::String], + null: true, + description: 'Quick action response messages.' + + field :command_names, [GraphQL::Types::String], + null: true, + description: 'Quick action command names.' + + field :commands_only, GraphQL::Types::Boolean, + null: true, + description: 'Returns true if only quick action commands were in the note.' + + field :error, GraphQL::Types::Boolean, + null: true, + description: 'Error in processing quick actions.' + end + end +end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index b4b3ad7bb14acf..4092e364eacc45 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -3686,6 +3686,7 @@ Input type: `CreateDiffNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.createDiscussion` @@ -3707,6 +3708,7 @@ Input type: `CreateDiscussionInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.createEpic` @@ -3763,6 +3765,7 @@ Input type: `CreateImageDiffNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.createIssue` @@ -3861,6 +3864,7 @@ Input type: `CreateNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.createPackagesProtectionRule` @@ -4771,6 +4775,7 @@ Input type: `DestroyNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.destroyPackage` @@ -7635,6 +7640,7 @@ Input type: `NoteConvertToThreadInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.oncallRotationCreate` @@ -8944,6 +8950,7 @@ Input type: `RepositionImageDiffNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.restorePagesDeployment` @@ -10186,6 +10193,7 @@ Input type: `UpdateImageDiffNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.updateIssue` @@ -10307,6 +10315,7 @@ Input type: `UpdateNoteInput` | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | `note` | [`Note`](#note) | Note after mutation. | +| `quickActionsStatus` | [`QuickActionsStatus`](#quickactionsstatus) | Status of quick actions after mutation. | ### `Mutation.updatePackagesCleanupPolicy` @@ -33442,6 +33451,17 @@ The amount of time for a job to be picked up by a runner, in percentiles. | `p99` | [`Duration`](#duration) | 99th percentile. 99% of the durations are lower than this value. | | `time` | [`Time!`](#time) | Start of the time interval. | +### `QuickActionsStatus` + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| `commandNames` | [`[String!]`](#string) | Quick action command names. | +| `commandsOnly` | [`Boolean`](#boolean) | Returns true if only quick action commands were in the note. | +| `error` | [`Boolean`](#boolean) | Error in processing quick actions. | +| `messages` | [`[String!]`](#string) | Quick action response messages. | + ### `RecentFailures` Recent failure history of a test case. diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb index dd3763fc180fa2..b6c451f446a2d4 100644 --- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb +++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb @@ -132,14 +132,18 @@ def mutation_response let(:head_sha) { noteable.diff_head_sha } let(:body) { '/merge' } - # NOTE: Known issue https://gitlab.com/gitlab-org/gitlab/-/issues/346557 it 'returns a nil note and info about the command in errors' do post_graphql_mutation(mutation, current_user: current_user) expect(mutation_response).to include( - 'errors' => include(/Merged this merge request/), - 'note' => nil - ) + 'errors' => [], + 'note' => nil, + 'quickActionsStatus' => { + "commandNames" => ["merge"], + "commandsOnly" => true, + "error" => false, + "messages" => ["Merged this merge request."] + }) end it 'starts the merge process' do diff --git a/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb b/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb index 0e0aff02dd3d60..c77359f496b68a 100644 --- a/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb +++ b/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb @@ -208,8 +208,9 @@ .and change { noteable.assignees }.to([]) expect(response).to have_gitlab_http_status(:success) - expect(mutation_response['errors']) - .to include("Commands only Type changed successfully. Assigned @#{assignee.username}.") + expect(mutation_response['errors']).to eq([]) + expect(mutation_response['quickActionsStatus']['messages']) + .to include("Type changed successfully. Assigned @#{assignee.username}.") end end -- GitLab From d679cd7876dc29a48816455ffdc0235102bcc8dc Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 15 Nov 2024 10:03:20 -0800 Subject: [PATCH 3/4] Move QuickActionsStatus into app/models/notes This better reflects its use. --- app/{services => models}/notes/quick_actions_status.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename app/{services => models}/notes/quick_actions_status.rb (100%) diff --git a/app/services/notes/quick_actions_status.rb b/app/models/notes/quick_actions_status.rb similarity index 100% rename from app/services/notes/quick_actions_status.rb rename to app/models/notes/quick_actions_status.rb -- GitLab From b0ba2fbb30f8d9c02932a640a4c8370c2c9bf449 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 15 Nov 2024 22:26:17 -0800 Subject: [PATCH 4/4] Separate quick action errors and messages Previously both errors and messages were combined into one stream, but for the GraphQL API it's better to separate the two. --- .../notes/store/legacy_notes/actions.js | 13 ++- .../javascripts/notes/stores/actions.js | 13 ++- app/assets/javascripts/notes/utils.js | 4 +- .../types/notes/quick_actions_status_type.rb | 6 +- app/models/notes/quick_actions_status.rb | 34 +++--- app/services/notes/create_service.rb | 10 +- app/services/notes/update_service.rb | 8 +- doc/api/graphql/reference/index.md | 4 +- lib/api/entities/note.rb | 2 +- lib/api/notes.rb | 2 + .../projects/notes_controller_spec.rb | 15 ++- .../notes/components/comment_form_spec.js | 4 +- spec/frontend/notes/utils_spec.js | 3 +- .../models/notes/quick_actions_status_spec.rb | 102 ++++++++++++++++++ .../mutations/notes/create/note_spec.rb | 4 +- spec/requests/api/notes_spec.rb | 24 +++++ spec/services/notes/create_service_spec.rb | 13 ++- spec/services/notes/update_service_spec.rb | 6 +- ..._actions_for_work_items_shared_examples.rb | 1 + 19 files changed, 197 insertions(+), 71 deletions(-) create mode 100644 spec/models/notes/quick_actions_status_spec.rb diff --git a/app/assets/javascripts/notes/store/legacy_notes/actions.js b/app/assets/javascripts/notes/store/legacy_notes/actions.js index 8fa35d6496cbf3..175fd9b801db88 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -509,9 +509,8 @@ export function saveNote(noteData) { } const processQuickActions = (res) => { - const { - quick_actions_status: { messages: message = null, command_names: commandNames = [] } = {}, - } = res; + const { quick_actions_status: { messages = null, command_names: commandNames = [] } = {} } = + res; if (commandNames?.indexOf('submit_review') >= 0) { useBatchComments().clearDrafts(); @@ -520,14 +519,14 @@ export function saveNote(noteData) { /* The following reply means that quick actions have been successfully applied: - {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true,"error":false}} + {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true}} */ - if (hasQuickActions && message) { + if (hasQuickActions && messages) { // synchronizing the quick action with the sidebar widget // this is a temporary solution until we have confidentiality real-time updates if ( confidentialWidget.setConfidentiality && - message.some((m) => m.includes('Made this issue confidential')) + messages.some((m) => m.includes('Made this issue confidential')) ) { confidentialWidget.setConfidentiality(); } @@ -535,7 +534,7 @@ export function saveNote(noteData) { $('.js-gfm-input').trigger('clear-commands-cache.atwho'); createAlert({ - message: message || __('Commands applied'), + message: messages || __('Commands applied'), variant: VARIANT_INFO, parent: noteData.flashContainer, }); diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 96af4cce697e99..2423a5d418e0a0 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -492,9 +492,8 @@ export const saveNote = ({ commit, dispatch }, noteData) => { } const processQuickActions = (res) => { - const { - quick_actions_status: { messages: message = null, command_names: commandNames = [] } = {}, - } = res; + const { quick_actions_status: { messages = null, command_names: commandNames = [] } = {} } = + res; if (commandNames?.indexOf('submit_review') >= 0) { dispatch('batchComments/clearDrafts'); } @@ -502,14 +501,14 @@ export const saveNote = ({ commit, dispatch }, noteData) => { /* The following reply means that quick actions have been successfully applied: - {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true,"error":false}} + {"commands_changes":{},"valid":false,"errors":{},"quick_actions_status":{"messages":["Commands applied"],"command_names":["due"],"commands_only":true}} */ - if (hasQuickActions && message) { + if (hasQuickActions && messages) { // synchronizing the quick action with the sidebar widget // this is a temporary solution until we have confidentiality real-time updates if ( confidentialWidget.setConfidentiality && - message.some((m) => m.includes('Made this issue confidential')) + messages.some((m) => m.includes('Made this issue confidential')) ) { confidentialWidget.setConfidentiality(); } @@ -517,7 +516,7 @@ export const saveNote = ({ commit, dispatch }, noteData) => { $('.js-gfm-input').trigger('clear-commands-cache.atwho'); createAlert({ - message: message || __('Commands applied'), + message: messages || __('Commands applied'), variant: VARIANT_INFO, parent: noteData.flashContainer, }); diff --git a/app/assets/javascripts/notes/utils.js b/app/assets/javascripts/notes/utils.js index 0790f01ac433c2..0d818433ea34b2 100644 --- a/app/assets/javascripts/notes/utils.js +++ b/app/assets/javascripts/notes/utils.js @@ -27,8 +27,8 @@ export const createNoteErrorMessages = (data, status) => { const errors = data?.errors; if (status === HTTP_STATUS_UNPROCESSABLE_ENTITY) { - if (data.quick_actions_status?.messages?.length) { - return data.quick_actions_status.messages; + if (data.quick_actions_status?.error_messages?.length) { + return data.quick_actions_status.error_messages; } if (errors) { diff --git a/app/graphql/types/notes/quick_actions_status_type.rb b/app/graphql/types/notes/quick_actions_status_type.rb index 046cd517c80d7b..a7db400c462543 100644 --- a/app/graphql/types/notes/quick_actions_status_type.rb +++ b/app/graphql/types/notes/quick_actions_status_type.rb @@ -9,7 +9,7 @@ class QuickActionsStatusType < BaseObject field :messages, [GraphQL::Types::String], null: true, - description: 'Quick action response messages.' + description: 'Response messages from quick actions.' field :command_names, [GraphQL::Types::String], null: true, @@ -19,9 +19,9 @@ class QuickActionsStatusType < BaseObject null: true, description: 'Returns true if only quick action commands were in the note.' - field :error, GraphQL::Types::Boolean, + field :error_messages, [GraphQL::Types::String], null: true, - description: 'Error in processing quick actions.' + description: 'Error messages from quick actions that failed to apply.' end end end diff --git a/app/models/notes/quick_actions_status.rb b/app/models/notes/quick_actions_status.rb index 566c389ca8cd53..d55a8656826f54 100644 --- a/app/models/notes/quick_actions_status.rb +++ b/app/models/notes/quick_actions_status.rb @@ -2,13 +2,21 @@ module Notes class QuickActionsStatus - attr_accessor :message, :commands_only, :command_names, :error + attr_accessor :messages, :error_messages, :commands_only, :command_names - def initialize(message:, command_names:, commands_only:, error: false) - @message = message + def initialize(command_names:, commands_only:) @command_names = command_names @commands_only = commands_only - @error = error + @messages = [] + @error_messages = [] + end + + def add_message(message) + @messages.append(message) unless message.blank? + end + + def add_error(message) + @error_messages.append(message) end def commands_only? @@ -16,26 +24,22 @@ def commands_only? end def success? - !error + !error? end def error? - error + error_messages.any? end def to_h - { - messages: messages, + payload = { command_names: command_names, - commands_only: commands_only, - error: error + commands_only: commands_only } - end - - def messages - return unless message.presence - [message] + payload[:messages] = messages.presence + payload[:error_messages] = error_messages if error_messages.any? + payload end end end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 9dc543e78494c2..fe90adf6940f4b 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -123,9 +123,10 @@ def when_saved( def do_commands(note, update_params, message, command_names, only_commands) status = ::Notes::QuickActionsStatus.new( - message: message, command_names: command_names&.flatten, commands_only: only_commands) + status.add_message(message) + note.quick_actions_status = status return if quick_actions_service.commands_executed_count.to_i == 0 @@ -133,13 +134,10 @@ def do_commands(note, update_params, message, command_names, only_commands) update_error = quick_actions_update_errors(note, update_params) if update_error note.errors.add(:validation, update_error) - status.message = update_error + status.add_error(update_error) end - if only_commands && message.blank? - status.error = true - status.message = _('Failed to apply commands.') - end + status.add_error(_('Failed to apply commands.')) if only_commands && message.blank? end def quick_actions_update_errors(note, params) diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index c9484464159293..3ee91a89e27066 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -30,7 +30,8 @@ def execute(note) note.note = content status = ::Notes::QuickActionsStatus.new( - message: message, command_names: command_names, commands_only: only_commands) + command_names: command_names, commands_only: only_commands) + status.add_message(message) note.quick_actions_status = status end @@ -74,10 +75,7 @@ def update_note(note, only_commands) end def delete_note(note, message) - if message.blank? - note.quick_actions_status.error = true - note.quick_actions_status.message = _('Commands did not apply') - end + note.quick_actions_status.add_error(_('Commands did not apply')) if message.blank? Notes::DestroyService.new(project, current_user).execute(note) end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 4092e364eacc45..ee61ac7eff8ef1 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -33459,8 +33459,8 @@ The amount of time for a job to be picked up by a runner, in percentiles. | ---- | ---- | ----------- | | `commandNames` | [`[String!]`](#string) | Quick action command names. | | `commandsOnly` | [`Boolean`](#boolean) | Returns true if only quick action commands were in the note. | -| `error` | [`Boolean`](#boolean) | Error in processing quick actions. | -| `messages` | [`[String!]`](#string) | Quick action response messages. | +| `errorMessages` | [`[String!]`](#string) | Error messages from quick actions that failed to apply. | +| `messages` | [`[String!]`](#string) | Response messages from quick actions. | ### `RecentFailures` diff --git a/lib/api/entities/note.rb b/lib/api/entities/note.rb index 0610e66dc950eb..40c4643f9686ec 100644 --- a/lib/api/entities/note.rb +++ b/lib/api/entities/note.rb @@ -44,7 +44,7 @@ class Note < Grape::Entity # To be returned if the note was command-only class NoteCommands < Grape::Entity expose(:commands_changes) { |note| note.commands_changes || {} } - expose(:summary) { |note| [note.quick_actions_status&.message] } + expose(:summary) { |note| note.quick_actions_status.messages } end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 12127477d6efba..e3f01b56a11f4f 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -106,6 +106,8 @@ class Notes < ::API::Base present note, with: Entities::NoteCommands elsif note.persisted? present note, with: Entities.const_get(note.class.name, false) + elsif quick_action_status&.error? + bad_request!(quick_action_status.error_messages.join(', ')) elsif note.errors.present? bad_request!("Note #{note.errors.messages}") end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 9c5a9634f281b0..c56e68862e4777 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -563,8 +563,7 @@ def create! { "messages" => nil, "command_names" => %w[spend], - "commands_only" => false, - "error" => false + "commands_only" => false } end @@ -583,8 +582,7 @@ def create! { "messages" => ["Set time estimate to 1d."], "command_names" => %w[estimate spend], - "commands_only" => false, - "error" => false + "commands_only" => false } end @@ -627,20 +625,19 @@ def create! let(:expected) do { 'quick_actions_status' => { - "messages" => ["Failed to apply commands."], + "error_messages" => ["Failed to apply commands."], "command_names" => ["label"], "commands_only" => true, - "error" => true + "messages" => nil } } end before do note.quick_actions_status = ::Notes::QuickActionsStatus.new( - message: 'Failed to apply commands.', command_names: ['label'], - commands_only: true, - error: true) + commands_only: true) + note.quick_actions_status.add_error('Failed to apply commands.') allow_next_instance_of(Notes::CreateService) do |service| allow(service).to receive(:execute).and_return(note) diff --git a/spec/frontend/notes/components/comment_form_spec.js b/spec/frontend/notes/components/comment_form_spec.js index b4a52bddf6ad48..4207ec50e48ba1 100644 --- a/spec/frontend/notes/components/comment_form_spec.js +++ b/spec/frontend/notes/components/comment_form_spec.js @@ -198,7 +198,7 @@ describe('issue_comment_form component', () => { saveNote: jest.fn().mockRejectedValue({ response: { status: httpStatus, - data: { quick_actions_status: { messages: errors } }, + data: { quick_actions_status: { error_messages: errors } }, }, }), }, @@ -261,7 +261,7 @@ describe('issue_comment_form component', () => { saveNote: jest.fn().mockRejectedValue({ response: { status: HTTP_STATUS_UNPROCESSABLE_ENTITY, - data: { quick_actions_status: { messages: [...commandErrors] } }, + data: { quick_actions_status: { error_messages: [...commandErrors] } }, }, }), }, diff --git a/spec/frontend/notes/utils_spec.js b/spec/frontend/notes/utils_spec.js index 1a7bd97e7e6694..1c86b73f7357ce 100644 --- a/spec/frontend/notes/utils_spec.js +++ b/spec/frontend/notes/utils_spec.js @@ -35,9 +35,8 @@ describe('createNoteErrorMessages', () => { base: ['base error 1'], }, quick_actions_status: { - error: true, commands_only: true, - messages: ['commands_only error 1', 'commands_only error 2'], + error_messages: ['commands_only error 1', 'commands_only error 2'], }, }, HTTP_STATUS_UNPROCESSABLE_ENTITY, diff --git a/spec/models/notes/quick_actions_status_spec.rb b/spec/models/notes/quick_actions_status_spec.rb new file mode 100644 index 00000000000000..56bc56ff3855e8 --- /dev/null +++ b/spec/models/notes/quick_actions_status_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Notes::QuickActionsStatus, feature_category: :team_planning do + let(:command_names) { %w[/assign /label] } + let(:commands_only) { true } + let(:status) { described_class.new(command_names: command_names, commands_only: commands_only) } + + describe '#add_message' do + it 'adds a non-blank message to messages' do + status.add_message('Test message') + + expect(status.messages).to eq(['Test message']) + end + + it 'does not add a blank message' do + status.add_message('') + + expect(status.messages).to be_empty + end + end + + describe '#add_error' do + it 'adds an error message to error_messages' do + status.add_error('Test error') + + expect(status.error_messages).to eq(['Test error']) + end + end + + describe '#commands_only?' do + it 'returns the value of commands_only' do + expect(status.commands_only?).to eq(commands_only) + end + end + + describe '#success?' do + it 'returns true when there are no error messages' do + expect(status.success?).to be true + end + + it 'returns false when there are error messages' do + status.add_error('Test error') + + expect(status.success?).to be false + end + end + + describe '#error?' do + it 'returns false when there are no error messages' do + expect(status.error?).to be false + end + + it 'returns true when there are error messages' do + status.add_error('Test error') + + expect(status.error?).to be true + end + end + + describe '#to_h' do + context 'when there are no messages or errors' do + it 'returns a hash with command_names and commands_only' do + expect(status.to_h).to eq({ + command_names: command_names, + commands_only: commands_only, + messages: nil + }) + end + end + + context 'when there are messages' do + before do + status.add_message('Test message') + end + + it 'includes messages in the hash' do + expect(status.to_h).to eq({ + command_names: command_names, + commands_only: commands_only, + messages: ['Test message'] + }) + end + end + + context 'when there are error messages' do + before do + status.add_error('Test error') + end + + it 'includes error_messages in the hash' do + expect(status.to_h).to eq({ + command_names: command_names, + commands_only: commands_only, + error_messages: ['Test error'], + messages: nil + }) + end + end + end +end diff --git a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb index b6c451f446a2d4..168f532f326c40 100644 --- a/spec/requests/api/graphql/mutations/notes/create/note_spec.rb +++ b/spec/requests/api/graphql/mutations/notes/create/note_spec.rb @@ -141,8 +141,8 @@ def mutation_response 'quickActionsStatus' => { "commandNames" => ["merge"], "commandsOnly" => true, - "error" => false, - "messages" => ["Merged this merge request."] + "messages" => ["Merged this merge request."], + "errorMessages" => nil }) end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index cadfe9e6c70578..6507285e4aaf9c 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -352,6 +352,30 @@ end end + context 'an invalid command-only note' do + let(:request_body) { "/spend asdf" } + + before do + project.add_developer(user) + end + + it 'returns a 400 and does not create a note' do + expect { subject }.not_to change { Note.where(system: false).count } + + expect(response).to have_gitlab_http_status(:bad_request) + end + + it 'does not apply the command' do + expect { subject }.not_to change { merge_request.reset.total_time_spent } + end + + it 'reports the errors' do + subject + + expect(json_response).to eq({ "message" => "400 Bad request - Failed to apply commands." }) + end + end + context 'a command only note' do context '/spend' do let(:request_body) { "/spend 1h" } diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 5860b25fc78ec1..44145afffa7b1b 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -513,14 +513,16 @@ note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.quick_actions_status.message).to be_present + expect(note.quick_actions_status.messages).to be_present + expect(note.quick_actions_status.error_messages).to be_empty end it 'adds commands failed message to note errors' do note_text = %(/reopen) note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.quick_actions_status.message).to eq('Could not apply reopen command.') + expect(note.quick_actions_status.messages).to eq(['Could not apply reopen command.']) + expect(note.quick_actions_status.error_messages).to be_empty end it 'generates success and failed error messages' do @@ -533,7 +535,8 @@ expect(note.quick_actions_status.error?).to be(false) expect(note.quick_actions_status.command_names).to eq(%w[close reopen]) - expect(note.quick_actions_status.message).to eq('Closed this issue. Could not apply reopen command.') + expect(note.quick_actions_status.messages).to eq(['Closed this issue. Could not apply reopen command.']) + expect(note.quick_actions_status.error_messages).to be_empty end it 'does not check for spam' do @@ -556,9 +559,9 @@ note = described_class.new(project, user, opts.merge(note: note_text)).execute - expect(note.quick_actions_status.error?).to be(false) + expect(note.quick_actions_status.error?).to be(true) expect(note.quick_actions_status.command_names).to eq(['confidential']) - expect(note.quick_actions_status.message).to eq('Confidential an error occurred') + expect(note.quick_actions_status.error_messages).to eq(['Confidential an error occurred']) end end end diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index 4d5d8568031ca9..ba38f1853a6e01 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -54,8 +54,8 @@ def update_note(opts) updated_note = described_class.new(project, user, edit_note_text).execute(note) expect(updated_note.destroyed?).to eq(true) - expect(updated_note.quick_actions_status.error).to be(true) - expect(updated_note.quick_actions_status.message).to eq('Commands did not apply') + expect(updated_note.quick_actions_status.error?).to be(true) + expect(updated_note.quick_actions_status.error_messages).to eq(['Commands did not apply']) end end @@ -146,7 +146,7 @@ def update_note(opts) ]) expect(updated_note.quick_actions_status.error?).to be(false) expect(updated_note.quick_actions_status.command_names).to eq(['close']) - expect(updated_note.quick_actions_status.message).to eq('Closed this issue.') + expect(updated_note.quick_actions_status.messages).to eq(['Closed this issue.']) end end diff --git a/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb b/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb index c77359f496b68a..299d95882fadf8 100644 --- a/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb +++ b/spec/support/shared_examples/graphql/notes_quick_actions_for_work_items_shared_examples.rb @@ -211,6 +211,7 @@ expect(mutation_response['errors']).to eq([]) expect(mutation_response['quickActionsStatus']['messages']) .to include("Type changed successfully. Assigned @#{assignee.username}.") + expect(mutation_response['quickActionsStatus']['error_messages']).to be_nil end end -- GitLab