Merge branch '13809-missing-error-handling-for-epic-quick-action' into 'master'

Missing error handling for /epic quick action

Closes #13809

See merge request gitlab-org/gitlab!15648

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,10 +35,14 @@ module EE
           command :remove_child_epic do |epic_param|
             child_epic = extract_epic(epic_param)
 
-            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) }
-            end
+            @execution_message[:remove_child_epic] =
+              if child_epic && quick_action_target.child?(child_epic.id)
+                EpicLinks::DestroyService.new(child_epic, current_user).execute
+
+                _("Removed %{epic_ref} from child epics.") % { epic_ref: child_epic.to_reference(quick_action_target) }
+              else
+                _("Child epic does not exist.")
+              end
           end
 
           desc _('Set parent epic to an epic')
@@ -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
-            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) }
+
+            @execution_message[:remove_parent_epic] =
+              if parent_epic
+                EpicLinks::DestroyService.new(quick_action_target, current_user).execute
+
+                _('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,24 +219,55 @@ describe QuickActions::InterpretService do
           stub_licensed_features(epics: true)
         end
 
-        it 'assigns an issue to an epic' do
-          _, updates = service.execute(content, issue)
+        context 'when epic exists' do
+          it 'assigns an issue to an epic' do
+            _, 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
+            let(:user_project) { create(:project) }
+            let(:issue)        { create(:issue, project: user_project) }
+
+            before do
+              user_project.add_developer(user)
+            end
 
-          expect(updates).to eq(epic: epic)
+            it 'does not assign an issue to an epic' do
+              _, updates = service.execute(content, issue)
+
+              expect(updates).to be_empty
+            end
+          end
         end
 
-        context 'when an issue belongs to a project without group' do
-          let(:user_project) { create(:project) }
-          let(:issue)        { create(:issue, project: user_project) }
+        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
-            user_project.add_developer(user)
+            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 = service.execute(content, issue)
+            _, 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
@@ -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 ""
 
@@ -7700,6 +7706,9 @@ msgstr ""
 msgid "Given access %{time_ago}"
 msgstr ""
 
+msgid "Given epic is already related to this epic."
+msgstr ""
+
 msgid "Global Shortcuts"
 msgstr ""
 
@@ -11141,6 +11150,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 ""
 
@@ -14306,7 +14321,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."
@@ -16146,6 +16161,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 ""
 
@@ -18354,6 +18372,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 ""