diff --git a/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php b/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php index e2d3956b7a..180ef7af22 100644 --- a/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php +++ b/datamodels/2.x/combodo-data-feature-removal/src/Service/DataCleanupService.php @@ -134,10 +134,6 @@ private function RecursiveDeletion(DBObject $oObjectToClean): bool /** @var DBObject $oDependentObj */ while ($oDependentObj = $oSet->Fetch()) { $iDeletePropagationOption = $oExtKeyAttDef->GetDeletionPropagationOption(); - if ($iDeletePropagationOption == DEL_MANUAL) { - $this->oObjectService->SetIssue(get_class($oDependentObj)); - continue; - } if ($oExtKeyAttDef->IsNullAllowed()) { // Optional external key, list to reset @@ -152,6 +148,12 @@ private function RecursiveDeletion(DBObject $oObjectToClean): bool return false; } } else { + // Mandatory external key + if ($iDeletePropagationOption == DEL_MANUAL) { + // Cannot be deleted automatically, must be handled manually + $this->oObjectService->SetIssue(get_class($oDependentObj)); + continue; + } // Propagate deletion only if not visited if ($this->IsVisited($oDependentObj)) { continue; diff --git a/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php b/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php index 82cf651570..6c3b790c41 100644 --- a/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php +++ b/datamodels/2.x/combodo-data-feature-removal/src/Service/StaticDeletionPlan.php @@ -59,9 +59,11 @@ public function GetStaticDeletionPlan(array $aClasses): array { foreach ($aClasses as $sClass) { $oDeletionPlanItem = $this->GetInitialClassDeletionPlan($sClass); - $oDeletionPlanEntity = new DeletionPlanEntity(); - $oDeletionPlanEntity->oDelete->Merge($oDeletionPlanItem); - $this->aDeletionPlan[$sClass] = $oDeletionPlanEntity; + // N°9831 Do not overwrite existing entity as it may already exist for this class if a previously processed class references it (issues/updates already accumulated must be kept) + if (false === array_key_exists($sClass, $this->aDeletionPlan)) { + $this->aDeletionPlan[$sClass] = new DeletionPlanEntity(); + } + $this->aDeletionPlan[$sClass]->oDelete->Merge($oDeletionPlanItem); $this->DeletionPlanForReferencingClasses($sClass); } diff --git a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php index cbf3b54f2e..9df032c777 100644 --- a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php +++ b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/DataCleanupServiceTest.php @@ -163,6 +163,58 @@ public function testGetCleanupSummary_ManualDeleteShouldFail() $this->AssertSummaryEquals($aExpected, $aRes); } + /** + * N°9831 + * + * A nullable external key configured as DEL_MANUAL must NOT be treated as an issue: it is simply + * reset (set to null), exactly like core DBObject::MakeDeletionPlan does. DEL_MANUAL only blocks + * for mandatory keys. This mirrors the real "Hypervisor.farm_id -> Farm" case (nullable + DEL_MANUAL). + * + * Regression guard for the ordering of the IsNullAllowed()/DEL_MANUAL checks in RecursiveDeletion: + * reverting that change would make ExecuteCleanup throw here instead of resetting the object. + */ + public function testExecuteCleanup_NullableManualShouldResetAndNotThrow() + { + $this->GivenDFRTreeInDB(<<ExecuteCleanup($aClasses); + + $aExpected = [ + ['DFRManualNullable', 1, 0 ], + ['DFRToRemoveLeaf', 0, 1 ], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + + /** + * N°9831 + * + * Summary counterpart of the test above: a nullable DEL_MANUAL external key is reported as an + * update (reset), with an issue count of 0. Reverting the RecursiveDeletion change would report + * it as an issue instead. + */ + public function testGetCleanupSummary_NullableManualShouldBeUpdateNotIssue() + { + $this->GivenDFRTreeInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + ['DFRManualNullable', 1, 0, 0 ], + ['DFRToRemoveLeaf', 0, 1, 0 ], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + private function AssertSummaryEquals(array $expected, $actual, $sMessage = '') { $aExpected = []; diff --git a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php index bd0e00a470..ca292f3ca1 100644 --- a/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php +++ b/tests/php-unit-tests/unitary-tests/datamodels/2.x/combodo-data-feature-removal/StaticDeletionPlanTest.php @@ -9,7 +9,6 @@ use Combodo\iTop\DataFeatureRemoval\Entity\DataCleanupSummaryEntity; use Combodo\iTop\DataFeatureRemoval\Service\StaticDeletionPlan; -use MetaModel; require_once __DIR__."/AbstractCleanup.php"; class StaticDeletionPlanTest extends \AbstractCleanup @@ -150,6 +149,66 @@ public function testGetCleanupSummaryWithIssues() $this->AssertSummaryEquals($aExpected, $aRes); } + /** + * N°9831 + * + * A DEL_MANUAL issue must still be reported when the referencing class (DFRManual) is itself part + * of the classes to remove AND is processed AFTER the class it points to (DFRToRemoveLeaf). + * + * This mirrors the real "VirtualMachine -> VirtualHost" case: the setup audit sorts the removed + * classes alphabetically and drops abstract classes, so a referencing class such as VirtualMachine + * ends up processed after its concrete targets (Farm/Hypervisor/Cloud). Here DFRToRemove is abstract + * and DFRToRemoveLeaf is its concrete child, while DFRManual points to DFRToRemove with a mandatory + * DEL_MANUAL external key. + * + * Regression: the plan entity accumulating the issue used to be overwritten when the referencing + * class was processed as a top-level class, silently dropping the issue (no blocking message on the + * summary page, then a crash at execution time). + */ + public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedAfterTarget(): void + { + $this->GivenDFRObjectsInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + 'DFRToRemoveLeaf' => ['iDeleteCount' => 1], + 'DFRManual' => ['iDeleteCount' => 1, 'iIssueCount' => 1], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + + /** + * N°9831 + * + * Control case: the same issue must also be reported when the referencing class is listed BEFORE + * its target (this order already worked, e.g. "Enclosure -> Rack"). Together with the test above, + * this locks in that issue detection is independent of the processing order. + */ + public function testGetCleanupSummary_IssueKeptWhenReferencingClassListedBeforeTarget(): void + { + $this->GivenDFRObjectsInDB(<<GetCleanupSummary($aClasses); + + $aExpected = [ + 'DFRToRemoveLeaf' => ['iDeleteCount' => 1], + 'DFRManual' => ['iDeleteCount' => 1, 'iIssueCount' => 1], + ]; + $this->AssertSummaryEquals($aExpected, $aRes); + } + public function testCircularRefsShouldNotRunInfinitely() { $this->GivenDFRObjectsInDB(<< cmdbAbstractObject + + + bizmodel,searchable + false + dfrmanualnullable + + + + + + + + + + name + + true + + + all + + + extkey_id + + + true + DFRToRemove + DEL_MANUAL + all + + + + + + + + + + +
+ + + 10 + + + 20 + + +
+ + cmdbAbstractObject + bizmodel,searchable @@ -718,6 +770,14 @@ + + + + + + + +