Merge branch '33039_backend_drag_drop_change_parent' into 'master'

Allow epic tree GraphQL mutation to change parent

See merge request gitlab-org/gitlab!29567

doc/api/graphql/reference/gitlab_schema.graphql

@@ -3008,6 +3008,11 @@ input EpicTreeNodeFieldsInputType {
   """
   id: ID!
 
+  """
+  ID of the new parent epic
+  """
+  newParentId: ID
+
   """
   The type of the switch, after or before allowed
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -8716,6 +8716,16 @@
                 }
               },
               "defaultValue": null
+            },
+            {
+              "name": "newParentId",
+              "description": "ID of the new parent epic",
+              "type": {
+                "kind": "SCALAR",
+                "name": "ID",
+                "ofType": null
+              },
+              "defaultValue": null
             }
           ],
           "interfaces": null,

ee/app/graphql/mutations/epic_tree/reorder.rb

@@ -19,7 +19,7 @@ module Mutations
 
       def resolve(args)
         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
         errors = result[:status] == :error ? [result[:message]] : []

ee/app/graphql/types/epic_tree/epic_tree_node_input_type.rb

@@ -21,6 +21,11 @@ module Types
                MoveTypeEnum,
                required: true,
                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
     # rubocop: enable Graphql/AuthorizeTypes
   end

ee/app/services/epics/tree_reorder_service.rb

@@ -12,7 +12,9 @@ module Epics
 
     def execute
       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?
 
       move!
@@ -21,6 +23,24 @@ module Epics
 
     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!
       moving_object.move_between(before_object, after_object)
       moving_object.save!(touch: false)
@@ -46,15 +66,22 @@ module Epics
       end
 
       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
 
     def valid_relative_position?
       %w(before after).include?(params[:relative_position])
     end
 
-    def same_parent?
-      moving_object.parent == adjacent_reference.parent
+    def different_epic_parent?
+      if new_parent
+        new_parent != adjacent_reference.parent
+      else
+        moving_object.parent != adjacent_reference.parent
+      end
     end
 
     def supported_type?(object)
@@ -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, 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
     end
 
@@ -87,6 +119,12 @@ module Epics
       @adjacent_reference ||= find_object(params[:adjacent_reference_id])&.sync
     end
 
+    def new_parent
+      return unless params[:new_parent_id]
+
+      @new_parent ||= find_object(params[:new_parent_id])&.sync
+    end
+
     def find_object(id)
       GitlabSchema.object_from_id(id)
     end

ee/changelogs/unreleased/33039_backend_drag_drop_change_parent.yml

+---
+title: Allow changing item parent in epic tree via GraphQL
+merge_request: 29567
+author:
+type: added

ee/spec/requests/api/graphql/mutations/epic_tree/reorder_spec.rb

@@ -21,13 +21,15 @@ describe 'Updating an epic tree' do
   end
 
   let(:relative_position) { :after }
+  let(:new_parent_id) { nil }
   let(:variables) do
     {
       base_epic_id: GitlabSchema.id_from_object(base_epic).to_s,
       moved: {
         id: GitlabSchema.id_from_object(epic2).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
@@ -80,6 +82,28 @@ describe 'Updating an epic tree' do
 
             expect(mutation_response['array']).to be_nil
           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
 
         context 'when relative_position is invalid' do
@@ -106,7 +130,7 @@ describe 'Updating an epic tree' do
           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) }
 
           it_behaves_like 'a mutation that does not update the tree'
@@ -114,7 +138,7 @@ describe 'Updating an epic tree' do
           it 'returns the error message' do
             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
@@ -136,9 +160,31 @@ describe 'Updating an epic tree' do
 
           expect(mutation_response['array']).to be_nil
         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
 
-      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) }
 
         before do
@@ -151,7 +197,7 @@ describe 'Updating an epic tree' do
         it 'returns the error message' do
           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

ee/spec/services/epics/tree_reorder_service_spec.rb

@@ -20,11 +20,13 @@ describe Epics::TreeReorderService do
     let!(:tree_object_2) { epic2 }
     let(:adjacent_reference_id) { GitlabSchema.id_from_object(tree_object_1) }
     let(:moving_object_id) { GitlabSchema.id_from_object(tree_object_2) }
+    let(:new_parent_id) { nil }
     let(:params) do
       {
         base_epic_id: GitlabSchema.id_from_object(epic),
         adjacent_reference_id: adjacent_reference_id,
-        relative_position: relative_position
+        relative_position: relative_position,
+        new_parent_id: new_parent_id
       }
     end
 
@@ -32,8 +34,12 @@ describe Epics::TreeReorderService do
 
     shared_examples 'error for the tree update' do |expected_error|
       it 'does not change relative_positions' do
-        expect { subject }.not_to change { tree_object_1.relative_position }
-        expect { subject }.not_to change { tree_object_2.relative_position }
+        expect { subject }.not_to change { tree_object_1.reload.relative_position }
+        expect { subject }.not_to change { tree_object_2.reload.relative_position }
+      end
+
+      it 'does not change parent' do
+        expect { subject }.not_to change { tree_object_2.reload.parent }
       end
 
       it 'returns error status' do
@@ -58,7 +64,7 @@ describe Epics::TreeReorderService do
         it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
       end
 
-      context 'when user does has permissions to admin the base epic' do
+      context 'when user does have permission to admin the base epic' do
         before do
           group.add_developer(user)
         end
@@ -93,13 +99,32 @@ describe Epics::TreeReorderService do
             end
           end
 
-          context 'when object being moved is from of another epic' do
+          context 'when object being moved is from another epic' do
             before do
               other_epic = create(:epic, group: group)
               epic_issue2.update(epic: other_epic)
             end
 
-            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
+            context 'when the new_parent_id has not been provided' do
+              it_behaves_like 'error for the tree update', "The sibling object's parent must match the current parent epic."
+            end
+
+            context 'when the new_parent_id does not match the parent of the relative positioning object' do
+              let(:unrelated_epic) { create(:epic, group: group) }
+              let(:new_parent_id) { GitlabSchema.id_from_object(unrelated_epic) }
+
+              it_behaves_like 'error for the tree update', "The sibling object's parent must match the new parent epic."
+            end
+
+            context 'when the new_parent_id matches the parent id of the relative positioning object' do
+              let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
+              it 'reorders the objects' do
+                subject
+
+                expect(epic2.reload.relative_position).to be > tree_object_2.reload.relative_position
+              end
+            end
           end
 
           context 'when object being moved is not supported type' do
@@ -114,6 +139,26 @@ describe Epics::TreeReorderService do
             it_behaves_like 'error for the tree update', 'Only epics and epic_issues are supported.'
           end
 
+          context 'when user does not have permissions to admin the previous parent' do
+            let(:other_group) { create(:group) }
+            let(:other_epic) { create(:epic, group: other_group) }
+            let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
+            before do
+              epic_issue2.update(parent: other_epic)
+            end
+
+            it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+          end
+
+          context 'when user does not have permissions to admin the new parent' do
+            let(:other_group) { create(:group) }
+            let(:other_epic) { create(:epic, group: other_group) }
+            let(:new_parent_id) { GitlabSchema.id_from_object(other_epic) }
+
+            it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+          end
+
           context 'when the epics of reordered epic-issue links are not subepics of the base epic' do
             let(:another_group) { create(:group) }
             let(:another_epic) { create(:epic, group: another_group) }
@@ -123,7 +168,15 @@ describe Epics::TreeReorderService do
               epic_issue2.update(epic: another_epic)
             end
 
-            it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+            context 'when new_parent_id is not provided' do
+              it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+            end
+
+            context 'when new_parent_id is provided' do
+              let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
+              it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+            end
           end
 
           context 'when moving is successful' do
@@ -132,6 +185,24 @@ describe Epics::TreeReorderService do
 
               expect(tree_object_1.reload.relative_position).to be > tree_object_2.reload.relative_position
             end
+
+            context 'when a new_parent_id of a valid parent is provided' do
+              let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
+              before do
+                epic_issue2.update(epic: epic1)
+              end
+
+              it 'updates the parent' do
+                expect { subject }.to change { tree_object_2.reload.epic }.from(epic1).to(epic)
+              end
+
+              it 'updates the links relative positions' do
+                subject
+
+                expect(tree_object_1.reload.relative_position).to be > tree_object_2.reload.relative_position
+              end
+            end
           end
         end
 
@@ -139,25 +210,49 @@ describe Epics::TreeReorderService do
           let!(:tree_object_1) { epic1 }
           let!(:tree_object_2) { epic2 }
 
-          context 'when the reordered epics are not subepics of the base epic' do
-            let(:another_group) { create(:group) }
-            let(:another_epic) { create(:epic, group: another_group) }
+          context 'when user does not have permissions to admin the previous parent' do
+            let(:other_group) { create(:group) }
+            let(:other_epic) { create(:epic, group: other_group) }
+            let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
 
             before do
-              epic1.update(group: another_group, parent: another_epic)
-              epic2.update(group: another_group, parent: another_epic)
+              epic2.update(parent: other_epic)
             end
 
             it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
           end
 
-          context 'when object being moved is froms another epic' do
+          context 'when there is some other error with the new parent' do
+            let(:other_group) { create(:group) }
+            let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
             before do
-              other_epic = create(:epic, group: group)
-              epic2.update(parent: other_epic)
+              other_group.add_developer(user)
+              epic.update(group: other_group)
+              epic2.update(parent: epic1)
             end
 
-            it_behaves_like 'error for the tree update', 'Both objects have to belong to the same parent epic.'
+            it_behaves_like 'error for the tree update', "This epic can't be added because it must belong to the same group as the parent, or subgroup of the parent epic’s group"
+          end
+
+          context 'when user does not have permissions to admin the new parent' do
+            let(:other_group) { create(:group) }
+            let(:other_epic) { create(:epic, group: other_group) }
+            let(:new_parent_id) { GitlabSchema.id_from_object(other_epic) }
+
+            it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
+          end
+
+          context 'when the reordered epics are not subepics of the base epic' do
+            let(:another_group) { create(:group) }
+            let(:another_epic) { create(:epic, group: another_group) }
+
+            before do
+              epic1.update(group: another_group, parent: another_epic)
+              epic2.update(group: another_group, parent: another_epic)
+            end
+
+            it_behaves_like 'error for the tree update', 'You don\'t have permissions to move the objects.'
           end
 
           context 'when moving is successful' do
@@ -166,6 +261,37 @@ describe Epics::TreeReorderService do
 
               expect(tree_object_1.reload.relative_position).to be > tree_object_2.reload.relative_position
             end
+
+            context 'when new parent is current epic' do
+              let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+
+              it 'updates the relative positions' do
+                subject
+
+                expect(tree_object_1.reload.relative_position).to be > tree_object_2.reload.relative_position
+              end
+
+              it 'does not update the parent_id' do
+                expect { subject }.not_to change { tree_object_2.reload.parent }
+              end
+            end
+
+            context 'when object being moved is from another epic and new_parent_id matches parent of adjacent object' do
+              let(:other_epic) { create(:epic, group: group) }
+              let(:new_parent_id) { GitlabSchema.id_from_object(epic) }
+              let(:epic3) { create(:epic, parent: other_epic, group: group) }
+              let(:tree_object_2) { epic3 }
+
+              it 'updates the relative positions and parent_id' do
+                subject
+
+                expect(tree_object_1.reload.relative_position).to be > tree_object_2.reload.relative_position
+              end
+
+              it 'updates the parent' do
+                expect { subject }.to change { tree_object_2.reload.parent }.from(other_epic).to(epic)
+              end
+            end
           end
         end
       end