Merge branch '233440-refactor-move-services' into 'master'

GraphQL mutation for moving epics between lists

See merge request gitlab-org/gitlab!55467

app/models/label.rb

@@ -131,6 +131,10 @@ class Label < ApplicationRecord
     nil
   end
 
+  def self.ids_on_board(board_id)
+    on_board(board_id).pluck(:label_id)
+  end
+
   # Searches for labels with a matching title or description.
   #
   # This method uses ILIKE on PostgreSQL.

app/services/boards/base_item_move_service.rb

+# frozen_string_literal: true
+
+module Boards
+  class BaseItemMoveService < Boards::BaseService
+    def execute(issuable)
+      issuable_modification_params = issuable_params(issuable)
+      return false if issuable_modification_params.empty?
+
+      move_single_issuable(issuable, issuable_modification_params)
+    end
+
+    private
+
+    def issuable_params(issuable)
+      attrs = {}
+
+      if move_between_lists?
+        attrs.merge!(
+          add_label_ids: add_label_ids,
+          remove_label_ids: remove_label_ids,
+          state_event: issuable_state
+        )
+      end
+
+      attrs
+    end
+
+    def move_single_issuable(issuable, issuable_modification_params)
+      ability_name = :"admin_#{issuable.to_ability_name}"
+      return unless can?(current_user, ability_name, issuable)
+
+      update(issuable, issuable_modification_params)
+    end
+
+    def move_between_lists?
+      moving_from_list.present? && moving_to_list.present? &&
+        moving_from_list != moving_to_list
+    end
+
+    def moving_from_list
+      return unless params[:from_list_id].present?
+
+      @moving_from_list ||= board.lists.id_in(params[:from_list_id]).first
+    end
+
+    def moving_to_list
+      return unless params[:to_list_id].present?
+
+      @moving_to_list ||= board.lists.id_in(params[:to_list_id]).first
+    end
+
+    def issuable_state
+      return 'reopen' if moving_from_list.closed?
+      return 'close'  if moving_to_list.closed?
+    end
+
+    def add_label_ids
+      [moving_to_list.label_id].compact
+    end
+
+    def remove_label_ids
+      label_ids =
+        if moving_to_list.movable?
+          moving_from_list.label_id
+        else
+          ::Label.ids_on_board(board.id)
+        end
+
+      Array(label_ids).compact
+    end
+  end
+end

app/services/boards/issues/move_service.rb

@@ -2,13 +2,8 @@
 
 module Boards
   module Issues
-    class MoveService < Boards::BaseService
-      def execute(issue)
-        issue_modification_params = issue_params(issue)
-        return false if issue_modification_params.empty?
-
-        move_single_issue(issue, issue_modification_params)
-      end
+    class MoveService < Boards::BaseItemMoveService
+      extend ::Gitlab::Utils::Override
 
       def execute_multiple(issues)
         return execute_multiple_empty_result if issues.empty?
@@ -16,7 +11,7 @@ module Boards
         handled_issues = []
         last_inserted_issue_id = nil
         count = issues.each.inject(0) do |moved_count, issue|
-          issue_modification_params = issue_params(issue)
+          issue_modification_params = issuable_params(issue)
           next moved_count if issue_modification_params.empty?
 
           if last_inserted_issue_id
@@ -24,7 +19,7 @@ module Boards
           end
 
           last_inserted_issue_id = issue.id
-          handled_issue = move_single_issue(issue, issue_modification_params)
+          handled_issue = move_single_issuable(issue, issue_modification_params)
           handled_issues << present_issue_entity(handled_issue) if handled_issue
           handled_issue && handled_issue.valid? ? moved_count + 1 : moved_count
         end
@@ -54,51 +49,17 @@ module Boards
         move_between_ids({ move_after_id: nil, move_before_id: id })
       end
 
-      def move_single_issue(issue, issue_modification_params)
-        return unless can?(current_user, :update_issue, issue)
-
-        update(issue, issue_modification_params)
-      end
-
       def board
         @board ||= parent.boards.find(params[:board_id])
       end
 
-      def move_between_lists?
-        moving_from_list.present? && moving_to_list.present? &&
-          moving_from_list != moving_to_list
-      end
-
-      # 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
-
       def update(issue, issue_modification_params)
         ::Issues::UpdateService.new(issue.project, current_user, issue_modification_params).execute(issue)
       end
 
-      def issue_params(issue)
-        attrs = {}
-
-        if move_between_lists?
-          attrs.merge!(
-            add_label_ids: add_label_ids,
-            remove_label_ids: remove_label_ids,
-            state_event: issue_state
-          )
-        end
+      override :issuable_params
+      def issuable_params(issuable)
+        attrs = super
 
         move_between_ids = move_between_ids(params)
         if move_between_ids
@@ -109,28 +70,6 @@ module Boards
         attrs
       end
 
-      def issue_state
-        return 'reopen' if moving_from_list.closed?
-        return 'close'  if moving_to_list.closed?
-      end
-
-      def add_label_ids
-        [moving_to_list.label_id].compact
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def remove_label_ids
-        label_ids =
-          if moving_to_list.movable?
-            moving_from_list.label_id
-          else
-            ::Label.on_board(board.id).pluck(:label_id)
-          end
-
-        Array(label_ids).compact
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
       def move_between_ids(move_params)
         ids = [move_params[:move_after_id], move_params[:move_before_id]]
                 .map(&:to_i)

doc/api/graphql/reference/index.md

@@ -2063,6 +2063,15 @@ Represents an epic board list.
 | `position` | Int | Position of the list within the board. |
 | `title` | String! | Title of the list. |
 
+### `EpicMoveListPayload`
+
+Autogenerated return type of EpicMoveList.
+
+| Field | Type | Description |
+| ----- | ---- | ----------- |
+| `clientMutationId` | String | A unique identifier for the client performing the mutation. |
+| `errors` | String! => Array | Errors encountered during execution of the mutation. |
+
 ### `EpicPermissions`
 
 Check permissions for the current user on an epic.

ee/app/graphql/ee/types/mutation_type.rb

@@ -39,6 +39,7 @@ module EE
         mount_mutation ::Mutations::Boards::UpdateEpicUserPreferences
         mount_mutation ::Mutations::Boards::EpicBoards::Create
         mount_mutation ::Mutations::Boards::EpicBoards::Update
+        mount_mutation ::Mutations::Boards::EpicBoards::EpicMoveList
         mount_mutation ::Mutations::Boards::EpicLists::Create
         mount_mutation ::Mutations::Boards::Lists::UpdateLimitMetrics
         mount_mutation ::Mutations::InstanceSecurityDashboard::AddProject

ee/app/graphql/mutations/boards/epic_boards/epic_move_list.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Boards
+    module EpicBoards
+      class EpicMoveList < ::Mutations::BaseMutation
+        graphql_name 'EpicMoveList'
+
+        authorize :admin_epic_board
+
+        argument :board_id, ::Types::GlobalIDType[::Boards::EpicBoard],
+                  required: true,
+                  description: 'Global ID of the board that the epic is in.'
+
+        argument :epic_id, ::Types::GlobalIDType[::Epic],
+                  required: true,
+                  description: 'ID of the epic to mutate.'
+
+        argument :from_list_id, ::Types::GlobalIDType[::Boards::EpicList],
+                  required: true,
+                  description: 'ID of the board list that the epic will be moved from.'
+
+        argument :to_list_id, ::Types::GlobalIDType[::Boards::EpicList],
+                  required: true,
+                  description: 'ID of the board list that the epic will be moved to.'
+
+        def resolve(**args)
+          board = authorized_find!(id: args[:board_id])
+          epic = authorized_find!(id: args[:epic_id])
+
+          unless Feature.enabled?(:epic_boards, board.resource_parent)
+            raise Gitlab::Graphql::Errors::ResourceNotAvailable, 'epic_boards feature is disabled'
+          end
+
+          move_epic(board, epic, move_list_arguments(args).merge(board_id: board.id))
+
+          {
+            epic: epic.reset,
+            errors: epic.errors.full_messages
+          }
+        end
+
+        private
+
+        def find_object(id:)
+          GitlabSchema.find_by_gid(id)
+        end
+
+        def move_epic(board, epic, move_params)
+          service = ::Boards::Epics::MoveService.new(board.resource_parent, current_user, move_params)
+
+          service.execute(epic)
+        end
+
+        def move_list_arguments(args)
+          {
+            from_list_id: args[:from_list_id].find&.id,
+            to_list_id: args[:to_list_id].find&.id
+          }
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Boards
+  module Epics
+    class MoveService < Boards::BaseItemMoveService
+      private
+
+      def update(epic, epic_modification_params)
+        ::Epics::UpdateService.new(epic.group, current_user, epic_modification_params).execute(epic)
+      end
+
+      def board
+        @board ||= parent.epic_boards.find(params[:board_id])
+      end
+    end
+  end
+end

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

@@ -6,8 +6,8 @@ module EE
       module MoveService
         extend ::Gitlab::Utils::Override
 
-        override :issue_params
-        def issue_params(issue)
+        override :issuable_params
+        def issuable_params(issue)
           args = super
           args[:epic_id] = params[:epic_id] if params.has_key?(:epic_id)
 

ee/changelogs/unreleased/233440-refactor-move-services.yml

+---
+title: Add mutation to move epics between lists
+merge_request: 55467
+author:
+type: added

ee/spec/graphql/mutations/boards/epic_boards/epic_move_list_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Mutations::Boards::EpicBoards::EpicMoveList do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:development) { create(:group_label, group: group, name: 'Development') }
+  let(:epic) { create(:epic, group: group) }
+
+  let_it_be(:board) { create(:epic_board, group: group) }
+  let_it_be(:backlog) { create(:epic_list, epic_board: board, list_type: :backlog) }
+  let_it_be(:labeled_list) { create(:epic_list, epic_board: board, label: development) }
+
+  let(:mutation) { described_class.new(object: nil, context: { current_user: current_user }, field: nil) }
+  let(:params) do
+    {
+      board_id: board.to_global_id,
+      epic_id: epic.to_global_id,
+      from_list_id: backlog.to_global_id,
+      to_list_id: labeled_list.to_global_id
+    }
+  end
+
+  subject { mutation.resolve(**params) }
+
+  context 'arguments' do
+    subject { described_class }
+
+    it { is_expected.to have_graphql_arguments(:boardId, :epicId, :fromListId, :toListId) }
+  end
+
+  describe '#resolve' do
+    context 'when epic_boards are disabled' do
+      before do
+        stub_licensed_features(epics: true)
+        stub_feature_flags(epic_boards: false)
+        group.add_developer(current_user)
+      end
+
+      it 'raises resource not available error' do
+        expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+      end
+    end
+
+    context 'when epic_boards are enabled' do
+      before do
+        stub_licensed_features(epics: true)
+        stub_feature_flags(epic_boards: true)
+      end
+
+      context 'when user does not have permissions to admin the board' do
+        it 'raises resource not available error' do
+          expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+        end
+      end
+
+      context 'when user has permissions to admin the board' do
+        before do
+          group.add_developer(current_user)
+        end
+
+        it 'moves the epic to another list' do
+          expect { subject }.to change { epic.reload.labels }.from([]).to([development])
+        end
+      end
+    end
+  end
+end

ee/spec/services/boards/epics/move_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Boards::Epics::MoveService do
+  describe '#execute' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:board) { create(:epic_board, group: group) }
+    let_it_be(:other_board) { create(:epic_board, group: group) }
+
+    let_it_be(:development) { create(:group_label, group: group, name: 'Development') }
+    let_it_be(:testing) { create(:group_label, group: group, name: 'Testing') }
+
+    let_it_be(:backlog) { create(:epic_list, epic_board: board, list_type: :backlog, label: nil) }
+    let_it_be(:list1) { create(:epic_list, epic_board: board, label: development, position: 0) }
+    let_it_be(:list2) { create(:epic_list, epic_board: board, label: testing, position: 1) }
+    let_it_be(:closed) { create(:epic_list, epic_board: board, list_type: :closed, label: nil) }
+    let_it_be(:other_board_list) { create(:epic_list, epic_board: other_board, list_type: :closed, label: nil) }
+
+    let(:epic) { create(:epic, group: group) }
+
+    let(:params) { { board_id: board.id, from_list_id: from_list.id, to_list_id: to_list.id } }
+    let(:from_list) { backlog }
+    let(:to_list) { closed }
+
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    subject { described_class.new(group, user, params).execute(epic) }
+
+    context 'when user does not have permissions to move an epic' do
+      it 'does not close the epic' do
+        expect { subject }.not_to change { epic.state }
+      end
+    end
+
+    context 'when user has permissions to move an epic' do
+      before do
+        group.add_maintainer(user)
+      end
+
+      context 'when moving the epic from backlog' do
+        context 'to a labeled list' do
+          let(:to_list) { list1 }
+
+          it 'keeps the epic opened and adds the labels' do
+            expect { subject }.not_to change { epic.state }
+
+            expect(epic.labels).to eq([development])
+          end
+        end
+
+        context 'to the closed list' do
+          it 'closes the epic' do
+            expect { subject }.to change { epic.state }.from('opened').to('closed')
+          end
+        end
+
+        context 'to the closed list in another board' do
+          let(:to_list) { other_board_list }
+
+          it 'does not close the epic' do
+            expect { subject }.not_to change { epic.state }
+          end
+        end
+      end
+
+      context 'when moving the epic from a labeled list' do
+        before do
+          epic.labels = [development]
+        end
+
+        let(:from_list) { list1 }
+
+        context 'to another labeled list' do
+          let(:to_list) { list2 }
+
+          it 'changes the labels' do
+            expect { subject }.to change { epic.reload.labels }.from([development]).to([testing])
+          end
+        end
+
+        context 'to the closed list' do
+          let(:to_list) { closed }
+
+          it 'closes the epic' do
+            expect { subject }.to change { epic.state }.from('opened').to('closed')
+          end
+        end
+      end
+    end
+  end
+end