Report command only notes correctly

This changes the behaviour of note creation to respond with HTTP-202
when the note is command only - i.e. no note was created, but the
changes mentioned were applied.

The response type is also changed to expose the command changes,
allowing the client to verify that their commands were correctly
applied, and allowing the client to distinguish from real notes that
have been persisted to the database from command only responses with a
mechanism other than just checking the note id.

Be aware that as a result of this change the response from this service
will now return notes that have a null ID, a condition that previously
excluded.

app/services/notes/create_service.rb

@@ -17,16 +17,8 @@ module Notes
       # We execute commands (extracted from `params[:note]`) on the noteable
       # **before** we save the note because if the note consists of commands
       # only, there is no need be create a note!
-      quick_actions_service = QuickActionsService.new(project, current_user)
-
-      if quick_actions_service.supported?(note)
-        content, update_params, message = quick_actions_service.execute(note, quick_action_options)
-
-        only_commands = content.empty?
-
-        note.note = content
-      end
 
+      execute_quick_actions(note) do |only_commands|
         note.run_after_commit do
           # Finish the harder work in the background
           NewNoteWorker.perform_async(note.id)
@@ -36,7 +28,31 @@ module Notes
           !only_commands && note.save
         end
 
-      if note_saved
+        when_saved(note) if note_saved
+      end
+
+      note
+    end
+
+    private
+
+    def execute_quick_actions(note)
+      return yield(false) unless quick_actions_service.supported?(note)
+
+      content, update_params, message = quick_actions_service.execute(note, quick_action_options)
+      only_commands = content.empty?
+      note.note = content
+
+      yield(only_commands)
+
+      do_commands(note, update_params, message, only_commands)
+    end
+
+    def quick_actions_service
+      @quick_actions_service ||= QuickActionsService.new(project, current_user)
+    end
+
+    def when_saved(note)
       if note.part_of_discussion? && note.discussion.can_convert_to_discussion?
         note.discussion.convert_to_discussion!(save: true)
       end
@@ -51,7 +67,9 @@ module Notes
       end
     end
 
-      if quick_actions_service.commands_executed_count.to_i > 0
+    def do_commands(note, update_params, message, only_commands)
+      return if quick_actions_service.commands_executed_count.to_i.zero?
+
       if update_params.present?
         quick_actions_service.apply_updates(update_params, note)
         note.commands_changes = update_params
@@ -61,14 +79,11 @@ module Notes
       # when #save is called
       if only_commands
         note.errors.add(:commands_only, message.presence || _('Failed to apply commands.'))
+        # Allow consumers to detect problems applying commands
+        note.errors.add(:commands, _('Failed to apply commands.')) unless message.present?
       end
     end
 
-      note
-    end
-
-    private
-
     # EE::Notes::CreateService would override this method
     def quick_action_options
       { merge_request_diff_head_sha: params[:merge_request_diff_head_sha] }

app/services/notes/update_service.rb

@@ -55,6 +55,8 @@ module Notes
       # 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?
 
       Notes::DestroyService.new(project, current_user).execute(note)
     end

changelogs/unreleased/35627-api-response-for-adding-a-note-returns-http-400-for-command-only-no.yml

+---
+title: Return 202 for command only notes in REST API
+merge_request: 19624
+author:
+type: fixed

ee/spec/requests/api/notes_spec.rb

@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 describe API::Notes do
-  let(:user) { create(:user) }
-  let!(:project) { create(:project, :public, namespace: user.namespace) }
+  let!(:user) { create(:user) }
+  let!(:project) { create(:project, :public) }
   let(:private_user) { create(:user) }
 
   before do

lib/api/entities/note.rb

@@ -25,6 +25,14 @@ module API
 
       # Avoid N+1 queries as much as possible
       expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
+
+      expose(:commands_changes) { |note| note.commands_changes || {} }
+    end
+
+    # 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] }
     end
   end
 end

lib/api/helpers/notes_helpers.rb

@@ -113,6 +113,7 @@ module API
       end
 
       def create_note(noteable, opts)
+        whitelist_query_limiting
         authorize!(:create_note, noteable)
 
         parent = noteable_parent(noteable)
@@ -139,6 +140,10 @@ module API
 
         present discussion, with: Entities::Discussion
       end
+
+      def whitelist_query_limiting
+        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/211538')
+      end
     end
   end
 end

lib/api/notes.rb

@@ -82,9 +82,13 @@ module API
 
           note = create_note(noteable, opts)
 
-          if note.valid?
+          if note.errors.keys == [:commands_only]
+            status 202
+            present note, with: Entities::NoteCommands
+          elsif note.valid?
             present note, with: Entities.const_get(note.class.name, false)
           else
+            note.errors.delete(:commands_only) if note.errors.has_key?(:commands)
             bad_request!("Note #{note.errors.messages}")
           end
         end

spec/fixtures/api/schemas/entities/discussion.json

@@ -54,7 +54,8 @@
           "cached_markdown_version": { "type": "integer" },
           "human_access": { "type": ["string", "null"] },
           "toggle_award_path": { "type": "string" },
-          "path": { "type": "string" }
+          "path": { "type": "string" },
+          "commands_changes": { "type": "object", "additionalProperties": true }
         },
         "required": [
           "id", "attachment", "author", "created_at", "updated_at",

spec/fixtures/api/schemas/public_api/v4/notes.json

@@ -19,6 +19,7 @@
         },
         "additionalProperties": false
       },
+      "commands_changes": { "type": "object", "additionalProperties": true },
       "created_at": { "type": "date" },
       "updated_at": { "type": "date" },
       "system": { "type": "boolean" },

spec/requests/api/notes_spec.rb

@@ -3,8 +3,8 @@
 require 'spec_helper'
 
 describe API::Notes do
-  let(:user) { create(:user) }
-  let!(:project) { create(:project, :public, namespace: user.namespace) }
+  let!(:user) { create(:user) }
+  let!(:project) { create(:project, :public) }
   let(:private_user) { create(:user) }
 
   before do
@@ -226,14 +226,56 @@ describe API::Notes do
       let(:note) { merge_request_note }
     end
 
+    let(:request_body) { 'Hi!' }
+    let(:request_path) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes" }
+
+    subject { post api(request_path, user), params: { body: request_body } }
+
+    context 'a command only note' do
+      let(:assignee) { create(:user) }
+      let(:request_body) { "/assign #{assignee.to_reference}" }
+
+      before do
+        project.add_developer(assignee)
+        project.add_developer(user)
+      end
+
+      it 'returns 202 Accepted status' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:accepted)
+      end
+
+      it 'does not actually create a new note' do
+        expect { subject }.not_to change { Note.where(system: false).count }
+      end
+
+      it 'does however create a system note about the change' do
+        expect { subject }.to change { Note.system.count }.by(1)
+      end
+
+      it 'applies the commands' do
+        expect { subject }.to change { merge_request.reset.assignees }
+      end
+
+      it 'reports the changes' do
+        subject
+
+        expect(json_response).to include(
+          'commands_changes' => include(
+            'assignee_ids' => [Integer]
+          ),
+          'summary' => include("Assigned #{assignee.to_reference}.")
+        )
+      end
+    end
+
     context 'when the merge request discussion is locked' do
       before do
         merge_request.update_attribute(:discussion_locked, true)
       end
 
       context 'when a user is a team member' do
-        subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user), params: { body: 'Hi!' } }
-
         it 'returns 200 status' do
           subject
 

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -172,6 +172,8 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
 
       if parent_type == 'projects'
         context 'by a project owner' do
+          let(:user) { project.owner }
+
           it 'sets the creation time on the new note' do
             post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes", user), params: params