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