diff --git a/ee/lib/ee/gitlab/quick_actions/epic_actions.rb b/ee/lib/ee/gitlab/quick_actions/epic_actions.rb index 30e7d5a7dddec2c0aeedb197475a191a83d0ea61..06de0c366533b02dcab0ea8e3f2785e56d848ede 100644 --- a/ee/lib/ee/gitlab/quick_actions/epic_actions.rb +++ b/ee/lib/ee/gitlab/quick_actions/epic_actions.rb @@ -15,12 +15,20 @@ module EpicActions _("Adds %{epic_ref} as child epic.") % { epic_ref: child_epic.to_reference(quick_action_target) } if child_epic end types Epic - condition { action_allowed? } + condition { can_admin_relation? } params '<&epic | group&epic | Epic URL>' command :child_epic do |epic_param| child_epic = extract_epic(epic_param) + child_error = validate_update(quick_action_target, child_epic, :child) - @execution_message[:child_epic] = set_child_epic_update(quick_action_target, child_epic) + @execution_message[:child_epic] = + if child_error.present? + child_error + else + @updates[:quick_action_assign_child_epic] = child_epic + + success_set_message(quick_action_target, child_epic, :child) + end end desc { _('Remove child epic from an epic') } @@ -34,14 +42,15 @@ module EpicActions params '<&epic | group&epic | Epic URL>' command :remove_child_epic do |epic_param| child_epic = extract_epic(epic_param) + child_error = validate_removal(quick_action_target, child_epic, :child) @execution_message[:remove_child_epic] = - if child_epic && quick_action_target.child?(child_epic.id) + if child_error.present? + child_error + else Epics::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.") + success_remove_message(quick_action_target, child_epic, :child) end end @@ -52,12 +61,20 @@ module EpicActions _("Sets %{epic_ref} as parent epic.") % { epic_ref: parent_epic.to_reference(quick_action_target) } if parent_epic end types Epic - condition { action_allowed? } + condition { can_admin_relation? } params '<&epic | group&epic | Epic URL>' command :parent_epic do |epic_param| parent_epic = extract_epic(epic_param) + parent_error = validate_update(quick_action_target, parent_epic, :parent) - @execution_message[:parent_epic] = set_parent_epic_update(quick_action_target, parent_epic) + @execution_message[:parent_epic] = + if parent_error.present? + parent_error + else + @updates[:quick_action_assign_to_parent_epic] = parent_epic + + success_set_message(quick_action_target, parent_epic, :parent) + end end desc { _('Remove parent epic from an epic') } @@ -70,14 +87,15 @@ module EpicActions condition { action_allowed_only_on_update? } command :remove_parent_epic do parent_epic = quick_action_target.parent + parent_error = validate_removal(quick_action_target, parent_epic, :parent) @execution_message[:remove_parent_epic] = - if parent_epic - Epics::EpicLinks::DestroyService.new(quick_action_target, current_user).execute - - _('Removed parent epic %{epic_ref}.') % { epic_ref: parent_epic.to_reference(quick_action_target) } + if parent_error.present? + parent_error else - _("Parent epic is not present.") + Epics::EpicLinks::DestroyService.new(parent_epic, current_user).execute + + success_remove_message(quick_action_target, parent_epic, :parent) end end @@ -89,58 +107,64 @@ def extract_epic(params) extract_references(params, :epic).first end - def action_allowed? - quick_action_target.group&.feature_available?(:subepics) && - current_user.can?(:"admin_#{quick_action_target.to_ability_name}", quick_action_target) + def can_admin_relation?(epic = quick_action_target) + current_user.can?(:admin_epic_tree_relation, epic) end def action_allowed_only_on_update? - quick_action_target.persisted? && action_allowed? + quick_action_target.persisted? && can_admin_relation? end def epics_related?(epic, target_epic) epic.child?(target_epic.id) || target_epic.child?(epic.id) end - def set_child_epic_update(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) - - @updates[:quick_action_assign_child_epic] = child_epic + def validate_update(target_epic, epic, type) + return error_message(:does_not_exist, type) unless epic.present? + return error_message(:already_related, type) if epics_related?(epic, target_epic) - _("Added %{epic_ref} as a child epic.") % { epic_ref: child_epic.to_reference(target_epic) } + error_message(:no_permission, type) unless can_admin_relation?(epic) end - def set_parent_epic_update(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) + def validate_removal(target_epic, epic, type) + return error_message(:not_present, type) unless epic.present? + return error_message(:does_not_exist, type) if type == :child && !target_epic.child?(epic.id) - @updates[:quick_action_assign_to_parent_epic] = parent_epic - - _("Set %{epic_ref} as the parent epic.") % { epic_ref: parent_epic.to_reference(target_epic) } + error_message(:no_permission, type) unless can_admin_relation?(epic) end - def parent_error_message(reason) + def error_message(reason, relation) case reason + when :does_not_exist + _('%{relation_type} epic does not exist.') % { relation_type: relation.to_s.capitalize } when :not_present - _("Parent epic doesn't exist.") + _('%{relation_type} epic is not present.') % { relation_type: relation.to_s.capitalize } when :already_related - _("Given epic is already related to this epic.") + _('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.") + def success_set_message(target_epic, epic, relation) + reference = epic.to_reference(target_epic) + + case relation + when :child + _('Added %{epic_ref} as a child epic.') % { epic_ref: reference } + when :parent + _('Set %{epic_ref} as the parent epic.') % { epic_ref: reference } + end + end + + def success_remove_message(target_epic, epic, relation) + reference = epic.to_reference(target_epic) + + case relation + when :child + _('Removed %{epic_ref} from child epics.') % { epic_ref: reference } + when :parent + _('Removed parent epic %{epic_ref}.') % { epic_ref: reference } end end end diff --git a/ee/spec/services/quick_actions/interpret_service_spec.rb b/ee/spec/services/quick_actions/interpret_service_spec.rb index 28f86bf37f4c4c04e84461ad245e7a59de73c56c..1764c34911cb0ef0b4ee2876f17b9380a65115d8 100644 --- a/ee/spec/services/quick_actions/interpret_service_spec.rb +++ b/ee/spec/services/quick_actions/interpret_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe QuickActions::InterpretService do +RSpec.describe QuickActions::InterpretService, feature_category: :team_planning do let(:current_user) { create(:user) } let(:developer) { create(:user) } let(:developer2) { create(:user) } @@ -691,7 +691,7 @@ let(:referenced_epic) { create(:epic, group: epic.group) } before do - group.add_developer(current_user) + group.add_guest(current_user) stub_licensed_features(epics: true, subepics: true) end @@ -727,8 +727,8 @@ context 'when a user has permissions to add epic relations' do before do - group.add_developer(current_user) - another_group.add_developer(current_user) + group.add_guest(current_user) + another_group.add_guest(current_user) end it_behaves_like 'adds quick action parameter', :quick_action_assign_child_epic, :child_epic @@ -952,7 +952,7 @@ context 'when subepics are disabled' do before do stub_licensed_features(epics: true, subepics: false) - group.add_developer(current_user) + group.add_guest(current_user) end it_behaves_like 'epic relation is not removed' do @@ -1481,12 +1481,14 @@ end context 'epic commands' do + let(:other_group) { create(:group, :private) } let(:epic) { create(:epic, group: group) } - let(:epic2) { create(:epic, group: group) } + let(:epic2) { create(:epic, group: other_group) } before do stub_licensed_features(epics: true, subepics: true) - group.add_developer(current_user) + group.add_guest(current_user) + other_group.add_guest(current_user) end shared_examples 'target epic does not exist' do |relation| @@ -1494,7 +1496,7 @@ _, _, message = service.execute(content, epic) expect(message) - .to eq("#{relation.capitalize} epic doesn't exist.") + .to eq("#{relation.capitalize} epic does not exist.") end end @@ -1508,6 +1510,13 @@ end shared_examples 'without permissions for action' do + before do + allow(current_user).to receive(:can?).with(:use_quick_actions).and_return(true) + allow(current_user).to receive(:can?).with(:admin_all_resources).and_return(true) + allow(current_user).to receive(:can?).with(:admin_epic_tree_relation, epic).and_return(true) + allow(current_user).to receive(:can?).with(:admin_epic_tree_relation, epic2).and_return(false) + end + it 'returns unsuccessful execution message' do _, _, message = service.execute(content, epic) @@ -1545,15 +1554,9 @@ it_behaves_like 'target epic does not exist', :child end - context 'when user has no permission to read epic' do + context 'when user has no permissions to relate the child 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 @@ -1581,7 +1584,7 @@ end context 'when epic reference is wrong' do - let(:content) { "/child_epic qwe" } + let(:content) { "/remove_child_epic qwe" } it 'returns empty explain message' do _, explanations = service.explain(content, epic) @@ -1603,6 +1606,16 @@ .to eq("Child epic does not exist.") end end + + context 'when user has no permissions to remove child epic' do + let(:content) { "/remove_child_epic #{epic2&.to_reference(epic)}" } + + before do + epic2.update!(parent: epic) + end + + it_behaves_like 'without permissions for action' + end end context 'parent_epic command' do @@ -1636,15 +1649,9 @@ it_behaves_like 'target epic does not exist', :parent end - context 'when user has no permission to read epic' do + context 'when user has no permissions to relate the parent 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 @@ -1693,6 +1700,16 @@ .to eq("Parent epic is not present.") end end + + context 'when user has no permissions to remove parent epic' do + let(:content) { "/remove_parent_epic" } + + before do + epic.parent = epic2 + end + + it_behaves_like 'without permissions for action' + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e23a5239d78872d49e61d9f2e5c47e620445e5b5..d5224f8dc06ccf0920510f849e5db833d2f2507d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -953,6 +953,12 @@ msgstr "" msgid "%{ref} cannot be added: %{error}" msgstr "" +msgid "%{relation_type} epic does not exist." +msgstr "" + +msgid "%{relation_type} epic is not present." +msgstr "" + msgid "%{releases} release" msgid_plural "%{releases} releases" msgstr[0] "" @@ -8469,12 +8475,6 @@ msgstr "" msgid "Child epic" msgstr "" -msgid "Child epic does not exist." -msgstr "" - -msgid "Child epic doesn't exist." -msgstr "" - msgid "Child issues and epics" msgstr "" @@ -29743,12 +29743,6 @@ msgstr "" msgid "Parent" msgstr "" -msgid "Parent epic doesn't exist." -msgstr "" - -msgid "Parent epic is not present." -msgstr "" - msgid "Parsing error for param :embed_json. %{message}" msgstr ""