Merge branch 'prevent_type_convertion_for_synched_epic_wi' into 'master'

Reject conversion of Epic WIs that are a sync version of legacy Epic

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144615



Merged-by: Nicolas Dular <ndular@gitlab.com>
Approved-by: Nicolas Dular <ndular@gitlab.com>
Reviewed-by: Alexandru Croitor <acroitor@gitlab.com>
Reviewed-by: Nicolas Dular <ndular@gitlab.com>
Co-authored-by: Alexandru Croitor <acroitor@gitlab.com>

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
 

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

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

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

ee/spec/requests/api/graphql/mutations/work_items/delete_spec.rb

+# 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

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

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

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 ""

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