diff --git a/ee/app/models/ee/epic.rb b/ee/app/models/ee/epic.rb index f24d1db17d787d949ebfe77703e64a422b272db4..7bd048003273cd85972d8eb3dd16b2983d739a2e 100644 --- a/ee/app/models/ee/epic.rb +++ b/ee/app/models/ee/epic.rb @@ -66,7 +66,7 @@ module Epic belongs_to :parent, class_name: "Epic" has_many :children, class_name: "Epic", foreign_key: :parent_id has_many :events, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent - belongs_to :work_item, foreign_key: 'issue_id', inverse_of: :synced_epic + belongs_to :work_item, foreign_key: 'issue_id', inverse_of: :synced_epic, dependent: :destroy has_internal_id :iid, scope: :group diff --git a/ee/app/models/ee/work_item.rb b/ee/app/models/ee/work_item.rb index 73df632f26321ad7e3961d95571e2fae40956bf8..a40364f4ae8077568d69a2d9c6e93e7460d238e9 100644 --- a/ee/app/models/ee/work_item.rb +++ b/ee/app/models/ee/work_item.rb @@ -8,6 +8,8 @@ module WorkItem prepended do include FilterableByTestReports + before_destroy :check_if_can_be_destroyed, prepend: true + has_one :progress, class_name: 'WorkItems::Progress', foreign_key: 'issue_id', inverse_of: :work_item has_one :color, class_name: 'WorkItems::Color', foreign_key: 'issue_id', inverse_of: :work_item @@ -89,5 +91,33 @@ def blocking_work_items_query(inverse_direction: false) def epic_work_item? work_item_type.base_type == ::WorkItems::Type.default_by_type(:epic)&.base_type end + + override :allowed_work_item_type_change + def allowed_work_item_type_change + super + + return unless previous_type_was_epic? + return unless synced_epic.present? + + errors.add( + :work_item_type_id, + format( + _('cannot be changed to %{new_type} when the work item is a legacy epic synced work item'), + new_type: work_item_type.name.downcase + ) + ) + end + + def previous_type_was_epic? + changes["work_item_type_id"].first == ::WorkItems::Type.default_by_type(:epic).id + end + + def check_if_can_be_destroyed + return unless work_item_type.base_type.casecmp(::WorkItems::Type::BASE_TYPES[:epic][:name]) == 0 + return if !synced_epic.present? || synced_epic.destroyed? + + errors.add(:base, _('cannot be destroyed because this is a synced work item for a legacy epic')) + throw :abort # rubocop:disable Cop/BanCatchThrow -- to stop work item being destroyed + end end end diff --git a/ee/spec/models/work_item_spec.rb b/ee/spec/models/work_item_spec.rb index b79b864ab22587da1f10d5319e442588ec7ab815..ddd212a124eeffdcdb5734193abf2086e7cab7d7 100644 --- a/ee/spec/models/work_item_spec.rb +++ b/ee/spec/models/work_item_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WorkItem do +RSpec.describe WorkItem, feature_category: :team_planning do let_it_be(:reusable_project) { create(:project) } it 'has one `color`' do @@ -494,4 +494,52 @@ end end end + + describe '#allowed_work_item_type_change' do + context 'when epic work item does not have a synced legacy epic' do + let(:work_item) { create(:work_item, :epic) } + + it 'is does change work item type from epic to issue' do + work_item.assign_attributes(work_item_type: WorkItems::Type.default_by_type(:issue)) + + expect(work_item).to be_valid + expect(work_item.errors[:work_item_type_id]).to be_empty + end + end + + context 'when epic work item has a synced legacy epic' do + let!(:epic) { create(:epic, :with_synced_work_item) } + let(:work_item) { epic.work_item } + + it 'is does not change work item type from epic to issue' do + work_item.assign_attributes(work_item_type: WorkItems::Type.default_by_type(:issue)) + + expect(work_item).not_to be_valid + expect(work_item.errors[:work_item_type_id]) + .to include(_('cannot be changed to issue when the work item is a legacy epic synced work item')) + end + end + end + + describe '#before_destroy' do + context 'when epic work item does not have a synced legacy epic' do + let!(:work_item) { create(:work_item, :epic) } + + it 'is does destroy the epic work item' do + expect { work_item.destroy! }.to change { WorkItem.count }.by(-1) + end + end + + context 'when epic work item has a synced legacy epic' do + let!(:epic) { create(:epic, :with_synced_work_item) } + let(:work_item) { epic.work_item } + + it 'is does not destroy the epic work item' do + expect { work_item.destroy! }.to raise_error(ActiveRecord::RecordNotDestroyed) + expect(work_item.errors[:base]).to include( + _('cannot be destroyed because this is a synced work item for a legacy epic') + ) + end + end + end end diff --git a/ee/spec/requests/api/graphql/mutations/work_items/convert_spec.rb b/ee/spec/requests/api/graphql/mutations/work_items/convert_spec.rb index ce9bf16046ff816d22ff3de7b3658acd7cf985b9..1423f48eae3fbeeea72a6e593e3526228812d7ad 100644 --- a/ee/spec/requests/api/graphql/mutations/work_items/convert_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/work_items/convert_spec.rb @@ -62,4 +62,42 @@ expect(mutation_response['workItem']).to include('id' => work_item.to_global_id.to_s) end end + + context 'when converting epic work item' do + let(:current_user) { developer } + let_it_be(:group) { create(:group).tap { |group| group.add_developer(developer) } } + + before do + stub_licensed_features(okrs: true) + end + + context 'when epic work item does not have a synced epic' do + let_it_be(:work_item) { create(:work_item, :epic, namespace: group) } + + it 'converts the work item type', :aggregate_failures do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.to change { work_item.reload.work_item_type }.to(new_type) + + expect(response).to have_gitlab_http_status(:success) + expect(work_item.reload.work_item_type.base_type).to eq('objective') + expect(mutation_response['workItem']).to include('id' => work_item.to_global_id.to_s) + end + end + + context 'when epic work item has a synced epic' do + let_it_be(:epic) { create(:epic, :with_synced_work_item, group: group) } + let(:work_item) { epic.work_item } + + it 'does not convert the work item type', :aggregate_failures do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.not_to change { work_item.reload.work_item_type } + + expect(mutation_response['errors'].first).to eq( + "Work item type cannot be changed to objective when the work item is a legacy epic synced work item" + ) + end + end + end end diff --git a/ee/spec/requests/api/graphql/mutations/work_items/delete_spec.rb b/ee/spec/requests/api/graphql/mutations/work_items/delete_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..25a836b3893dc1fab0633200c7df62a5524cfb43 --- /dev/null +++ b/ee/spec/requests/api/graphql/mutations/work_items/delete_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Delete a work item', feature_category: :team_planning do + include GraphqlHelpers + + let_it_be(:group) { create(:group) } + let_it_be(:owner) { create(:user).tap { |user| group.add_owner(user) } } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:developer) { create(:user).tap { |user| project.add_developer(user) } } + + let(:current_user) { developer } + let(:mutation) { graphql_mutation(:workItemDelete, { 'id' => work_item.to_global_id.to_s }) } + let(:mutation_response) { graphql_mutation_response(:work_item_delete) } + + context 'when the user is not allowed to delete a work item' do + let(:work_item) { create(:work_item, project: project) } + + it_behaves_like 'a mutation that returns a top-level access error' + end + + context 'when user has permissions to delete a work item' do + context 'when deleting an epic work item' do + context 'when epic work item does not have a synced epic' do + let_it_be_with_refind(:work_item) do + create(:work_item, :epic, namespace: group) + end + + it 'deletes the epic work item' do + expect do + post_graphql_mutation(mutation, current_user: owner) + end.to change { WorkItem.count }.by(-1) + end + end + + context 'when epic work item has a synced epic' do + let_it_be(:epic) { create(:epic, :with_synced_work_item, group: group) } + let(:work_item) { epic.work_item } + + it 'does not deletes the epic work item' do + expect do + post_graphql_mutation(mutation, current_user: owner) + end.not_to change { WorkItem.count } + end + end + end + end +end diff --git a/ee/spec/requests/api/graphql/mutations/work_items/update_spec.rb b/ee/spec/requests/api/graphql/mutations/work_items/update_spec.rb index b6081144031ddf838fb30a8721e6f3ef6ef66c0a..1052b957da9194bb4542ad5ae729463335b846b9 100644 --- a/ee/spec/requests/api/graphql/mutations/work_items/update_spec.rb +++ b/ee/spec/requests/api/graphql/mutations/work_items/update_spec.rb @@ -804,4 +804,64 @@ def work_item_status end end end + + context 'when changing work item type' do + let(:fields) do + <<~FIELDS + workItem { + workItemType { + name + } + } + errors + FIELDS + end + + context 'when using quick action' do + let(:current_user) { reporter } + + before do + stub_licensed_features(okrs: true, epics: true, subepics: true) + end + + context 'for epic work item type' do + let(:input) { { 'descriptionWidget' => { 'description' => "/type objective" } } } + + context 'when epic work item does not have a synced epic' do + let_it_be_with_refind(:work_item) do + create(:work_item, :epic, namespace: group) + end + + it 'updates work item type' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + work_item.reload + end.to change { work_item.work_item_type.base_type }.from('epic').to('objective') + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['workItem']['workItemType']).to include( + { 'name' => 'Objective' } + ) + end + end + + context 'when epic work item has a synced epic' do + let_it_be(:epic) { create(:epic, :with_synced_work_item, group: group) } + let(:work_item) { epic.work_item } + + it 'does not update work item type' do + expect do + post_graphql_mutation(mutation, current_user: current_user) + work_item.reload + end.not_to change { work_item.work_item_type.base_type } + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['errors'].first).to eq( + "Work item type cannot be changed to objective when the work item is a legacy epic synced work item" + ) + end + end + end + end + end end diff --git a/ee/spec/services/ee/issuable/destroy_service_spec.rb b/ee/spec/services/ee/issuable/destroy_service_spec.rb index 66fa5c80d8a8588d663d44c038d9df0d2ab74b64..15fcff7d5d53f93f74479a7d394260fd2ab41fd4 100644 --- a/ee/spec/services/ee/issuable/destroy_service_spec.rb +++ b/ee/spec/services/ee/issuable/destroy_service_spec.rb @@ -9,19 +9,50 @@ describe '#execute' do context 'when destroying an epic' do - let_it_be_with_refind(:issuable) { create(:epic) } + context 'and epic does not have a synced work item' do + let_it_be_with_refind(:issuable) { create(:epic) } - let(:group) { issuable.group } + let(:group) { issuable.group } - it 'records usage ping epic destroy event' do - expect(Gitlab::UsageDataCounters::EpicActivityUniqueCounter).to receive(:track_epic_destroyed) - .with(author: user, namespace: group) + it 'records usage ping epic destroy event' do + expect(Gitlab::UsageDataCounters::EpicActivityUniqueCounter).to receive(:track_epic_destroyed).with( + author: user, namespace: group + ) - subject.execute(issuable) + subject.execute(issuable) + end + + it_behaves_like 'service deleting todos' + it_behaves_like 'service deleting label links' end - it_behaves_like 'service deleting todos' - it_behaves_like 'service deleting label links' + context 'and epic has a synced work item' do + let_it_be(:issuable) { create(:epic, :with_synced_work_item) } + let(:epic_work_item_id) { issuable.work_item.id } + let(:group) { issuable.group } + + it 'deletes the epic and the epic work item' do + # making sure we have this work item + expect(WorkItem.find_by_id(epic_work_item_id)).to be_present + + expect { subject.execute(issuable) }.to change { Epic.count }.by(-1).and( + change { WorkItem.count }.by(-1) + ) + + expect(epic_work_item_id).not_to be_nil + expect(WorkItem.find_by_id(epic_work_item_id)).to be_nil + end + + it 'records usage ping epic destroy event' do + expect(Gitlab::UsageDataCounters::EpicActivityUniqueCounter).to receive(:track_epic_destroyed) + .with(author: user, namespace: group) + + subject.execute(issuable) + end + + it_behaves_like 'service deleting todos' + it_behaves_like 'service deleting label links' + end end context 'when destroying other issuable type' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0d47d8f876d374a9ec88d9943ba87894b93b9c93..0509613e952d0aca9533ee670473311d47fa67e1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -58427,9 +58427,15 @@ msgstr "" msgid "cannot be changed to %{new_type} when linked to a parent %{parent_type}." msgstr "" +msgid "cannot be changed to %{new_type} when the work item is a legacy epic synced work item" +msgstr "" + msgid "cannot be changed to %{new_type} with these child item types." msgstr "" +msgid "cannot be destroyed because this is a synced work item for a legacy epic" +msgstr "" + msgid "cannot be enabled" msgstr "" diff --git a/spec/requests/api/graphql/mutations/work_items/convert_spec.rb b/spec/requests/api/graphql/mutations/work_items/convert_spec.rb index 97289597331df0c43700d609e478e9c32990db39..aebbd0867efd5e52107b17377c965ad518c738b1 100644 --- a/spec/requests/api/graphql/mutations/work_items/convert_spec.rb +++ b/spec/requests/api/graphql/mutations/work_items/convert_spec.rb @@ -12,6 +12,7 @@ create(:work_item, :task, project: project, milestone: create(:milestone, project: project)) end + let(:current_user) { create(:user) } let(:work_item_type_id) { new_type.to_global_id.to_s } let(:mutation) { graphql_mutation(:workItemConvert, input) } let(:mutation_response) { graphql_mutation_response(:work_item_convert) } @@ -58,4 +59,29 @@ let(:mutation_class) { ::Mutations::WorkItems::Convert } end end + + context 'when converting epic work item' do + let_it_be(:new_type) { create(:work_item_type, :issue, :default) } + let(:current_user) { developer } + let_it_be(:group) { create(:group).tap { |group| group.add_developer(developer) } } + + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(current_user, :create_issue, work_item).and_return(true) + end + + context 'when epic work item does not have a synced epic' do + let_it_be(:work_item) { create(:work_item, :epic, namespace: group) } + + it 'converts the work item type', :aggregate_failures do + expect do + post_graphql_mutation(mutation, current_user: current_user) + end.to change { work_item.reload.work_item_type }.to(new_type) + + expect(response).to have_gitlab_http_status(:success) + expect(work_item.reload.work_item_type.base_type).to eq('issue') + expect(mutation_response['workItem']).to include('id' => work_item.to_global_id.to_s) + end + end + end end