Merge branch...

Merge branch '35627-api-response-for-adding-a-note-returns-http-400-for-command-only-notes' into 'master'

API requests creating command only notes should return 2xx responses

Closes #35627

See merge request gitlab-org/gitlab!19624

app/services/notes/create_service.rb

@@ -17,57 +17,72 @@ 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)
+      execute_quick_actions(note) do |only_commands|
+        note.run_after_commit do
+          # Finish the harder work in the background
+          NewNoteWorker.perform_async(note.id)
+        end
 
-        only_commands = content.empty?
+        note_saved = note.with_transaction_returning_status do
+          !only_commands && note.save
+        end
 
-        note.note = content
+        when_saved(note) if note_saved
       end
 
-      note.run_after_commit do
-        # Finish the harder work in the background
-        NewNoteWorker.perform_async(note.id)
-      end
+      note
+    end
 
-      note_saved = note.with_transaction_returning_status do
-        !only_commands && note.save
-      end
+    private
 
-      if note_saved
-        if note.part_of_discussion? && note.discussion.can_convert_to_discussion?
-          note.discussion.convert_to_discussion!(save: true)
-        end
+    def execute_quick_actions(note)
+      return yield(false) unless quick_actions_service.supported?(note)
 
-        todo_service.new_note(note, current_user)
-        clear_noteable_diffs_cache(note)
-        Suggestions::CreateService.new(note).execute
-        increment_usage_counter(note)
+      content, update_params, message = quick_actions_service.execute(note, quick_action_options)
+      only_commands = content.empty?
+      note.note = content
 
-        if Feature.enabled?(:notes_create_service_tracking, project)
-          Gitlab::Tracking.event('Notes::CreateService', 'execute', tracking_data_for(note))
-        end
-      end
+      yield(only_commands)
 
-      if quick_actions_service.commands_executed_count.to_i > 0
-        if update_params.present?
-          quick_actions_service.apply_updates(update_params, note)
-          note.commands_changes = update_params
-        end
+      do_commands(note, update_params, message, only_commands)
+    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.'))
-        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
 
-      note
+      todo_service.new_note(note, current_user)
+      clear_noteable_diffs_cache(note)
+      Suggestions::CreateService.new(note).execute
+      increment_usage_counter(note)
+
+      if Feature.enabled?(:notes_create_service_tracking, project)
+        Gitlab::Tracking.event('Notes::CreateService', 'execute', tracking_data_for(note))
+      end
     end
 
-    private
+    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
+      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.'))
+        # Allow consumers to detect problems applying commands
+        note.errors.add(:commands, _('Failed to apply commands.')) unless message.present?
+      end
+    end
 
     # EE::Notes::CreateService would override this method
     def quick_action_options

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