Merge branch 'improve-quick-action-messages' into 'master'

Improve quick action error messages

See merge request gitlab-org/gitlab-ce!31451

app/models/label.rb

@@ -138,6 +138,8 @@ class Label < ApplicationRecord
   end
 
   def self.on_project_board?(project_id, label_id)
+    return false if label_id.blank?
+
     on_project_boards(project_id).where(id: label_id).exists?
   end
 

app/services/notes/create_service.rb

@@ -53,7 +53,7 @@ module Notes
         # 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 || _('Commands did not apply'))
+          note.errors.add(:commands_only, message.presence || _('Failed to apply commands.'))
         end
       end
 

changelogs/unreleased/improve-quick-action-messages.yml

+---
+title: Improve quick action error messages
+merge_request: 31451
+author:
+type: other

lib/gitlab/quick_actions/issuable_actions.rb

@@ -145,9 +145,9 @@ module Gitlab
           run_label_command(labels: find_labels(labels_param), command: :relabel, updates_key: :label_ids)
         end
 
-        desc _('Add a todo')
-        explanation _('Adds a todo.')
-        execution_message _('Added a todo.')
+        desc _('Add a To Do')
+        explanation _('Adds a To Do.')
+        execution_message _('Added a To Do.')
         types Issuable
         condition do
           quick_action_target.persisted? &&
@@ -157,9 +157,9 @@ module Gitlab
           @updates[:todo_event] = 'add'
         end
 
-        desc _('Mark to do as done')
-        explanation _('Marks to do as done.')
-        execution_message _('Marked to do as done.')
+        desc _('Mark To Do as done')
+        explanation _('Marks To Do as done.')
+        execution_message _('Marked To Do as done.')
         types Issuable
         condition do
           quick_action_target.persisted? &&

lib/gitlab/quick_actions/issue_actions.rb

@@ -57,19 +57,18 @@ module Gitlab
           labels = find_labels(target_list_name)
           label_ids = labels.map(&:id)
 
-          if label_ids.size == 1
+          if label_ids.size > 1
+            message = _('Failed to move this issue because only a single label can be provided.')
+          elsif !Label.on_project_board?(quick_action_target.project_id, label_ids.first)
+            message = _('Failed to move this issue because label was not found.')
+          else
             label_id = label_ids.first
 
-            # Ensure this label corresponds to a list on the board
-            next unless Label.on_project_board?(quick_action_target.project_id, label_id)
-
             @updates[:remove_label_ids] =
               quick_action_target.labels.on_project_boards(quick_action_target.project_id).where.not(id: label_id).pluck(:id) # rubocop: disable CodeReuse/ActiveRecord
             @updates[:add_label_ids] = [label_id]
 
             message = _("Moved issue to %{label} column in the board.") % { label: labels_to_reference(labels).first }
-          else
-            message = _('Move this issue failed because you need to specify only one label.')
           end
 
           @execution_message[:board_move] = message
@@ -93,7 +92,7 @@ module Gitlab
 
             message = _("Marked this issue as a duplicate of %{duplicate_param}.") % { duplicate_param: duplicate_param }
           else
-            message = _('Mark as duplicate failed because referenced issue was not found')
+            message = _('Failed to mark this issue as a duplicate because referenced issue was not found.')
           end
 
           @execution_message[:duplicate] = message
@@ -117,18 +116,18 @@ module Gitlab
 
             message = _("Moved this issue to %{path_to_project}.") % { path_to_project: target_project_path }
           else
-            message = _("Move this issue failed because target project doesn't exists")
+            message = _("Failed to move this issue because target project doesn't exist.")
           end
 
           @execution_message[:move] = message
         end
 
-        desc _('Make issue confidential.')
+        desc _('Make issue confidential')
         explanation do
-          _('Makes this issue confidential')
+          _('Makes this issue confidential.')
         end
         execution_message do
-          _('Made this issue confidential')
+          _('Made this issue confidential.')
         end
         types Issue
         condition do
@@ -138,19 +137,19 @@ module Gitlab
           @updates[:confidential] = true
         end
 
-        desc _('Create a merge request.')
+        desc _('Create a merge request')
         explanation do |branch_name = nil|
           if branch_name
-            _("Creates branch '%{branch_name}' and a merge request to resolve this issue") % { branch_name: branch_name }
+            _("Creates branch '%{branch_name}' and a merge request to resolve this issue.") % { branch_name: branch_name }
           else
-            _('Creates a branch and a merge request to resolve this issue')
+            _('Creates a branch and a merge request to resolve this issue.')
           end
         end
         execution_message do |branch_name = nil|
           if branch_name
-            _("Created branch '%{branch_name}' and a merge request to resolve this issue") % { branch_name: branch_name }
+            _("Created branch '%{branch_name}' and a merge request to resolve this issue.") % { branch_name: branch_name }
           else
-            _('Created a branch and a merge request to resolve this issue')
+            _('Created a branch and a merge request to resolve this issue.')
           end
         end
         params "<branch name>"

lib/gitlab/quick_actions/issue_and_merge_request_actions.rb

@@ -24,7 +24,7 @@ module Gitlab
         end
         command :assign do |users|
           if users.empty?
-            @execution_message[:assign] = _("Assign command failed because no user was found")
+            @execution_message[:assign] = _("Failed to assign a user because no user was found.")
             next
           end
 
@@ -211,8 +211,8 @@ module Gitlab
         end
 
         desc _("Lock the discussion")
-        explanation _("Locks the discussion")
-        execution_message _("Locked the discussion")
+        explanation _("Locks the discussion.")
+        execution_message _("Locked the discussion.")
         types Issue, MergeRequest
         condition do
           quick_action_target.persisted? &&
@@ -224,8 +224,8 @@ module Gitlab
         end
 
         desc _("Unlock the discussion")
-        explanation _("Unlocks the discussion")
-        execution_message _("Unlocked the discussion")
+        explanation _("Unlocks the discussion.")
+        execution_message _("Unlocked the discussion.")
         types Issue, MergeRequest
         condition do
           quick_action_target.persisted? &&

locale/gitlab.pot

@@ -675,9 +675,6 @@ msgstr ""
 msgid "Add a task list"
 msgstr ""
 
-msgid "Add a todo"
-msgstr ""
-
 msgid "Add an SSH key"
 msgstr ""
 
@@ -753,7 +750,7 @@ msgstr ""
 msgid "Added %{label_references} %{label_text}."
 msgstr ""
 
-msgid "Added a todo."
+msgid "Added a To Do."
 msgstr ""
 
 msgid "Added at"
@@ -768,7 +765,7 @@ msgstr ""
 msgid "Adds %{labels} %{label_text}."
 msgstr ""
 
-msgid "Adds a todo."
+msgid "Adds a To Do."
 msgstr ""
 
 msgid "Admin Area"
@@ -1365,9 +1362,6 @@ msgstr ""
 msgid "Assign"
 msgstr ""
 
-msgid "Assign command failed because no user was found"
-msgstr ""
-
 msgid "Assign custom color like #FF0000"
 msgstr ""
 
@@ -2968,9 +2962,6 @@ msgstr ""
 msgid "Commands applied"
 msgstr ""
 
-msgid "Commands did not apply"
-msgstr ""
-
 msgid "Comment"
 msgstr ""
 
@@ -3396,7 +3387,7 @@ msgstr ""
 msgid "Create a GitLab account first, and then connect it to your %{label} account."
 msgstr ""
 
-msgid "Create a merge request."
+msgid "Create a merge request"
 msgstr ""
 
 msgid "Create a new branch"
@@ -3504,10 +3495,10 @@ msgstr ""
 msgid "Created At"
 msgstr ""
 
-msgid "Created a branch and a merge request to resolve this issue"
+msgid "Created a branch and a merge request to resolve this issue."
 msgstr ""
 
-msgid "Created branch '%{branch_name}' and a merge request to resolve this issue"
+msgid "Created branch '%{branch_name}' and a merge request to resolve this issue."
 msgstr ""
 
 msgid "Created by me"
@@ -3519,10 +3510,10 @@ msgstr ""
 msgid "Created on:"
 msgstr ""
 
-msgid "Creates a branch and a merge request to resolve this issue"
+msgid "Creates a branch and a merge request to resolve this issue."
 msgstr ""
 
-msgid "Creates branch '%{branch_name}' and a merge request to resolve this issue"
+msgid "Creates branch '%{branch_name}' and a merge request to resolve this issue."
 msgstr ""
 
 msgid "Creating graphs uses the data from the Prometheus server. If this takes a long time, ensure that data is available."
@@ -4799,6 +4790,12 @@ msgstr ""
 msgid "Failed create wiki"
 msgstr ""
 
+msgid "Failed to apply commands."
+msgstr ""
+
+msgid "Failed to assign a user because no user was found."
+msgstr ""
+
 msgid "Failed to change the owner"
 msgstr ""
 
@@ -4841,6 +4838,18 @@ msgstr ""
 msgid "Failed to load related branches"
 msgstr ""
 
+msgid "Failed to mark this issue as a duplicate because referenced issue was not found."
+msgstr ""
+
+msgid "Failed to move this issue because label was not found."
+msgstr ""
+
+msgid "Failed to move this issue because only a single label can be provided."
+msgstr ""
+
+msgid "Failed to move this issue because target project doesn't exist."
+msgstr ""
+
 msgid "Failed to promote label due to internal error. Please contact administrators."
 msgstr ""
 
@@ -6562,13 +6571,13 @@ msgstr ""
 msgid "Locked by %{fileLockUserName}"
 msgstr ""
 
-msgid "Locked the discussion"
+msgid "Locked the discussion."
 msgstr ""
 
 msgid "Locked to current projects"
 msgstr ""
 
-msgid "Locks the discussion"
+msgid "Locks the discussion."
 msgstr ""
 
 msgid "Logo was successfully removed."
@@ -6586,13 +6595,13 @@ msgstr ""
 msgid "MRDiff|Show full file"
 msgstr ""
 
-msgid "Made this issue confidential"
+msgid "Made this issue confidential."
 msgstr ""
 
 msgid "Make and review changes in the browser with the Web IDE"
 msgstr ""
 
-msgid "Make issue confidential."
+msgid "Make issue confidential"
 msgstr ""
 
 msgid "Make sure you save it - you won't be able to access it again."
@@ -6601,7 +6610,7 @@ msgstr ""
 msgid "Make sure you're logged into the account that owns the projects you'd like to import."
 msgstr ""
 
-msgid "Makes this issue confidential"
+msgid "Makes this issue confidential."
 msgstr ""
 
 msgid "Manage"
@@ -6667,10 +6676,10 @@ msgstr ""
 msgid "March"
 msgstr ""
 
-msgid "Mark as done"
+msgid "Mark To Do as done"
 msgstr ""
 
-msgid "Mark as duplicate failed because referenced issue was not found"
+msgid "Mark as done"
 msgstr ""
 
 msgid "Mark as resolved"
@@ -6682,9 +6691,6 @@ msgstr ""
 msgid "Mark this issue as a duplicate of another issue"
 msgstr ""
 
-msgid "Mark to do as done"
-msgstr ""
-
 msgid "Markdown"
 msgstr ""
 
@@ -6697,13 +6703,16 @@ msgstr ""
 msgid "Markdown is supported"
 msgstr ""
 
+msgid "Marked To Do as done."
+msgstr ""
+
 msgid "Marked this %{noun} as Work In Progress."
 msgstr ""
 
 msgid "Marked this issue as a duplicate of %{duplicate_param}."
 msgstr ""
 
-msgid "Marked to do as done."
+msgid "Marks To Do as done."
 msgstr ""
 
 msgid "Marks this %{noun} as Work In Progress."
@@ -6712,9 +6721,6 @@ msgstr ""
 msgid "Marks this issue as a duplicate of %{duplicate_reference}."
 msgstr ""
 
-msgid "Marks to do as done."
-msgstr ""
-
 msgid "Max access level"
 msgstr ""
 
@@ -7081,12 +7087,6 @@ msgstr ""
 msgid "Move selection up"
 msgstr ""
 
-msgid "Move this issue failed because target project doesn't exists"
-msgstr ""
-
-msgid "Move this issue failed because you need to specify only one label."
-msgstr ""
-
 msgid "Move this issue to another project."
 msgstr ""
 
@@ -12188,10 +12188,10 @@ msgstr ""
 msgid "Unlocked"
 msgstr ""
 
-msgid "Unlocked the discussion"
+msgid "Unlocked the discussion."
 msgstr ""
 
-msgid "Unlocks the discussion"
+msgid "Unlocks the discussion."
 msgstr ""
 
 msgid "Unmarked this %{noun} as Work In Progress."

spec/services/quick_actions/interpret_service_spec.rb

@@ -226,7 +226,7 @@ describe QuickActions::InterpretService do
       it 'returns the todo message' do
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq('Added a todo.')
+        expect(message).to eq('Added a To Do.')
       end
     end
 
@@ -242,7 +242,7 @@ describe QuickActions::InterpretService do
         TodoService.new.mark_todo(issuable, developer)
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq('Marked to do as done.')
+        expect(message).to eq('Marked To Do as done.')
       end
     end
 
@@ -453,7 +453,7 @@ describe QuickActions::InterpretService do
       it 'returns the lock discussion message' do
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq('Locked the discussion')
+        expect(message).to eq('Locked the discussion.')
       end
     end
 
@@ -470,7 +470,7 @@ describe QuickActions::InterpretService do
       it 'returns the unlock discussion message' do
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq('Unlocked the discussion')
+        expect(message).to eq('Unlocked the discussion.')
       end
     end
 
@@ -570,7 +570,7 @@ describe QuickActions::InterpretService do
       it 'returns move issue failure message when the referenced issue is not found' do
         _, _, message = service.execute('/move invalid', issue)
 
-        expect(message).to eq("Move this issue failed because target project doesn't exists")
+        expect(message).to eq("Failed to move this issue because target project doesn't exist.")
       end
     end
 
@@ -584,7 +584,7 @@ describe QuickActions::InterpretService do
       it 'returns the confidential message' do
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq('Made this issue confidential')
+        expect(message).to eq('Made this issue confidential.')
       end
     end
 
@@ -783,7 +783,7 @@ describe QuickActions::InterpretService do
       end
     end
 
-    it_behaves_like 'empty command', "Assign command failed because no user was found" do
+    it_behaves_like 'empty command', "Failed to assign a user because no user was found." do
       let(:content) { '/assign @abcd1234' }
       let(:issuable) { issue }
     end
@@ -1217,12 +1217,12 @@ describe QuickActions::InterpretService do
           let(:issuable) { issue }
         end
 
-        it_behaves_like 'empty command', 'Mark as duplicate failed because referenced issue was not found' do
+        it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do
           let(:content) { "/duplicate imaginary#1234" }
           let(:issuable) { issue }
         end
 
-        it_behaves_like 'empty command', 'Mark as duplicate failed because referenced issue was not found' do
+        it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do
           let(:other_project) { create(:project, :private) }
           let(:issue_duplicate) { create(:issue, project: other_project) }
 
@@ -1287,7 +1287,7 @@ describe QuickActions::InterpretService do
         let(:issuable) { issue }
       end
 
-      it_behaves_like 'empty command', 'Mark as duplicate failed because referenced issue was not found' do
+      it_behaves_like 'empty command', 'Failed to mark this issue as a duplicate because referenced issue was not found.' do
         let(:content) { '/duplicate #{issue.to_reference}' }
         let(:issuable) { issue }
       end
@@ -1463,19 +1463,19 @@ describe QuickActions::InterpretService do
       context 'if the given label does not exist' do
         let(:issuable) { issue }
         let(:content) { '/board_move ~"Fake Label"' }
-        it_behaves_like 'empty command', 'Move this issue failed because you need to specify only one label.'
+        it_behaves_like 'empty command', 'Failed to move this issue because label was not found.'
       end
 
       context 'if multiple labels are given' do
         let(:issuable) { issue }
         let(:content) { %{/board_move ~"#{inreview.title}" ~"#{todo.title}"} }
-        it_behaves_like 'empty command', 'Move this issue failed because you need to specify only one label.'
+        it_behaves_like 'empty command', 'Failed to move this issue because only a single label can be provided.'
       end
 
       context 'if the given label is not a list on the board' do
         let(:issuable) { issue }
         let(:content) { %{/board_move ~"#{bug.title}"} }
-        it_behaves_like 'empty command'
+        it_behaves_like 'empty command', 'Failed to move this issue because label was not found.'
       end
 
       context 'if issuable is not an Issue' do
@@ -1552,7 +1552,7 @@ describe QuickActions::InterpretService do
       it 'returns the create_merge_request message' do
         _, _, message = service.execute(content, issuable)
 
-        expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue")
+        expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.")
       end
     end
   end
@@ -1814,13 +1814,13 @@ describe QuickActions::InterpretService do
         it 'uses the default branch name' do
           _, explanations = service.explain(content, issue)
 
-          expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue'])
+          expect(explanations).to eq(['Creates a branch and a merge request to resolve this issue.'])
         end
 
         it 'returns the execution message using the default branch name' do
           _, _, message = service.execute(content, issue)
 
-          expect(message).to eq('Created a branch and a merge request to resolve this issue')
+          expect(message).to eq('Created a branch and a merge request to resolve this issue.')
         end
       end
 
@@ -1830,13 +1830,13 @@ describe QuickActions::InterpretService do
         it 'uses the given branch name' do
           _, explanations = service.explain(content, issue)
 
-          expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue"])
+          expect(explanations).to eq(["Creates branch 'foo' and a merge request to resolve this issue."])
         end
 
         it 'returns the execution message using the given branch name' do
           _, _, message = service.execute(content, issue)
 
-          expect(message).to eq("Created branch 'foo' and a merge request to resolve this issue")
+          expect(message).to eq("Created branch 'foo' and a merge request to resolve this issue.")
         end
       end
     end

spec/support/shared_examples/quick_actions/issue/move_quick_action_shared_examples.rb

@@ -40,7 +40,7 @@ shared_examples 'move quick action' do
 
         wait_for_requests
 
-        expect(page).to have_content "Move this issue failed because target project doesn't exists"
+        expect(page).to have_content "Failed to move this issue because target project doesn't exist."
         expect(issue.reload).to be_open
       end
     end