Merge branch 'move_to_swimlane' into 'master'

Support moving issue between epics

See merge request gitlab-org/gitlab!41790

app/graphql/mutations/boards/issues/issue_move_list.rb

@@ -50,6 +50,8 @@ module Mutations
         end
 
         def resolve(board:, **args)
+          Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/247861')
+
           raise_resource_not_available_error! unless board
           authorize_board!(board)
 
@@ -89,3 +91,5 @@ module Mutations
     end
   end
 end
+
+Mutations::Boards::Issues::IssueMoveList.prepend_if_ee('EE::Mutations::Boards::Issues::IssueMoveList')

app/services/boards/issues/move_service.rb

@@ -71,12 +71,16 @@ module Boards
 
       # rubocop: disable CodeReuse/ActiveRecord
       def moving_from_list
+        return unless params[:from_list_id].present?
+
         @moving_from_list ||= board.lists.find_by(id: params[:from_list_id])
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       # rubocop: disable CodeReuse/ActiveRecord
       def moving_to_list
+        return unless params[:to_list_id].present?
+
         @moving_to_list ||= board.lists.find_by(id: params[:to_list_id])
       end
       # rubocop: enable CodeReuse/ActiveRecord

doc/api/graphql/reference/gitlab_schema.graphql

@@ -5677,6 +5677,11 @@ type EpicHealthStatus {
   issuesOnTrack: Int
 }
 
+"""
+Identifier of Epic
+"""
+scalar EpicID
+
 """
 Relationship between an epic and an issue
 """
@@ -8096,6 +8101,11 @@ input IssueMoveListInput {
   """
   clientMutationId: String
 
+  """
+  The ID of the parent epic. NULL when removing the association
+  """
+  epicId: EpicID
+
   """
   ID of the board list that the issue will be moved from
   """

doc/api/graphql/reference/gitlab_schema.json

@@ -15890,6 +15890,16 @@
           "enumValues": null,
           "possibleTypes": null
         },
+        {
+          "kind": "SCALAR",
+          "name": "EpicID",
+          "description": "Identifier of Epic",
+          "fields": null,
+          "inputFields": null,
+          "interfaces": null,
+          "enumValues": null,
+          "possibleTypes": null
+        },
         {
           "kind": "OBJECT",
           "name": "EpicIssue",
@@ -22411,6 +22421,16 @@
               },
               "defaultValue": null
             },
+            {
+              "name": "epicId",
+              "description": "The ID of the parent epic. NULL when removing the association",
+              "type": {
+                "kind": "SCALAR",
+                "name": "EpicID",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
             {
               "name": "clientMutationId",
               "description": "A unique identifier for the client performing the mutation.",

ee/app/graphql/ee/mutations/boards/issues/issue_move_list.rb

+# frozen_string_literal: true
+
+module EE
+  module Mutations
+    module Boards
+      module Issues
+        module IssueMoveList
+          extend ActiveSupport::Concern
+          extend ::Gitlab::Utils::Override
+
+          prepended do
+            argument :epic_id, ::Types::GlobalIDType[::Epic],
+                      required: false,
+                      description: 'The ID of the parent epic. NULL when removing the association'
+          end
+
+          override :move_issue
+          def move_issue(board, issue, move_params)
+            super
+          rescue ::Issues::BaseService::EpicAssignmentError => e
+            issue.errors.add(:epic_issue, e.message)
+          # because we can't be sure if these exceptions were raised because of epic
+          # we return just a generic error here for now
+          # https://gitlab.com/gitlab-org/gitlab/-/issues/247096
+          rescue ::Gitlab::Access::AccessDeniedError, ActiveRecord::RecordNotFound
+            issue.errors.add(:base, 'Resource not found')
+          end
+
+          override :move_arguments
+          def move_arguments(args)
+            allowed_args = super
+            allowed_args[:epic_id] = args[:epic_id]&.model_id if args.has_key?(:epic_id)
+
+            allowed_args
+          end
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/boards/issues/move_service.rb

@@ -8,9 +8,10 @@ module EE
 
         override :issue_params
         def issue_params(issue)
-          return super unless move_between_lists?
-
           args = super
+          args[:epic_id] = params[:epic_id] if params.has_key?(:epic_id)
+
+          return args unless move_between_lists?
 
           unless both_are_same_type? || !moving_to_list.movable?
             args.delete(:remove_label_ids)

ee/app/services/ee/issuable_links/create_service.rb

@@ -16,7 +16,9 @@ module EE
 
         super
 
-        Epics::UpdateDatesService.new(affected_epics).execute unless affected_epics.blank?
+        if !params[:skip_epic_dates_update] && affected_epics.present?
+          Epics::UpdateDatesService.new(affected_epics).execute
+        end
       end
 
       def affected_epics(_issues)

ee/app/services/ee/issues/base_service.rb

@@ -5,45 +5,46 @@ module EE
     module BaseService
       extend ::Gitlab::Utils::Override
 
-      def filter_params(issue)
-        set_epic_param(issue)
-        super
-      end
+      class EpicAssignmentError < ::ArgumentError; end
 
       def handle_epic(issue)
-        set_epic_param(issue)
+        return unless epic_param_present?
 
-        return unless params.key?(:epic)
+        epic = epic_param(issue)
+        result = epic ? assign_epic(issue, epic) : unassign_epic(issue)
+        issue.reload_epic
 
-        if epic_param
-          EpicIssues::CreateService.new(epic_param, current_user, { target_issuable: issue }).execute
-        else
-          link = EpicIssue.find_by_issue_id(issue.id)
+        if result[:status] == :error
+          raise EpicAssignmentError, result[:message]
+        end
+      end
 
-          return unless link
+      def assign_epic(issue, epic)
+        issue.confidential = true if !issue.persisted? && epic.confidential
 
-          EpicIssues::DestroyService.new(link, current_user).execute
-        end
+        link_params = { target_issuable: issue, skip_epic_dates_update: true }
 
-        params.delete(:epic)
+        EpicIssues::CreateService.new(epic, current_user, link_params).execute
       end
 
-      def set_epic_param(issue)
-        return unless epic_param_present?
+      def unassign_epic(issue)
+        link = EpicIssue.find_by_issue_id(issue.id)
+        return success unless link
 
+        EpicIssues::DestroyService.new(link, current_user).execute
+      end
+
+      def epic_param(issue)
         epic_id = params.delete(:epic_id)
-        epic = epic_param || find_epic(issue, epic_id)
+        epic = params.delete(:epic) || find_epic(issue, epic_id)
 
-        unless epic
-          params[:epic] = nil
-          return
-        end
+        return unless epic
 
         unless can?(current_user, :admin_epic, epic)
           raise ::Gitlab::Access::AccessDeniedError
         end
 
-        params[:epic] = epic
+        epic
       end
 
       def find_epic(issue, epic_id)
@@ -56,10 +57,6 @@ module EE
                         include_ancestor_groups: true).find(epic_id)
       end
 
-      def epic_param
-        params[:epic]
-      end
-
       def epic_param_present?
         params.key?(:epic) || params.key?(:epic_id)
       end

ee/app/services/ee/issues/create_service.rb

@@ -5,17 +5,20 @@ module EE
     module CreateService
       extend ::Gitlab::Utils::Override
 
-      override :before_create
-      def before_create(issue)
+      override :filter_params
+      def filter_params(issue)
         handle_epic(issue)
 
         super
       end
 
-      def handle_epic(issue)
-        issue.confidential = true if epic_param&.confidential
-
-        super
+      override :execute
+      def execute(skip_system_notes: false)
+        super.tap do |issue|
+          if issue.previous_changes.include?(:milestone_id) && issue.epic_issue
+            ::Epics::UpdateDatesService.new([issue.epic_issue.epic]).execute
+          end
+        end
       end
     end
   end

ee/app/services/ee/issues/update_service.rb

@@ -6,9 +6,15 @@ module EE
       extend ::Gitlab::Utils::Override
       include ::Gitlab::Utils::StrongMemoize
 
+      override :filter_params
+      def filter_params(issue)
+        handle_epic(issue)
+
+        super
+      end
+
       override :execute
       def execute(issue)
-        handle_epic(issue)
         handle_promotion(issue)
 
         result = super

ee/changelogs/unreleased/move_to_swimlane.yml

+---
+title: Support moving issue between epics in GraphQL
+merge_request: 41790
+author:
+type: added

ee/spec/graphql/ee/mutations/boards/issues/issue_move_list_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Mutations::Boards::Issues::IssueMoveList do
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:board) { create(:board, group: group) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:issue1) { create(:labeled_issue, project: project, relative_position: 3) }
+  let_it_be(:existing_issue1) { create(:labeled_issue, project: project, relative_position: 10) }
+  let_it_be(:existing_issue2) { create(:labeled_issue, project: project, relative_position: 50) }
+
+  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+  let(:params) { { board: board, project_path: project.full_path, iid: issue1.iid } }
+  let(:move_params) do
+    {
+      epic_id: epic.to_global_id,
+      move_before_id: existing_issue2.id,
+      move_after_id: existing_issue1.id
+    }
+  end
+
+  before do
+    stub_licensed_features(epics: true)
+    project.add_maintainer(user)
+  end
+
+  subject do
+    mutation.resolve(params.merge(move_params))
+  end
+
+  describe '#resolve' do
+    context 'when user has access to the epic' do
+      before do
+        group.add_developer(user)
+      end
+
+      it 'moves and repositions issue' do
+        subject
+
+        expect(issue1.reload.epic).to eq(epic)
+        expect(issue1.relative_position).to be < existing_issue2.relative_position
+        expect(issue1.relative_position).to be > existing_issue1.relative_position
+      end
+    end
+
+    context 'when user does not have access to the epic' do
+      it 'does not update issue' do
+        subject
+
+        expect(issue1.reload.epic).to be_nil
+        expect(issue1.relative_position).to eq(3)
+      end
+    end
+  end
+end

ee/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Reposition and move issue within board lists' do
+  include GraphqlHelpers
+
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:board) { create(:board, group: group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let_it_be(:existing_issue1) { create(:labeled_issue, project: project, relative_position: 10) }
+  let_it_be(:existing_issue2) { create(:labeled_issue, project: project, relative_position: 50) }
+  let_it_be(:issue1) { create(:labeled_issue, project: project, relative_position: 3) }
+  let_it_be(:development) { create(:label, project: project, name: 'Development') }
+  let_it_be(:testing) { create(:label, project: project, name: 'Testing') }
+  let_it_be(:list1)   { create(:list, board: board, label: development, position: 0) }
+  let_it_be(:list2)   { create(:list, board: board, label: testing, position: 1) }
+
+  let(:mutation_class) { Mutations::Boards::Issues::IssueMoveList }
+  let(:mutation_name) { mutation_class.graphql_name }
+  let(:params) { { board_id: board.to_global_id.to_s, project_path: project.full_path, iid: issue1.iid.to_s } }
+  let(:issue_move_params) do
+    {
+      epic_id: epic.to_global_id.to_s,
+      move_before_id: existing_issue2.id,
+      move_after_id: existing_issue1.id,
+      from_list_id: list1.id,
+      to_list_id: list2.id
+    }
+  end
+
+  before do
+    stub_licensed_features(epics: true)
+    project.add_maintainer(user)
+  end
+
+  context 'when user has access to the epic' do
+    before do
+      group.add_maintainer(user)
+    end
+
+    it 'updates issue position and epic' do
+      post_graphql_mutation(mutation(params), current_user: user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      response_issue = graphql_mutation_response(:issue_move_list)['issue']
+      expect(response_issue['iid']).to eq(issue1.iid.to_s)
+      expect(response_issue['relativePosition']).to be > existing_issue1.relative_position
+      expect(response_issue['relativePosition']).to be < existing_issue2.relative_position
+      expect(response_issue['epic']['id']).to eq(epic.to_global_id.to_s)
+      expect(response_issue['labels']['nodes'].first['title']).to eq(testing.title)
+    end
+
+    context 'when user sets nil epic' do
+      let_it_be(:epic_issue) { create(:epic_issue, issue: issue1, epic: epic) }
+      let(:issue_move_params) do
+        {
+          epic_id: nil,
+          move_before_id: existing_issue2.id,
+          move_after_id: existing_issue1.id
+        }
+      end
+
+      it 'updates issue position and epic is unassigned' do
+        post_graphql_mutation(mutation(params), current_user: user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        response_issue = graphql_mutation_response(:issue_move_list)['issue']
+        expect(response_issue['iid']).to eq(issue1.iid.to_s)
+        expect(response_issue['relativePosition']).to be > existing_issue1.relative_position
+        expect(response_issue['relativePosition']).to be < existing_issue2.relative_position
+        expect(response_issue['epic']).to be_nil
+      end
+    end
+  end
+
+  context 'when user can not admin epic' do
+    it 'fails with error' do
+      post_graphql_mutation(mutation(params), current_user: user)
+
+      mutation_response = graphql_mutation_response(:issue_move_list)
+      expect(mutation_response['errors']).to eq(['Resource not found'])
+      expect(mutation_response['issue']['epic']).to eq(nil)
+      expect(mutation_response['issue']['relativePosition']).to eq(3)
+    end
+  end
+
+  def mutation(additional_params = {})
+    graphql_mutation(mutation_name, issue_move_params.merge(additional_params),
+                     <<-QL.strip_heredoc
+                       clientMutationId
+                       issue {
+                         iid,
+                         relativePosition
+                         epic {
+                           id
+                         }
+                         labels {
+                           nodes {
+                             title
+                           }
+                         }
+                       }
+                       errors
+                     QL
+    )
+  end
+end

ee/spec/services/ee/issues/create_service_spec.rb

@@ -60,7 +60,7 @@ RSpec.describe Issues::CreateService do
               issue = service.execute
 
               expect(issue).to be_persisted
-              expect(issue.epic).to eq(epic)
+              expect(issue.reload.epic).to eq(epic)
               expect(issue.confidential).to eq(false)
             end
           end
@@ -78,7 +78,7 @@ RSpec.describe Issues::CreateService do
               issue = service.execute
 
               expect(issue.milestone).to eq(milestone)
-              expect(issue.epic).to eq(epic)
+              expect(issue.reload.epic).to eq(epic)
               expect(epic.reload.start_date).to eq(milestone.start_date)
               expect(epic.due_date).to eq(milestone.due_date)
             end

ee/spec/services/ee/issues/update_service_spec.rb

@@ -211,9 +211,10 @@ RSpec.describe Issues::UpdateService do
 
           it 'calls EpicIssues::CreateService' do
             link_sevice = double
-            expect(EpicIssues::CreateService).to receive(:new).with(epic, user, { target_issuable: issue })
+            expect(EpicIssues::CreateService).to receive(:new)
+              .with(epic, user, { target_issuable: issue, skip_epic_dates_update: true })
               .and_return(link_sevice)
-            expect(link_sevice).to receive(:execute)
+            expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
             subject
           end
@@ -232,9 +233,10 @@ RSpec.describe Issues::UpdateService do
 
           it 'calls EpicIssues::CreateService' do
             link_sevice = double
-            expect(EpicIssues::CreateService).to receive(:new).with(epic, user, { target_issuable: issue })
+            expect(EpicIssues::CreateService).to receive(:new)
+              .with(epic, user, { target_issuable: issue, skip_epic_dates_update: true })
               .and_return(link_sevice)
-            expect(link_sevice).to receive(:execute)
+            expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
             subject
           end
@@ -273,7 +275,7 @@ RSpec.describe Issues::UpdateService do
             link_sevice = double
             expect(EpicIssues::DestroyService).to receive(:new).with(EpicIssue.last, user)
               .and_return(link_sevice)
-            expect(link_sevice).to receive(:execute)
+            expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
             subject
           end
@@ -311,7 +313,7 @@ RSpec.describe Issues::UpdateService do
         link_sevice = double
         expect(EpicIssues::DestroyService).to receive(:new).with(epic_issue, user)
           .and_return(link_sevice)
-        expect(link_sevice).to receive(:execute)
+        expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
         subject
       end

ee/spec/support/shared_examples/services/issue_epic_shared_examples.rb

@@ -55,7 +55,7 @@ RSpec.shared_examples 'issue with epic_id parameter' do
       it 'calls EpicIssues::CreateService' do
         link_sevice = double
         expect(EpicIssues::CreateService).to receive(:new).and_return(link_sevice)
-        expect(link_sevice).to receive(:execute)
+        expect(link_sevice).to receive(:execute).and_return({ status: :success })
 
         execute
       end