Merge branch '324283-cablett-destroy-epic-board-list' into 'master'

Destroy epic board list

See merge request gitlab-org/gitlab!58719

app/services/boards/lists/base_destroy_service.rb

+# frozen_string_literal: true
+
+module Boards
+  module Lists
+    # This class is used by issue and epic board lists
+    # for destroying a single list
+    class BaseDestroyService < Boards::BaseService
+      def execute(list)
+        unless list.destroyable?
+          return ServiceResponse.error(message: "Open and closed lists on a board cannot be destroyed.")
+        end
+
+        list.with_lock do
+          decrement_higher_lists(list)
+          list.destroy!
+        end
+
+        ServiceResponse.success
+      rescue StandardError => e
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
+        ServiceResponse.error(message: "List destroy failed.")
+      end
+
+      private
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def decrement_higher_lists(list)
+        list.board.lists.movable.where('position > ?', list.position)
+            .update_all('position = position - 1')
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+    end
+  end
+end

app/services/boards/lists/destroy_service.rb

@@ -2,36 +2,8 @@
 
 module Boards
   module Lists
-    class DestroyService < Boards::BaseService
-      def execute(list)
-        unless list.destroyable?
-          return ServiceResponse.error(message: "The list cannot be destroyed. Only label lists can be destroyed.")
-        end
-
-        @board = list.board
-
-        list.with_lock do
-          decrement_higher_lists(list)
-          remove_list(list)
-        end
-
-        ServiceResponse.success
-      end
-
-      private
-
-      attr_reader :board
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def decrement_higher_lists(list)
-        board.lists.movable.where('position > ?', list.position)
-                   .update_all('position = position - 1')
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def remove_list(list)
-        list.destroy!
-      end
+    # overridden in EE for board lists and also for epic board lists.
+    class DestroyService < Boards::Lists::BaseDestroyService
     end
   end
 end

doc/api/graphql/reference/index.md

@@ -2006,6 +2006,27 @@ Input type: `EpicBoardListCreateInput`
 | <a id="mutationepicboardlistcreateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
 | <a id="mutationepicboardlistcreatelist"></a>`list` | [`EpicList`](#epiclist) | Epic list in the epic board. |
 
+### `Mutation.epicBoardListDestroy`
+
+Destroys an epic board list.
+
+Input type: `EpicBoardListDestroyInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationepicboardlistdestroyclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationepicboardlistdestroylistid"></a>`listId` | [`BoardsEpicListID!`](#boardsepiclistid) | Global ID of the epic board list to destroy. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationepicboardlistdestroyclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationepicboardlistdestroyerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+| <a id="mutationepicboardlistdestroylist"></a>`list` | [`EpicList`](#epiclist) | The epic board list. `null` if the board was destroyed successfully. |
+
 ### `Mutation.epicBoardUpdate`
 
 Input type: `EpicBoardUpdateInput`

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

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

ee/app/graphql/mutations/boards/epic_lists/destroy.rb

+# frozen_string_literal: true
+
+module Mutations
+  module Boards
+    module EpicLists
+      class Destroy < ::Mutations::BaseMutation
+        graphql_name 'EpicBoardListDestroy'
+        description 'Destroys an epic board list.'
+
+        argument :list_id, ::Types::GlobalIDType[::Boards::EpicList],
+                 required: true,
+                 loads: Types::Boards::EpicListType,
+                 description: 'Global ID of the epic board list to destroy.'
+
+        field :list,
+              Types::Boards::EpicListType,
+              null: true,
+              description: 'The epic board list. `null` if the board was destroyed successfully.'
+
+        authorize :admin_epic_board_list
+
+        def resolve(list:)
+          raise_resource_not_available_error! unless can_admin_list?(list)
+
+          # authorisation is handled by the service in order to return consistent responses
+          # and so the service is essentially the authorisation SSOT.
+          response = ::Boards::EpicLists::DestroyService.new(list.board.resource_parent, current_user).execute(list)
+
+          list_result = response.success? ? nil : list
+          mutation_response(list_result, response.errors)
+        end
+
+        def mutation_response(list_object, errors)
+          {
+              list: list_object,
+              errors: errors
+          }
+        end
+
+        private
+
+        def can_admin_list?(list)
+          Ability.allowed?(current_user, :admin_epic_board_list, list)
+        end
+      end
+    end
+  end
+end

ee/app/models/boards/epic_list.rb

@@ -16,6 +16,7 @@ module Boards
     scope :movable, -> { where(list_type: list_types.slice(*movable_types).values) }
 
     alias_method :preferences, :epic_list_user_preferences
+    alias_method :board, :epic_board
 
     def preferences_for(user)
       return preferences.build unless user
@@ -29,9 +30,5 @@ module Boards
         end
       end
     end
-
-    def board
-      epic_board
-    end
   end
 end

ee/app/policies/boards/epic_list_policy.rb

+# frozen_string_literal: true
+
+module Boards
+  class EpicListPolicy < ::BasePolicy
+    delegate :board
+  end
+end

ee/app/services/boards/epic_lists/destroy_service.rb

+# frozen_string_literal: true
+
+module Boards
+  module EpicLists
+    class DestroyService < ::Boards::Lists::BaseDestroyService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute(list)
+        unless list.board.group.licensed_feature_available?(:epics)
+          return ServiceResponse.error(message: 'Epics feature is not available.')
+        end
+
+        unless Feature.enabled?(:epic_boards, list.board.group)
+          return ServiceResponse.error(message: 'Epic boards feature is not enabled.')
+        end
+
+        unless can?(current_user, :admin_epic_board_list, list)
+          return ServiceResponse.error(message: 'The epic board list that you are attempting to destroy does not '\
+                  'exist or you don\'t have permission to perform this action')
+        end
+
+        super
+      end
+    end
+  end
+end

ee/changelogs/unreleased/324283-cablett-destroy-epic-board-list.yml

+---
+title: Epic board lists can now be destroyed via GraphQL
+merge_request: 58719
+author:
+type: added

ee/spec/requests/api/graphql/mutations/boards/epic_lists/destroy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Destroy an epic board list' do
+  include GraphqlHelpers
+
+  let_it_be(:current_user, reload: true) { create(:user) }
+  let_it_be(:group) { create(:group, :private) }
+  let_it_be(:board) { create(:epic_board, group: group) }
+  let_it_be(:list) { create(:epic_list, epic_board: board) }
+
+  let(:variables) do
+    {
+      list_id: global_id_of(list)
+    }
+  end
+
+  let(:mutation) do
+    graphql_mutation(:epic_board_list_destroy, variables)
+  end
+
+  let(:mutation_response) { graphql_mutation_response(:epic_board_list_destroy) }
+
+  before do
+    stub_licensed_features(epics: true)
+  end
+
+  it_behaves_like 'board lists destroy request' do
+    let(:klass) { Boards::EpicList }
+  end
+end

ee/spec/services/boards/epic_lists/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Boards::EpicLists::DestroyService do
+  let_it_be(:group) { create(:group) }
+  let_it_be_with_reload(:board) { create(:epic_board, group: group) }
+  let_it_be(:label) { create(:group_label, group: group, name: 'in-progress') }
+  let_it_be(:closed_list) { board.lists.create!(list_type: :closed) }
+
+  let!(:list) { create(:epic_list, epic_board: board) }
+  let(:user) { create(:user) }
+  let(:parent) { group }
+  let(:list_type) { :epic_list }
+  let(:params) do
+    { epic_board: board }
+  end
+
+  before do
+    stub_licensed_features(epics: true)
+    stub_feature_flags(epic_boards: true)
+  end
+
+  context 'when user does not have permission' do
+    it 'returns an error' do
+      response = described_class.new(parent, nil).execute(list)
+
+      expect(response).to be_error
+      expect(response.errors).to include('The epic board list that you are attempting to destroy does not '\
+                  'exist or you don\'t have permission to perform this action')
+    end
+  end
+
+  context 'when user has permission' do
+    before do
+      group.add_maintainer(user)
+    end
+
+    it_behaves_like 'lists destroy service'
+
+    context 'when epic feature is unavailable' do
+      before do
+        stub_licensed_features(epics: false)
+      end
+
+      it 'returns an error' do
+        response = described_class.new(parent, nil).execute(list)
+
+        expect(response).to be_error
+        expect(response.errors).to include("Epics feature is not available.")
+      end
+    end
+
+    context 'when epic_boards feature flag is disabled' do
+      before do
+        stub_feature_flags(epic_boards: false)
+      end
+
+      it 'returns an error' do
+        response = described_class.new(parent, nil).execute(list)
+
+        expect(response).to be_error
+        expect(response.errors).to include("Epic boards feature is not enabled.")
+      end
+    end
+  end
+end

spec/requests/api/graphql/mutations/boards/lists/destroy_spec.rb

@@ -6,72 +6,22 @@ RSpec.describe Mutations::Boards::Lists::Destroy do
   include GraphqlHelpers
 
   let_it_be(:current_user, reload: true) { create(:user) }
-  let_it_be(:project, reload: true) { create(:project) }
-  let_it_be(:board) { create(:board, project: project) }
-  let_it_be(:list) { create(:list, board: board) }
-  let(:mutation) do
-    variables = {
-      list_id: GitlabSchema.id_from_object(list).to_s
-    }
 
-    graphql_mutation(:destroy_board_list, variables)
-  end
-
-  subject { post_graphql_mutation(mutation, current_user: current_user) }
-
-  def mutation_response
-    graphql_mutation_response(:destroy_board_list)
-  end
-
-  context 'when the user does not have permission' do
-    it_behaves_like 'a mutation that returns a top-level access error'
-
-    it 'does not destroy the list' do
-      expect { subject }.not_to change { List.count }
+  it_behaves_like 'board lists destroy request' do
+    let_it_be(:group, reload: true) { create(:group) }
+    let_it_be(:board) { create(:board, group: group) }
+    let_it_be(:list) { create(:list, board: board) }
+    let(:variables) do
+      {
+        list_id: GitlabSchema.id_from_object(list).to_s
+      }
     end
-  end
 
-  context 'when the user has permission' do
-    before do
-      project.add_maintainer(current_user)
+    let(:mutation) do
+      graphql_mutation(:destroy_board_list, variables)
     end
 
-    context 'when given id is not for a list' do
-      let_it_be(:list) { build_stubbed(:issue, project: project) }
-
-      it 'returns an error' do
-        subject
-
-        expect(graphql_errors.first['message']).to include('does not represent an instance of List')
-      end
-    end
-
-    context 'when everything is ok' do
-      it 'destroys the list' do
-        expect { subject }.to change { List.count }.from(2).to(1)
-      end
-
-      it 'returns an empty list' do
-        post_graphql_mutation(mutation, current_user: current_user)
-
-        expect(mutation_response).to have_key('list')
-        expect(mutation_response['list']).to be_nil
-      end
-    end
-
-    context 'when the list is not destroyable' do
-      let_it_be(:list) { create(:list, board: board, list_type: :backlog) }
-
-      it 'does not destroy the list' do
-        expect { subject }.not_to change { List.count }.from(3)
-      end
-
-      it 'returns an error and not nil list' do
-        subject
-
-        expect(mutation_response['errors']).not_to be_empty
-        expect(mutation_response['list']).not_to be_nil
-      end
-    end
+    let(:mutation_response) { graphql_mutation_response(:destroy_board_list) }
+    let(:klass) { List }
   end
 end

spec/services/boards/lists/destroy_service_spec.rb

@@ -3,11 +3,20 @@
 require 'spec_helper'
 
 RSpec.describe Boards::Lists::DestroyService do
+  let_it_be(:user) { create(:user) }
+
+  let(:list_type) { :list }
+
   describe '#execute' do
     context 'when board parent is a project' do
-      let(:project) { create(:project) }
-      let(:board)   { create(:board, project: project) }
-      let(:user)    { create(:user) }
+      let_it_be(:project) { create(:project) }
+      let_it_be(:board) { create(:board, project: project) }
+      let_it_be(:list) { create(:list, board: board) }
+      let_it_be(:closed_list) { board.lists.closed.first }
+
+      let(:params) do
+        { board: board }
+      end
 
       let(:parent) { project }
 
@@ -15,9 +24,14 @@ RSpec.describe Boards::Lists::DestroyService do
     end
 
     context 'when board parent is a group' do
-      let(:group) { create(:group) }
-      let(:board)   { create(:board, group: group) }
-      let(:user)    { create(:user) }
+      let_it_be(:group) { create(:group) }
+      let_it_be(:board) { create(:board, group: group) }
+      let_it_be(:list) { create(:list, board: board) }
+      let_it_be(:closed_list) { board.lists.closed.first }
+
+      let(:params) do
+        { board: board }
+      end
 
       let(:parent) { group }
 

spec/support/shared_examples/requests/api/graphql/mutations/destroy_list_shared_examples.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.shared_examples 'board lists destroy request' do
+  include GraphqlHelpers
+
+  subject { post_graphql_mutation(mutation, current_user: current_user) }
+
+  shared_examples 'does not destroy the list and returns an error' do
+    it 'does not destroy the list' do
+      expect { subject }.not_to change { klass.count }
+    end
+
+    it 'returns an error and not nil list' do
+      subject
+
+      expect(mutation_response['errors']).not_to be_empty
+      expect(mutation_response['list']).not_to be_nil
+    end
+  end
+
+  context 'when the user does not have permission' do
+    it 'does not destroy the list' do
+      expect { subject }.not_to change { klass.count }
+    end
+
+    it 'returns an error' do
+      subject
+
+      expect(graphql_errors.first['message']).to include("The resource that you are attempting to access does not exist or you don't have permission to perform this action")
+    end
+  end
+
+  context 'when the user has permission' do
+    before do
+      group.add_maintainer(current_user)
+    end
+
+    context 'when given id is not for a list' do
+      # could be any non-list thing
+      let_it_be(:list) { group }
+
+      it 'returns an error' do
+        subject
+
+        expect(graphql_errors.first['message']).to include('does not represent an instance of')
+      end
+    end
+
+    context 'when list does not exist' do
+      let(:variables) do
+        {
+          list_id: "gid://gitlab/#{klass}/#{non_existing_record_id}"
+        }
+      end
+
+      it 'returns a top level error' do
+        subject
+
+        expect(graphql_errors.first['message']).to include('No object found for')
+      end
+    end
+
+    context 'when everything is ok' do
+      it 'destroys the list' do
+        expect { subject }.to change { klass.count }.by(-1)
+      end
+
+      it 'returns an empty list' do
+        post_graphql_mutation(mutation, current_user: current_user)
+
+        expect(mutation_response).to have_key('list')
+        expect(mutation_response['list']).to be_nil
+        expect(mutation_response['errors']).to be_empty
+      end
+    end
+
+    context 'when the list is not destroyable' do
+      before do
+        list.update!(list_type: :backlog)
+      end
+
+      it_behaves_like 'does not destroy the list and returns an error'
+    end
+  end
+end

spec/support/shared_examples/services/boards/lists_destroy_service_shared_examples.rb

@@ -3,30 +3,27 @@
 RSpec.shared_examples 'lists destroy service' do
   context 'when list type is label' do
     it 'removes list from board' do
-      list = create(:list, board: board)
       service = described_class.new(parent, user)
 
       expect { service.execute(list) }.to change(board.lists, :count).by(-1)
     end
 
     it 'decrements position of higher lists' do
-      development = create(:list, board: board, position: 0)
-      review      = create(:list, board: board, position: 1)
-      staging     = create(:list, board: board, position: 2)
-      closed      = board.lists.closed.first
+      development = create(list_type, params.merge(position: 0))
+      review      = create(list_type, params.merge(position: 1))
+      staging     = create(list_type, params.merge(position: 2))
 
       described_class.new(parent, user).execute(development)
 
       expect(review.reload.position).to eq 0
       expect(staging.reload.position).to eq 1
-      expect(closed.reload.position).to be_nil
+      expect(closed_list.reload.position).to be_nil
     end
   end
 
   it 'does not remove list from board when list type is closed' do
-    list = board.lists.closed.first
     service = described_class.new(parent, user)
 
-    expect { service.execute(list) }.not_to change(board.lists, :count)
+    expect { service.execute(closed_list) }.not_to change(board.lists, :count)
   end
 end