diff --git a/app/assets/javascripts/deprecated_notes.js b/app/assets/javascripts/deprecated_notes.js index 19d4b363bf5feb8dc04fcb7826409cd1b8769845..d3886da1133b1b67bfbcb9bffa10841c28820f9b 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 c75bda99ca2809f9f4a64aa2dd57e6a69eafc206..175fd9b801db88d73625732be6362a1c95ef2bd2 100644 --- a/app/assets/javascripts/notes/store/legacy_notes/actions.js +++ b/app/assets/javascripts/notes/store/legacy_notes/actions.js @@ -509,14 +509,8 @@ export function saveNote(noteData) { } const processQuickActions = (res) => { - const { - errors: { commands_only: commandsOnly } = { - commands_only: null, - command_names: [], - }, - command_names: commandNames, - } = res; - const message = commandsOnly; + const { quick_actions_status: { messages = null, command_names: commandNames = [] } = {} } = + res; if (commandNames?.indexOf('submit_review') >= 0) { useBatchComments().clearDrafts(); @@ -525,14 +519,14 @@ 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}} */ - 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(); } @@ -540,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 c3261257dddddb6bc7d45f44f3f5caf832d2eea2..2423a5d418e0a0960c432cc8f240a8bf426cf953 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -492,15 +492,8 @@ export const saveNote = ({ commit, dispatch }, noteData) => { } const processQuickActions = (res) => { - const { - errors: { commands_only: commandsOnly } = { - commands_only: null, - command_names: [], - }, - command_names: commandNames, - } = res; - const message = commandsOnly; - + const { quick_actions_status: { messages = null, command_names: commandNames = [] } = {} } = + res; if (commandNames?.indexOf('submit_review') >= 0) { dispatch('batchComments/clearDrafts'); } @@ -508,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":{"commands_only":["Commands applied"]}} + {"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(); } @@ -523,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 4ccf7cae5d47eea499bdcfa8af11716b94265592..0d818433ea34b2ea08a5f92f0cd4eab2ccf2e887 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?.error_messages?.length) { + return data.quick_actions_status.error_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 0a0e614717b3f977b13f41100a3553312427fbcc..22a0a3886d8696190d3cea67be2db829f3274a06 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/graphql/mutations/notes/base.rb b/app/graphql/mutations/notes/base.rb index 58c075cd77129a571602b12a19531f0dae78d8b4..9d5f4f0efc23497d613b63b502025c526dc1001b 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 22cccbce91a60030104b3943b4fb30fbc799fa48..636129f8dc3a95ed45b8cdd0da3a46e403640dd2 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 5edb4fb023be12dc713e9ab44c4c2dcc3046d971..16a16f43f39d25801c62f3de965ac9086baea4d2 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 0000000000000000000000000000000000000000..a7db400c462543fd19133d546f411a876f3f876f --- /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: 'Response messages from quick actions.' + + 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_messages, [GraphQL::Types::String], + null: true, + description: 'Error messages from quick actions that failed to apply.' + end + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 91659615f76c695da5c3f19662f89cdc50c2f3b8..9b7fe1a4da1855a18e2479b32b4af3b55cb61125 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/models/notes/quick_actions_status.rb b/app/models/notes/quick_actions_status.rb new file mode 100644 index 0000000000000000000000000000000000000000..d55a8656826f54cef0a902ed1b2fec41e8a9fdcf --- /dev/null +++ b/app/models/notes/quick_actions_status.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Notes + class QuickActionsStatus + attr_accessor :messages, :error_messages, :commands_only, :command_names + + def initialize(command_names:, commands_only:) + @command_names = command_names + @commands_only = commands_only + @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? + commands_only + end + + def success? + !error? + end + + def error? + error_messages.any? + end + + def to_h + payload = { + command_names: command_names, + commands_only: commands_only + } + + 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 ee4a600bde36041e5b5a0db132e7a4e2d6c82c42..fe90adf6940f4be4e4aafbb6bfd21eaa6810fdd7 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,22 +122,22 @@ def when_saved( end def do_commands(note, update_params, message, command_names, only_commands) + status = ::Notes::QuickActionsStatus.new( + 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 update_error = quick_actions_update_errors(note, update_params) if update_error note.errors.add(:validation, update_error) - message = update_error + status.add_error(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? - 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 4916a5f2691ea6e729ab583978284c38245af283..3ee91a89e27066bbcfa3c258aacae08ffd6d4bca 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -24,11 +24,15 @@ 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( + command_names: command_names, commands_only: only_commands) + status.add_message(message) + note.quick_actions_status = status end update_note(note, only_commands) @@ -71,11 +75,7 @@ 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? + 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 b4b3ad7bb14acf26301f2dd2f3b2d90c9d178120..ee61ac7eff8ef158f58e2111d64476d36ae11a2a 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. | +| `errorMessages` | [`[String!]`](#string) | Error messages from quick actions that failed to apply. | +| `messages` | [`[String!]`](#string) | Response messages from quick actions. | + ### `RecentFailures` Recent failure history of a test case. diff --git a/lib/api/entities/note.rb b/lib/api/entities/note.rb index d40358185722bbf76820bde742147c4359257beb..40c4643f9686ec0436b9f81860cfb39834807f24 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.messages } end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 9596a4adea3d1c9b52f21dc0c07226b1bbb9f33c..e3f01b56a11f4fe1caaa5dd435a7eb32f4920883 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -99,14 +99,16 @@ 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 quick_action_status&.error? + bad_request!(quick_action_status.error_messages.join(', ')) + 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 6404a3a54f984b01b107a7a88ee950d55b97bc09..9fe10fe7294018eac604c68f38b39a49c8c32816 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 2f848125b10008828503a64c15bf26b623373daa..c56e68862e47778f5c9b6141d11967316477d7c3 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -551,7 +551,46 @@ 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 + } + 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 + } + 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 +615,29 @@ 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' => { + "error_messages" => ["Failed to apply commands."], + "command_names" => ["label"], + "commands_only" => true, + "messages" => nil + } + } + 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( + command_names: ['label'], + 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) @@ -600,7 +648,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 77ac1f8a39f75a8346863b318186f5ccbffabf13..4207ec50e48ba1378057d6c83aed4521b23cff50 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: { error_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: { error_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 3553169f8a21031b130b53fd1c117b6663a332d3..f07fc3a530b5df3b616eea0888ecd8081677511e 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 59ca7eea60be0a22c29e469fa3519d3806798b8c..40b621009047255c4babcbc9241e04c17e896404 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 3607c3c546c99c9ce1c73bd82a0e56df6f2a8dc8..1c86b73f7357ce5c42ec4eab98ed1b85426e8c7d 100644 --- a/spec/frontend/notes/utils_spec.js +++ b/spec/frontend/notes/utils_spec.js @@ -32,9 +32,12 @@ describe('createNoteErrorMessages', () => { const errorMessages = createNoteErrorMessages( { errors: { - commands_only: ['commands_only error 1', 'commands_only error 2'], base: ['base error 1'], }, + quick_actions_status: { + commands_only: true, + 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 0000000000000000000000000000000000000000..56bc56ff3855e887abc2ec0a00ea220863f12fd5 --- /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 dd3763fc180fa2c49e8f9544b90aa0f0dcae89e2..168f532f326c406743e184fd9d27a9e5ac2e5f52 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, + "messages" => ["Merged this merge request."], + "errorMessages" => nil + }) end it 'starts the merge process' do diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 50c63a70a33a2072a07d9e8cd451362f3e63dbf0..6507285e4aaf9cf954f5c4a3bfe8ed35410009c8 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -312,6 +312,70 @@ 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 '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 c0393d1e434dc03189f8af95ef79b40f5dd18ec4..44145afffa7b1ba72a07ebe471ec0e2bffb37021 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.errors[:commands_only]).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.errors[:commands_only]).to contain_exactly('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 @@ -531,7 +533,10 @@ 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.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 @@ -553,7 +558,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(true) + expect(note.quick_actions_status.command_names).to eq(['confidential']) + 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 abb64b3a62e2b7936959263e55c25a75d810dffc..ba38f1853a6e0181c99ba2c431a046c3a942f236 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.error_messages).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.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 0e0aff02dd3d6072eaa2704f8c2549d90a282346..299d95882fadf862f0de7e55882a7b602e48971b 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,10 @@ .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}.") + expect(mutation_response['quickActionsStatus']['error_messages']).to be_nil end end