Simplify quick actions JS handling

Stop putting content in `note.errors`, instead set
`note.quick_actions_commands` and `note.quick_actions_results`,
and display a notice message on the frontend when branch cannot be created.

Signed-off-by: Rémy Coutable <remy@rymai.me>

16 files changed, 123 insertions, 72 deletions

diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js
index 1556cffb1ba..33d869caefc 100644
--- a/app/assets/javascripts/notes.js
+++ b/app/assets/javascripts/notes.js
@@ -292,16 +292,16 @@ export default class Notes {
 
   handleQuickActions(noteEntity) {
     var votesBlock;
-    if (noteEntity.commands_changes) {
-      if ('merge' in noteEntity.commands_changes) {
+    if (noteEntity.quick_actions_commands) {
+      if ('merge' in noteEntity.quick_actions_commands) {
         Notes.checkMergeRequestStatus();
       }
 
-      if ('emoji_award' in noteEntity.commands_changes) {
+      if ('emoji_award' in noteEntity.quick_actions_commands) {
         votesBlock = $('.js-awards-block').eq(0);
 
         loadAwardsHandler().then((awardsHandler) => {
-          awardsHandler.addAwardToEmojiBar(votesBlock, noteEntity.commands_changes.emoji_award);
+          awardsHandler.addAwardToEmojiBar(votesBlock, noteEntity.quick_actions_commands.emoji_award);
           awardsHandler.scrollToAwards();
         }).catch(() => {
           // ignore
@@ -348,15 +348,38 @@ export default class Notes {
       return this.renderDiscussionNote(noteEntity, $form);
     }
 
+    const quickActionsCommands = noteEntity.quick_actions_commands;
+    const hasQuickActionsCommands = quickActionsCommands && Object.keys(quickActionsCommands).length;
+    let quickActionsMessage = null;
+    let quickActionsMessageType = 'notice';
+
+    // If the note is invalid with quickActionsCommands, that means it only contained quick actions
     if (!noteEntity.valid) {
-      if (noteEntity.errors.commands_only) {
-        if (noteEntity.commands_changes &&
-            Object.keys(noteEntity.commands_changes).length > 0) {
-          $notesList.find('.system-note.being-posted').remove();
+      if (hasQuickActionsCommands) {
+        quickActionsMessage = 'Commands applied';
+      }
+
+      const quickActionsResults = noteEntity.quick_actions_results;
+
+      if (quickActionsResults && quickActionsResults.create_branch) {
+        const createBranchResult = quickActionsResults.create_branch;
+
+        if (createBranchResult.status === 'error') {
+          if (quickActionsMessage) {
+            quickActionsMessage = `Commands applied, but the branch '${createBranchResult.branch_name}' could not be created.`;
+          } else {
+            quickActionsMessage = `The branch '${createBranchResult.branch_name}' could not be created!`;
+            quickActionsMessageType = 'alert';
+          }
         }
-        this.addFlash(noteEntity.errors.commands_only.join(", "), 'notice', this.parentTimeline.get(0));
+      }
+
+      if (quickActionsMessage) {
+        this.addFlash(quickActionsMessage, quickActionsMessageType, this.parentTimeline.get(0));
         this.refresh();
+        $notesList.find('.system-note.being-posted').remove();
       }
+
       return;
     }
 
@@ -1549,7 +1572,7 @@ export default class Notes {
           this.reenableTargetFormSubmitButton(e);
         }
 
-        if (note.commands_changes) {
+        if (note.quick_actions_commands) {
           this.handleQuickActions(note);
         }
 
diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue
index e594377bc40..3abccbce22f 100644
--- a/app/assets/javascripts/notes/components/comment_form.vue
+++ b/app/assets/javascripts/notes/components/comment_form.vue
@@ -139,15 +139,11 @@
               this.restartPolling();
 
               if (res.errors) {
-                if (res.errors.commands_only) {
-                  this.discard();
-                } else {
-                  Flash(
-                    'Something went wrong while adding your comment. Please try again.',
-                    'alert',
-                    this.$refs.commentForm,
-                  );
-                }
+                Flash(
+                  'Something went wrong while adding your comment. Please try again.',
+                  'alert',
+                  this.$refs.commentForm,
+                );
               } else {
                 this.discard();
               }
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index 085b18642ba..7d012a78854 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -92,23 +92,45 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
 
   return dispatch(methodToDispatch, noteData)
     .then((res) => {
-      const { errors } = res;
-      const commandsChanges = res.commands_changes;
+      const quickActionsCommands = res.quick_actions_commands;
+      const hasQuickActionsCommands = quickActionsCommands
+        && Object.keys(quickActionsCommands).length;
+      let quickActionsMessage = null;
+      let quickActionsMessageType = 'notice';
+
+      // If the note is invalid with quickActionsCommands, that means it only
+      // contained quick actions
+      if (!res.valid) {
+        if (hasQuickActionsCommands) {
+          quickActionsMessage = 'Commands applied';
+        }
 
-      if (hasQuickActions && errors && Object.keys(errors).length) {
-        eTagPoll.makeRequest();
+        const quickActionsResults = res.quick_actions_results;
+
+        if (quickActionsResults && quickActionsResults.create_branch) {
+          const createBranchResult = quickActionsResults.create_branch;
+
+          if (createBranchResult.status === 'error') {
+            if (quickActionsMessage) {
+              quickActionsMessage = `Commands applied, but the branch '${createBranchResult.branch_name}' could not be created.`;
+            } else {
+              quickActionsMessage = `The branch '${createBranchResult.branch_name}' could not be created!`;
+              quickActionsMessageType = 'alert';
+            }
+          } else {
+            quickActionsMessage = 'Commands applied';
+          }
+        }
 
-        $('.js-gfm-input').trigger('clear-commands-cache.atwho');
-        Flash('Commands applied', 'notice', noteData.flashContainer);
-      }
+        if (quickActionsMessage) {
+          Flash(quickActionsMessage, quickActionsMessageType, noteData.flashContainer);
 
-      if (commandsChanges) {
-        if (commandsChanges.emoji_award) {
-          const votesBlock = $('.js-awards-block').eq(0);
+          if (quickActionsCommands.emoji_award) {
+            const votesBlock = $('.js-awards-block').eq(0);
 
-          loadAwardsHandler()
+            loadAwardsHandler()
             .then((awardsHandler) => {
-              awardsHandler.addAwardToEmojiBar(votesBlock, commandsChanges.emoji_award);
+              awardsHandler.addAwardToEmojiBar(votesBlock, quickActionsCommands.emoji_award);
               awardsHandler.scrollToAwards();
             })
             .catch(() => {
@@ -118,16 +140,21 @@ export const saveNote = ({ commit, dispatch }, noteData) => {
                 noteData.flashContainer,
               );
             });
-        }
+          }
 
-        if (commandsChanges.spend_time != null || commandsChanges.time_estimate != null) {
-          sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', res);
+          if (quickActionsCommands.spend_time != null
+            || quickActionsCommands.time_estimate != null) {
+            sidebarTimeTrackingEventHub.$emit('timeTrackingUpdated', res);
+          }
         }
       }
 
-      if (errors && errors.commands_only) {
-        Flash(errors.commands_only, 'notice', noteData.flashContainer);
+      if (hasQuickActionsCommands) {
+        eTagPoll.makeRequest();
+
+        $('.js-gfm-input').trigger('clear-commands-cache.atwho');
       }
+
       commit(types.REMOVE_PLACEHOLDER_NOTES);
 
       return res;
diff --git a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js
index d32fe4abc7d..7ddbcff49fd 100644
--- a/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js
+++ b/app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.js
@@ -29,8 +29,8 @@ export default {
       const subscribedCommands = ['spend_time', 'time_estimate'];
       let changedCommands;
       if (data !== undefined) {
-        changedCommands = data.commands_changes
-          ? Object.keys(data.commands_changes)
+        changedCommands = data.quick_actions_commands
+          ? Object.keys(data.quick_actions_commands)
           : [];
       } else {
         changedCommands = [];
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index e82a5650935..2fd86a4ad23 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/app/controllers/concerns/notes_actions.rb
@@ -89,7 +89,8 @@ module NotesActions
 
   def note_json(note)
     attrs = {
-      commands_changes: note.commands_changes
+      quick_actions_commands: note.quick_actions_commands,
+      quick_actions_results: note.quick_actions_results
     }
 
     if note.persisted?
diff --git a/app/models/note.rb b/app/models/note.rb
index 184fbd5f5ae..b201050bb57 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -45,7 +45,9 @@ class Note < ActiveRecord::Base
   attr_accessor :user_visible_reference_count
 
   # Attribute used to store the attributes that have been changed by quick actions.
-  attr_accessor :commands_changes
+  attr_accessor :quick_actions_commands
+  # Attribute used to store results of quick actions that don't act on the note's noteable.
+  attr_accessor :quick_actions_results
 
   # A special role that may be displayed on issuable's discussions
   attr_accessor :special_role
diff --git a/app/services/create_branch_service.rb b/app/services/create_branch_service.rb
index e33d549486a..7f2c7bbb5d0 100644
--- a/app/services/create_branch_service.rb
+++ b/app/services/create_branch_service.rb
@@ -14,20 +14,20 @@ class CreateBranchService < BaseService
     new_branch = repository.add_branch(current_user, sanitized_branch_name, ref)
 
     if new_branch
-      success(new_branch)
+      success(branch: new_branch)
     else
-      error('Invalid reference name')
+      error('Invalid reference name', nil, branch_name)
     end
   rescue Gitlab::Git::HooksService::PreReceiveError => ex
-    error(ex.message)
-  end
-
-  def success(branch)
-    super().merge(branch: branch)
+    error(ex.message,  nil, branch_name)
   end
 
   private
 
+  def error(message, http_status = nil, branch_name = nil)
+    super(message, http_status).merge(branch_name: branch_name)
+  end
+
   def create_master_branch
     project.repository.create_file(
       current_user,
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 47e20f5fe95..179a08d1331 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -37,18 +37,8 @@ module Notes
 
       quick_actions_service.execute(commands, note)
 
-      # We must add the error after we call #save because errors are reset
-      # when #save is called
-      if only_quick_actions
-        note.errors.add(:commands_only, 'Commands applied')
-      end
-
-      create_branch_result = results&.delete(:create_branch)
-      if create_branch_result&.fetch(:status) == :error
-        note.errors.add(:commands_only, "Error creating branch: #{create_branch_result.fetch(:message)}")
-      end
-
-      note.commands_changes = commands
+      note.quick_actions_commands = commands
+      note.quick_actions_results = results
 
       note
     end
diff --git a/app/services/validate_new_branch_service.rb b/app/services/validate_new_branch_service.rb
index 7d1ed768ee8..f67f9e1800e 100644
--- a/app/services/validate_new_branch_service.rb
+++ b/app/services/validate_new_branch_service.rb
@@ -5,15 +5,21 @@ class ValidateNewBranchService < BaseService
     valid_branch = Gitlab::GitRefValidator.validate(branch_name)
 
     unless valid_branch
-      return error('Branch name is invalid')
+      return error('Branch name is invalid',  nil, branch_name)
     end
 
     if project.repository.branch_exists?(branch_name)
-      return error('Branch already exists')
+      return error('Branch already exists',  nil, branch_name)
     end
 
     success
   rescue Gitlab::Git::HooksService::PreReceiveError => ex
-    error(ex.message)
+    error(ex.message,  nil, branch_name)
+  end
+
+  private
+
+  def error(message, http_status = nil, branch_name = nil)
+    super(message, http_status).merge(branch_name: branch_name)
   end
 end
diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb
index 8eea33b9ab5..5711bf2deca 100644
--- a/lib/gitlab/email/handler/create_note_handler.rb
+++ b/lib/gitlab/email/handler/create_note_handler.rb
@@ -44,6 +44,16 @@ module Gitlab
         def create_note
           sent_notification.create_reply(message)
         end
+
+        def note_contains_only_quick_actions?(note)
+          note.note.empty? && (note.quick_actions_commands.any? || note.quick_actions_results.any?)
+        end
+
+        def verify_record!(record:, invalid_exception:, record_name:)
+          return if note_contains_only_quick_actions?(record)
+
+          super
+        end
       end
     end
   end
diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb
index 32c5caf93e8..41f4dffb0fe 100644
--- a/lib/gitlab/email/handler/reply_processing.rb
+++ b/lib/gitlab/email/handler/reply_processing.rb
@@ -38,7 +38,6 @@ module Gitlab
 
         def verify_record!(record:, invalid_exception:, record_name:)
           return if record.persisted?
-          return if record.errors.key?(:commands_only)
 
           error_title = "The #{record_name} could not be created for the following reasons:"
 
diff --git a/spec/features/issues/user_uses_slash_commands_spec.rb b/spec/features/issues/user_uses_slash_commands_spec.rb
index 8ec4fe90849..761ef12609e 100644
--- a/spec/features/issues/user_uses_slash_commands_spec.rb
+++ b/spec/features/issues/user_uses_slash_commands_spec.rb
@@ -284,7 +284,7 @@ feature 'Issues > User uses quick actions', :js do
           write_note("/create_branch A New Feature")
 
           expect(page).not_to have_content '/create_branch'
-          expect(page).to have_content 'Branch name is invalid'
+          expect(page).to have_content "The branch 'A New Feature' could not be created!"
         end
       end
 
diff --git a/spec/fixtures/emails/commands_only_reply.eml b/spec/fixtures/emails/only_quick_actions_reply.eml
index 2d2e2f94290..2d2e2f94290 100644
--- a/spec/fixtures/emails/commands_only_reply.eml
+++ b/spec/fixtures/emails/only_quick_actions_reply.eml
diff --git a/spec/fixtures/emails/commands_in_reply.eml b/spec/fixtures/emails/quick_actions_in_reply.eml
index 712f6f797b4..712f6f797b4 100644
--- a/spec/fixtures/emails/commands_in_reply.eml
+++ b/spec/fixtures/emails/quick_actions_in_reply.eml
diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js
index e09b8dc7fc5..22ffc028258 100644
--- a/spec/javascripts/notes_spec.js
+++ b/spec/javascripts/notes_spec.js
@@ -550,16 +550,13 @@ import '~/notes';
       });
     });
 
-    describe('postComment with Slash commands', () => {
+    describe('postComment with Quick actions', () => {
       const sampleComment = '/assign @root\n/award :100:';
       const note = {
-        commands_changes: {
+        quick_actions_commands: {
           assignee_id: 1,
           emoji_award: '100'
         },
-        errors: {
-          commands_only: ['Commands applied']
-        },
         valid: false
       };
       let $form;
@@ -583,7 +580,7 @@ import '~/notes';
         $form.find('textarea.js-note-text').val(sampleComment);
       });
 
-      it('should remove slash command placeholder when comment with slash commands is done posting', () => {
+      it('should remove quick action placeholder when comment with quick actions is done posting', () => {
         const deferred = $.Deferred();
         spyOn($, 'ajax').and.returnValue(deferred.promise());
         spyOn(gl.awardsHandler, 'addAwardToEmojiBar').and.callThrough();
diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
index d0fa16ce4d1..b1f17adbd31 100644
--- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
@@ -56,7 +56,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
     end
 
     context 'because the note was commands only' do
-      let!(:email_raw) { fixture_file("emails/commands_only_reply.eml") }
+      let!(:email_raw) { fixture_file("emails/only_quick_actions_reply.eml") }
 
       context 'and current user cannot update noteable' do
         it 'raises a CommandsOnlyNoteError' do
@@ -83,7 +83,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
   end
 
   context 'when the note contains quick actions' do
-    let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") }
+    let!(:email_raw) { fixture_file("emails/quick_actions_in_reply.eml") }
 
     context 'and current user cannot update noteable' do
       it 'post a note and does not update the noteable' do