Additional fixes to Clone Command (CO-479)

COmanage · Mar 1, 2026 · 6b253a6 · 6b253a6
1 parent ada9a0b
commit 6b253a6
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 23 deletions.
diff --git a/app/src/Command/CloneCommand.php b/app/src/Command/CloneCommand.php
@@ -791,7 +791,8 @@ protected function cloneEntityRelations(
                                         ->first();
 
         if(!empty($srcent)) {
-          $this->cloneUpsert(
+          // In the event of an insert we'll need the record ID that was created
+          $targetEntId = $this->cloneUpsert(
             $SourceRelatedTable,
             $srcent,
             $TargetRelatedTable,
@@ -806,7 +807,7 @@ protected function cloneEntityRelations(
             // Recurse on any subrelations, but only on upserts.
             // (Cake's dependency handling should deal with deletes.)
 
-            $this->cloneEntityRelations($srcent, $targetent->id, $related[$rt], $targetDataSource, $targetCoId);
+            $this->cloneEntityRelations($srcent, $targetEntId, $related[$rt], $targetDataSource, $targetCoId);
           }
         } elseif(!empty($targetent)) {
           // If there is no $srcent but there is a $targetent, delete $targetent.
@@ -841,7 +842,8 @@ protected function cloneEntityRelations(
                                           // There should be at most one
                                           ->first();
 
-          $foundEntities[] = $this->cloneUpsert(
+          // In the event of an insert we'll need the record ID that was created
+          $targetEntId = $this->cloneUpsert(
             $SourceRelatedTable,
             $srcent,
             $TargetRelatedTable,
@@ -853,14 +855,16 @@ protected function cloneEntityRelations(
             true
           );
 
+          $foundEntities[] = $targetEntId;
+
           if(!empty($related[$rt])) {
             // Recurse on any subrelations. We have to recurse on _each_ source entity.
             // We only recurse on inserts and updates. We assume that on a delete
             // Cake's dependency declarations will remove the related models.
 
             // If $original was Pipeline and $srcent was Flange, we're now calling ourselves
             // with Flange and its relations (its Plugin instantiations, eg PipelineToolkit.PersonRoleMappers)
-            $this->cloneEntityRelations($srcent, $targetent->id, $related[$rt], $targetDataSource, $targetCoId);
+            $this->cloneEntityRelations($srcent, $targetEntId, $related[$rt], $targetDataSource, $targetCoId);
           }
         }
 
@@ -869,13 +873,13 @@ protected function cloneEntityRelations(
 
         $this->io->out("Reviewing " . $targetIterator->count() . " records in target for deletions");
 
-        foreach($targetIterator as $targetent) {
-          if(!in_array($targetent->id, $foundEntities)) {
+        foreach($targetIterator as $te) {
+          if(!in_array($te->id, $foundEntities)) {
             // We didn't see this target entry in the source data, so remove it
 
-            $this->io->out("Deleting target record " . $targetent->id);
+            $this->io->out("Deleting target record " . $te->id);
 
-            $TargetRelatedTable->delete($targetent);
+            $TargetRelatedTable->delete($te);
           }
         }                
       }

diff --git a/app/src/Lib/Util/SearchUtilities.php b/app/src/Lib/Util/SearchUtilities.php
@@ -36,16 +36,16 @@ class SearchUtilities {
   // To add a new clonable model, see https://spaces.at.internet2.edu/x/DIBuFQ
   // Because this list is used by CloneCommand, it should be sorted in dependency order.
   static protected $clonableModels = [
-    'ApiUsers',
-    'Apis',
+    'Servers',
+    'Types',
     'Cous',
     'Groups',
+    'ApiUsers',
     'IdentifierAssignments',
     'Pipelines',
     'ExternalIdentitySources',
     'ProvisioningTargets',
-    'Servers',
-    'Types'
+    'Apis'
   ];
 
   // To add a new backend to search:

diff --git a/app/src/Model/Entity/Group.php b/app/src/Model/Entity/Group.php
@@ -75,7 +75,7 @@ public function isAllMembers(): bool {
    */
 
   public function isAutomatic(): bool {
-    // If this list is updated, CousTable::getCloneSuccessors will need to be updated as well
+    // If this list is updated, GroupsTable::findNonAutomaticGroups will need to be updated as well
     return in_array($this->group_type, [GroupTypeEnum::ActiveMembers, GroupTypeEnum::AllMembers]);
   }
 

diff --git a/app/src/Model/Table/CousTable.php b/app/src/Model/Table/CousTable.php
@@ -182,16 +182,10 @@ public function getCloneSuccessors(
   ): array {
     // Pull the set of non-automatic COU Groups and return their UUIDs
 
-    $clonableGroups = $this->Groups->find()
-                                   ->where([
-                                    'cou_id' => $original->id,
-                                    'group_type NOT IN' => [
-                                      GroupTypeEnum::ActiveMembers,
-                                      GroupTypeEnum::AllMembers
-                                    ]
-                                   ])
-                                   ->all();
-
+    $clonableGroups = $this->Groups
+                           ->find('nonAutomaticGroups', ['cou_id' => $original->id])
+                           ->all();
+
     return $clonableGroups->extract('uuid')->toArray();
   }
 

diff --git a/app/src/Model/Table/GroupsTable.php b/app/src/Model/Table/GroupsTable.php
@@ -643,6 +643,38 @@ public function findMfaExemptGroup(Query $query, array $options): Query {
     ]);
   }
 
+  /**
+   * Find non-Automatic Groups.
+   * 
+   * @since  COmanage Registry v5.2.0
+   * @param  \Cake\ORM\Query $query   Query
+   * @param  array           $options Options: co_id, cou_id
+   * @return \Cake\ORM\Query          Query
+   */
+
+  public function findNonAutomaticGroups(Query $query, array $options): Query {
+    $whereClause = [
+      'status'            => SuspendableStatusEnum::Active,
+      'group_type NOT IN' => [GroupTypeEnum::ActiveMembers, GroupTypeEnum::AllMembers]
+    ];
+
+    // We accept _either_ co_id or cou_id (but not both, since cou_id implies co_id).
+    // We also specifically allow co_id and a null cou_id (all groups in a CO without
+    // a cou_id, ie: the CO level non-automatic Groups).
+
+    if(isset($options['co_id'])) {
+      $whereClause['co_id'] = $options['co_id'];
+
+      if(isset($options['cou_id']) && is_null($options['cou_id'])) {
+        $whereClause['cou_id IS'] = null;
+      }
+    } elseif(isset($options['cou_id'])) {
+      $whereClause['cou_id'] = $options['cou_id'];
+    }
+
+    return $query->where($whereClause);
+  }
+
   /**
    * Get the Admin Group for a CO.
    *
@@ -845,6 +877,11 @@ public function implementedEvents(): array {
    */
 
   public function localAfterSave(EventInterface $event, EntityInterface $entity, \ArrayObject $options): bool {
+    // If we're cloning we don't want to record history or manage Owners Groups
+    if(isset($options['clone']) && $options['clone']) {
+      return true;
+    }
+
     // We don't record history if autoOnly is set because we're in the middle of cloning
     // and aside from the datasources not lining up, it's not clear it makes sense to record
     // the history in that context