Commit e00b76dc authored by charlieablett's avatar charlieablett

Allow epic tree mutation to change parent

- Add auth checks for old parent as well as new
- Add ability to modify parent
- Modify tests
parent c731db80
...@@ -3008,6 +3008,11 @@ input EpicTreeNodeFieldsInputType { ...@@ -3008,6 +3008,11 @@ input EpicTreeNodeFieldsInputType {
""" """
id: ID! id: ID!
"""
ID of the new parent epic
"""
newParentId: ID
""" """
The type of the switch, after or before allowed The type of the switch, after or before allowed
""" """
......
...@@ -8716,6 +8716,16 @@ ...@@ -8716,6 +8716,16 @@
} }
}, },
"defaultValue": null "defaultValue": null
},
{
"name": "newParentId",
"description": "ID of the new parent epic",
"type": {
"kind": "SCALAR",
"name": "ID",
"ofType": null
},
"defaultValue": null
} }
], ],
"interfaces": null, "interfaces": null,
......
...@@ -19,7 +19,7 @@ module Mutations ...@@ -19,7 +19,7 @@ module Mutations
def resolve(args) def resolve(args)
params = args[:moved] params = args[:moved]
moving_params = params.to_hash.slice(:adjacent_reference_id, :relative_position).merge(base_epic_id: args[:base_epic_id]) moving_params = params.to_hash.slice(:adjacent_reference_id, :relative_position, :new_parent_id).merge(base_epic_id: args[:base_epic_id])
result = ::Epics::TreeReorderService.new(current_user, params[:id], moving_params).execute result = ::Epics::TreeReorderService.new(current_user, params[:id], moving_params).execute
errors = result[:status] == :error ? [result[:message]] : [] errors = result[:status] == :error ? [result[:message]] : []
......
...@@ -21,6 +21,11 @@ module Types ...@@ -21,6 +21,11 @@ module Types
MoveTypeEnum, MoveTypeEnum,
required: true, required: true,
description: 'The type of the switch, after or before allowed' description: 'The type of the switch, after or before allowed'
argument :new_parent_id,
GraphQL::ID_TYPE,
required: false,
description: 'ID of the new parent epic'
end end
# rubocop: enable Graphql/AuthorizeTypes # rubocop: enable Graphql/AuthorizeTypes
end end
......
...@@ -12,7 +12,9 @@ module Epics ...@@ -12,7 +12,9 @@ module Epics
def execute def execute
error_message = validate_objects error_message = validate_objects
return error(error_message) if error_message.present?
error_message = set_new_parent
return error(error_message) if error_message.present? return error(error_message) if error_message.present?
move! move!
...@@ -21,6 +23,24 @@ module Epics ...@@ -21,6 +23,24 @@ module Epics
private private
def set_new_parent
return unless new_parent && new_parent_different?
moving_object.parent = new_parent
validate_new_parent
end
def new_parent_different?
params[:new_parent_id] != GitlabSchema.id_from_object(moving_object.parent)
end
def validate_new_parent
return unless moving_object.respond_to?(:valid_parent?)
return if moving_object.valid_parent?
moving_object.errors[:parent]&.first
end
def move! def move!
moving_object.move_between(before_object, after_object) moving_object.move_between(before_object, after_object)
moving_object.save!(touch: false) moving_object.save!(touch: false)
...@@ -46,15 +66,22 @@ module Epics ...@@ -46,15 +66,22 @@ module Epics
end end
return 'You don\'t have permissions to move the objects.' unless authorized? return 'You don\'t have permissions to move the objects.' unless authorized?
return 'Both objects have to belong to the same parent epic.' unless same_parent?
if different_epic_parent?
return "The sibling object's parent must match the #{new_parent ? "new" : "current"} parent epic."
end
end end
def valid_relative_position? def valid_relative_position?
%w(before after).include?(params[:relative_position]) %w(before after).include?(params[:relative_position])
end end
def same_parent? def different_epic_parent?
moving_object.parent == adjacent_reference.parent if new_parent
new_parent != adjacent_reference.parent
else
moving_object.parent != adjacent_reference.parent
end
end end
def supported_type?(object) def supported_type?(object)
...@@ -65,6 +92,11 @@ module Epics ...@@ -65,6 +92,11 @@ module Epics
return false unless can?(current_user, :admin_epic, base_epic.group) return false unless can?(current_user, :admin_epic, base_epic.group)
return false unless can?(current_user, :admin_epic, adjacent_reference_group) return false unless can?(current_user, :admin_epic, adjacent_reference_group)
if new_parent
return false unless can?(current_user, :admin_epic, new_parent.group)
return false unless moving_object.parent && can?(current_user, :admin_epic, moving_object.parent.group)
end
true true
end end
...@@ -87,6 +119,12 @@ module Epics ...@@ -87,6 +119,12 @@ module Epics
@adjacent_reference ||= find_object(params[:adjacent_reference_id])&.sync @adjacent_reference ||= find_object(params[:adjacent_reference_id])&.sync
end end
def new_parent
return unless params[:new_parent_id]
@new_parent ||= find_object(params[:new_parent_id])&.sync
end
def find_object(id) def find_object(id)
GitlabSchema.object_from_id(id) GitlabSchema.object_from_id(id)
end end
......
---
title: Allow changing item parent in epic tree via GraphQL
merge_request: 29567
author:
type: added
...@@ -21,13 +21,15 @@ describe 'Updating an epic tree' do ...@@ -21,13 +21,15 @@ describe 'Updating an epic tree' do
end end
let(:relative_position) { :after } let(:relative_position) { :after }
let(:new_parent_id) { nil }
let(:variables) do let(:variables) do
{ {
base_epic_id: GitlabSchema.id_from_object(base_epic).to_s, base_epic_id: GitlabSchema.id_from_object(base_epic).to_s,
moved: { moved: {
id: GitlabSchema.id_from_object(epic2).to_s, id: GitlabSchema.id_from_object(epic2).to_s,
adjacent_reference_id: GitlabSchema.id_from_object(epic1).to_s, adjacent_reference_id: GitlabSchema.id_from_object(epic1).to_s,
relative_position: relative_position relative_position: relative_position,
new_parent_id: new_parent_id
} }
} }
end end
...@@ -80,6 +82,28 @@ describe 'Updating an epic tree' do ...@@ -80,6 +82,28 @@ describe 'Updating an epic tree' do
expect(mutation_response['array']).to be_nil expect(mutation_response['array']).to be_nil
end end
context 'when a new_parent_id is provided' do
let(:new_parent_id) { GitlabSchema.id_from_object(base_epic).to_s }
before do
other_epic = create(:epic, group: group)
epic2.update(parent: other_epic)
end
it 'updates the epics relative positions and updates the parent' do
post_graphql_mutation(mutation, current_user: current_user)
expect(epic1.reload.relative_position).to be > epic2.reload.relative_position
expect(epic2.parent).to eq base_epic
end
it 'returns nil in errors' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['array']).to be_nil
end
end
end end
context 'when relative_position is invalid' do context 'when relative_position is invalid' do
...@@ -106,7 +130,7 @@ describe 'Updating an epic tree' do ...@@ -106,7 +130,7 @@ describe 'Updating an epic tree' do
end end
end end
context 'when moving an epic fails due to another parent' do context 'when moving an epic fails due to the parents of the relative position object and the moving object mismatching' do
let(:epic2) { create(:epic, relative_position: 20) } let(:epic2) { create(:epic, relative_position: 20) }
it_behaves_like 'a mutation that does not update the tree' it_behaves_like 'a mutation that does not update the tree'
...@@ -114,7 +138,7 @@ describe 'Updating an epic tree' do ...@@ -114,7 +138,7 @@ describe 'Updating an epic tree' do
it 'returns the error message' do it 'returns the error message' do
post_graphql_mutation(mutation, current_user: current_user) post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.']) expect(mutation_response['errors']).to eq(["The sibling object's parent must match the current parent epic."])
end end
end end
end end
...@@ -136,9 +160,31 @@ describe 'Updating an epic tree' do ...@@ -136,9 +160,31 @@ describe 'Updating an epic tree' do
expect(mutation_response['array']).to be_nil expect(mutation_response['array']).to be_nil
end end
context 'when a new_parent_id is provided' do
let(:new_parent_id) { GitlabSchema.id_from_object(base_epic).to_s }
before do
other_epic = create(:epic, group: group)
epic_issue2.update(epic: other_epic)
end
it "updates the epic's relative positions and parent" do
post_graphql_mutation(mutation, current_user: current_user)
expect(epic_issue1.reload.relative_position).to be > epic_issue2.reload.relative_position
expect(epic_issue2.parent).to eq base_epic
end
it 'returns nil in errors' do
post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['array']).to be_nil
end
end
end end
context 'when moving an issue fails due to another parent' do context 'when moving an issue fails due to the parents of the relative position object and the moving object mismatching' do
let(:epic_issue2) { create(:epic_issue, relative_position: 20) } let(:epic_issue2) { create(:epic_issue, relative_position: 20) }
before do before do
...@@ -151,7 +197,7 @@ describe 'Updating an epic tree' do ...@@ -151,7 +197,7 @@ describe 'Updating an epic tree' do
it 'returns the error message' do it 'returns the error message' do
post_graphql_mutation(mutation, current_user: current_user) post_graphql_mutation(mutation, current_user: current_user)
expect(mutation_response['errors']).to eq(['Both objects have to belong to the same parent epic.']) expect(mutation_response['errors']).to eq(["The sibling object's parent must match the current parent epic."])
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment