Add missing error handling for epic quick actions

Include error messages for epic quick actions when
epics don't exist, are already related, or user
has no permission to read them

ee/changelogs/unreleased/13809-missing-error-handling-for-epic-quick-action.yml

+---
+title: Add missing error handling for epic quick actions
+merge_request: 15648
+author:
+type: fixed

ee/lib/ee/gitlab/quick_actions/epic_actions.rb

@@ -20,17 +20,14 @@ module EE
           command :child_epic do |epic_param|
             child_epic = extract_epic(epic_param)
 
-            if child_epic && !quick_action_target.child?(child_epic.id)
-              EpicLinks::CreateService.new(quick_action_target, current_user, { target_issuable: child_epic }).execute
-              @execution_message[:child_epic] = _("Added %{epic_ref} as child epic.") % { epic_ref: child_epic.to_reference(quick_action_target) }
-            end
+            @execution_message[:child_epic] = add_child_epic(quick_action_target, child_epic)
           end
 
           desc _('Remove child epic from an epic')
           explanation do |epic_param|
             child_epic = extract_epic(epic_param)
 
-            _("Removes %{epic_ref} from child epics.") % { epic_ref: child_epic.to_reference(quick_action_target) }
+            _("Removes %{epic_ref} from child epics.") % { epic_ref: child_epic.to_reference(quick_action_target) } if child_epic
           end
           types Epic
           condition { action_allowed? }
@@ -38,9 +35,13 @@ module EE
           command :remove_child_epic do |epic_param|
             child_epic = extract_epic(epic_param)
 
+            @execution_message[:remove_child_epic] =
               if child_epic && quick_action_target.child?(child_epic.id)
                 EpicLinks::DestroyService.new(child_epic, current_user).execute
-              @execution_message[:remove_child_epic] = _("Removed %{epic_ref} from child epics.") % { epic_ref: child_epic.to_reference(quick_action_target) }
+
+                _("Removed %{epic_ref} from child epics.") % { epic_ref: child_epic.to_reference(quick_action_target) }
+              else
+                _("Child epic does not exist.")
               end
           end
 
@@ -56,26 +57,28 @@ module EE
           command :parent_epic do |epic_param|
             parent_epic = extract_epic(epic_param)
 
-            if parent_epic && !parent_epic.child?(quick_action_target.id)
-              EpicLinks::CreateService.new(parent_epic, current_user, { target_issuable: quick_action_target }).execute
-              @execution_message[:parent_epic] = _("Set %{epic_ref} as parent epic.") % { epic_ref: parent_epic.to_reference(quick_action_target) }
-            end
+            @execution_message[:parent_epic] = set_parent_epic(quick_action_target, parent_epic)
           end
 
           desc _('Remove parent epic from an epic')
           explanation do
             parent_epic = quick_action_target.parent
 
-            _('Removes parent epic %{epic_ref}.') % { epic_ref: parent_epic.to_reference(quick_action_target) }
+            _('Removes parent epic %{epic_ref}.') % { epic_ref: parent_epic.to_reference(quick_action_target) } if parent_epic
           end
           types Epic
-          condition do
-            action_allowed? && quick_action_target.parent.present?
-          end
+          condition { action_allowed? }
           command :remove_parent_epic do
             parent_epic = quick_action_target.parent
+
+            @execution_message[:remove_parent_epic] =
+              if parent_epic
                 EpicLinks::DestroyService.new(quick_action_target, current_user).execute
-            @execution_message[:remove_parent_epic] = _('Removed parent epic %{epic_ref}.') % { epic_ref: parent_epic.to_reference(quick_action_target) }
+
+                _('Removed parent epic %{epic_ref}.') % { epic_ref: parent_epic.to_reference(quick_action_target) }
+              else
+                _("Parent epic is not present.")
+              end
           end
 
           private
@@ -90,6 +93,52 @@ module EE
             quick_action_target.group&.feature_available?(:epics) &&
               current_user.can?(:"admin_#{quick_action_target.to_ability_name}", quick_action_target)
           end
+
+          def epics_related?(epic, target_epic)
+            epic.child?(target_epic.id) || target_epic.child?(epic.id)
+          end
+
+          def add_child_epic(target_epic, child_epic)
+            return child_error_message(:not_present) unless child_epic.present?
+            return child_error_message(:already_related) if epics_related?(child_epic, target_epic)
+            return child_error_message(:no_permission) unless current_user.can?(:read_epic, child_epic)
+
+            EpicLinks::CreateService.new(target_epic, current_user, { target_issuable: child_epic }).execute
+
+            _("Added %{epic_ref} as a child epic.") % { epic_ref: child_epic.to_reference(target_epic) }
+          end
+
+          def set_parent_epic(target_epic, parent_epic)
+            return parent_error_message(:not_present) unless parent_epic.present?
+            return parent_error_message(:already_related) if epics_related?(parent_epic, target_epic)
+            return parent_error_message(:no_permission) unless current_user.can?(:read_epic, parent_epic)
+
+            EpicLinks::CreateService.new(parent_epic, current_user, { target_issuable: target_epic }).execute
+
+            _("Set %{epic_ref} as the parent epic.") % { epic_ref: parent_epic.to_reference(target_epic) }
+          end
+
+          def parent_error_message(reason)
+            case reason
+            when :not_present
+              _("Parent epic doesn't exist.")
+            when :already_related
+              _("Given epic is already related to this epic.")
+            when :no_permission
+              _("You don't have sufficient permission to perform this action.")
+            end
+          end
+
+          def child_error_message(reason)
+            case reason
+            when :not_present
+              _("Child epic doesn't exist.")
+            when :already_related
+              _("Given epic is already related to this epic.")
+            when :no_permission
+              _("You don't have sufficient permission to perform this action.")
+            end
+          end
         end
       end
     end

ee/lib/ee/gitlab/quick_actions/issue_actions.rb

@@ -10,7 +10,6 @@ module EE
         included do
           desc _('Add to epic')
           explanation _('Adds an issue to an epic.')
-          execution_message _('Added an issue to an epic.')
           types Issue
           condition do
             quick_action_target.project.group&.feature_available?(:epics) &&
@@ -18,7 +17,16 @@ module EE
           end
           params '<&epic | group&epic | Epic URL>'
           command :epic do |epic_param|
-            @updates[:epic] = extract_epic(epic_param)
+            epic = extract_epic(epic_param)
+
+            if epic && current_user.can?(:read_epic, epic)
+              @updates[:epic] = epic
+              message = _('Added an issue to an epic.')
+            else
+              message = _("This epic does not exist or you don't have sufficient permission.")
+            end
+
+            @execution_message[:epic] = message
           end
 
           desc _('Remove from epic')

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -219,10 +219,12 @@ describe QuickActions::InterpretService do
           stub_licensed_features(epics: true)
         end
 
+        context 'when epic exists' do
           it 'assigns an issue to an epic' do
-          _, updates = service.execute(content, issue)
+            _, updates, message = service.execute(content, issue)
 
             expect(updates).to eq(epic: epic)
+            expect(message).to eq('Added an issue to an epic.')
           end
 
           context 'when an issue belongs to a project without group' do
@@ -241,6 +243,35 @@ describe QuickActions::InterpretService do
           end
         end
 
+        context 'when epic does not exist' do
+          let(:content) { "/epic none" }
+
+          it 'does not assign an issue to an epic' do
+            _, updates, message = service.execute(content, issue)
+
+            expect(updates).to be_empty
+            expect(message).to eq("This epic does not exist or you don't have sufficient permission.")
+          end
+        end
+
+        context 'when user has no permissions to read epic' do
+          let(:content) { "/epic #{epic.to_reference(project)}" }
+
+          before do
+            allow(current_user).to receive(:can?).with(:use_quick_actions).and_return(true)
+            allow(current_user).to receive(:can?).with(:admin_issue, issue).and_return(true)
+            allow(current_user).to receive(:can?).with(:read_epic, epic).and_return(false)
+          end
+
+          it 'does not assign an issue to an epic' do
+            _, updates, message = service.execute(content, issue)
+
+            expect(updates).to be_empty
+            expect(message).to eq("This epic does not exist or you don't have sufficient permission.")
+          end
+        end
+      end
+
       context 'when epics are disabled' do
         it 'does not recognize /epic' do
           _, updates = service.execute(content, issue)
@@ -925,12 +956,21 @@ describe QuickActions::InterpretService do
       shared_examples 'adds epic relation' do |relation|
         context 'when correct epic reference' do
           let(:content) { "/#{relation}_epic #{epic2&.to_reference(epic)}" }
-          let(:action) { relation == :child ? 'Adds' : 'Sets'}
+          let(:explain_action) { relation == :child ? 'Adds' : 'Sets'}
+          let(:execute_action) { relation == :child ? 'Added' : 'Set'}
+          let(:article)        { relation == :child ? 'a' : 'the'}
 
-          it 'returns message with epic reference' do
+          it 'returns explain message with epic reference' do
             _, explanations = service.explain(content, epic)
             expect(explanations)
-              .to eq(["#{action} #{epic2.group.name}&#{epic2.iid} as #{relation} epic."])
+              .to eq(["#{explain_action} #{epic2.group.name}&#{epic2.iid} as #{relation} epic."])
+          end
+
+          it 'returns successful execution message' do
+            _, _, message = service.execute(content, epic)
+
+            expect(message)
+              .to eq("#{execute_action} #{epic2.group.name}&#{epic2.iid} as #{article} #{relation} epic.")
           end
         end
 
@@ -944,18 +984,95 @@ describe QuickActions::InterpretService do
         end
       end
 
+      shared_examples 'target epic does not exist' do |relation|
+        it 'returns unsuccessful execution message' do
+          _, _, message = service.execute(content, epic)
+
+          expect(message)
+            .to eq("#{relation.capitalize} epic doesn't exist.")
+        end
+      end
+
+      shared_examples 'epics are already related' do
+        it 'returns unsuccessful execution message' do
+          _, _, message = service.execute(content, epic)
+
+          expect(message)
+            .to eq("Given epic is already related to this epic.")
+        end
+      end
+
+      shared_examples 'without permissions for action' do
+        it 'returns unsuccessful execution message' do
+          _, _, message = service.execute(content, epic)
+
+          expect(message)
+            .to eq("You don't have sufficient permission to perform this action.")
+        end
+      end
+
       context 'child_epic command' do
         it_behaves_like 'adds epic relation', :child
+
+        context 'when epic is already a child epic' do
+          let(:content) { "/child_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            epic2.update!(parent: epic)
+          end
+
+          it_behaves_like 'epics are already related'
+        end
+
+        context 'when epic is the parent epic' do
+          let(:content) { "/child_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            epic.update!(parent: epic2)
+          end
+
+          it_behaves_like 'epics are already related'
+        end
+
+        context 'when epic does not exist' do
+          let(:content) { "/child_epic none" }
+
+          it_behaves_like 'target epic does not exist', :child
+        end
+
+        context 'when user has no permission to read epic' do
+          let(:content) { "/child_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            allow(current_user).to receive(:can?).with(:use_quick_actions).and_return(true)
+            allow(current_user).to receive(:can?).with(:admin_epic, epic).and_return(true)
+            allow(current_user).to receive(:can?).with(:read_epic, epic2).and_return(false)
+          end
+
+          it_behaves_like 'without permissions for action'
+        end
       end
 
       context 'remove_child_epic command' do
         context 'when correct epic reference' do
           let(:content) { "/remove_child_epic #{epic2&.to_reference(epic)}" }
 
-          it 'returns message with epic reference' do
+          before do
+            epic2.update!(parent: epic)
+          end
+
+          it 'returns explain message with epic reference' do
             _, explanations = service.explain(content, epic)
+
             expect(explanations).to eq(["Removes #{epic2.group.name}&#{epic2.iid} from child epics."])
           end
+
+          it 'returns successful execution message' do
+            _, _, message = service.execute(content, epic)
+
+            expect(message)
+              .to eq("Removed #{epic2.group.name}&#{epic2.iid} from child epics.")
+          end
         end
 
         context 'when epic reference is wrong' do
@@ -966,10 +1083,63 @@ describe QuickActions::InterpretService do
             expect(explanations).to eq([])
           end
         end
+
+        context 'when child epic does not exist' do
+          let(:content) { "/remove_child_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            epic.update!(parent: nil)
+          end
+
+          it 'returns unsuccessful execution message' do
+            _, _, message = service.execute(content, epic)
+
+            expect(message)
+              .to eq("Child epic does not exist.")
+          end
+        end
       end
 
       context 'parent_epic command' do
         it_behaves_like 'adds epic relation', :parent
+
+        context 'when epic is already a parent epic' do
+          let(:content) { "/parent_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            epic.update!(parent: epic2)
+          end
+
+          it_behaves_like 'epics are already related'
+        end
+
+        context 'when epic is a an existing child epic' do
+          let(:content) { "/parent_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            epic2.update!(parent: epic)
+          end
+
+          it_behaves_like 'epics are already related'
+        end
+
+        context 'when epic does not exist' do
+          let(:content) { "/parent_epic none" }
+
+          it_behaves_like 'target epic does not exist', :parent
+        end
+
+        context 'when user has no permission to read epic' do
+          let(:content) { "/parent_epic #{epic2&.to_reference(epic)}" }
+
+          before do
+            allow(current_user).to receive(:can?).with(:use_quick_actions).and_return(true)
+            allow(current_user).to receive(:can?).with(:admin_epic, epic).and_return(true)
+            allow(current_user).to receive(:can?).with(:read_epic, epic2).and_return(false)
+          end
+
+          it_behaves_like 'without permissions for action'
+        end
       end
 
       context 'remove_parent_epic command' do
@@ -978,10 +1148,18 @@ describe QuickActions::InterpretService do
             epic.parent = epic2
           end
 
-          it 'returns message with epic reference' do
+          it 'returns explain message with epic reference' do
             _, explanations = service.explain("/remove_parent_epic", epic)
+
             expect(explanations).to eq(["Removes parent epic #{epic2.group.name}&#{epic2.iid}."])
           end
+
+          it 'returns successful execution message' do
+            _, _, message = service.execute("/remove_parent_epic", epic)
+
+            expect(message)
+              .to eq("Removed parent epic #{epic2.group.name}&#{epic2.iid}.")
+          end
         end
 
         context 'when parent is not present' do
@@ -991,8 +1169,16 @@ describe QuickActions::InterpretService do
 
           it 'returns empty explain message' do
             _, explanations = service.explain("/remove_parent_epic", epic)
+
             expect(explanations).to eq([])
           end
+
+          it 'returns unsuccessful execution message' do
+            _, _, message = service.execute("/remove_parent_epic", epic)
+
+            expect(message)
+              .to eq("Parent epic is not present.")
+          end
         end
       end
     end

locale/gitlab.pot

@@ -985,7 +985,7 @@ msgstr ""
 msgid "Added"
 msgstr ""
 
-msgid "Added %{epic_ref} as child epic."
+msgid "Added %{epic_ref} as a child epic."
 msgstr ""
 
 msgid "Added %{label_references} %{label_text}."
@@ -2934,6 +2934,12 @@ msgstr ""
 msgid "Cherry-pick this merge request"
 msgstr ""
 
+msgid "Child epic does not exist."
+msgstr ""
+
+msgid "Child epic doesn't exist."
+msgstr ""
+
 msgid "Choose <strong>Create archive</strong> and wait for archiving to complete."
 msgstr ""
 
@@ -7652,6 +7658,9 @@ msgstr ""
 msgid "Given access %{time_ago}"
 msgstr ""
 
+msgid "Given epic is already related to this epic."
+msgstr ""
+
 msgid "Global Shortcuts"
 msgstr ""
 
@@ -11087,6 +11096,12 @@ msgstr ""
 msgid "Parameter"
 msgstr ""
 
+msgid "Parent epic doesn't exist."
+msgstr ""
+
+msgid "Parent epic is not present."
+msgstr ""
+
 msgid "Part of merge request changes"
 msgstr ""
 
@@ -14243,7 +14258,7 @@ msgstr ""
 msgid "Session expiration, projects limit and attachment size."
 msgstr ""
 
-msgid "Set %{epic_ref} as parent epic."
+msgid "Set %{epic_ref} as the parent epic."
 msgstr ""
 
 msgid "Set a default template for issue descriptions."
@@ -16074,6 +16089,9 @@ msgstr ""
 msgid "This environment has no deployments yet."
 msgstr ""
 
+msgid "This epic does not exist or you don't have sufficient permission."
+msgstr ""
+
 msgid "This feature requires local storage to be enabled"
 msgstr ""
 
@@ -18282,6 +18300,9 @@ msgstr ""
 msgid "You don't have any recent searches"
 msgstr ""
 
+msgid "You don't have sufficient permission to perform this action."
+msgstr ""
+
 msgid "You don’t have access to Cycle Analytics for this group"
 msgstr ""