Merge pull request #10333 from creative-commoners/pulls/4.10/fix-group-code-dedupe

FIX Resolve deduping problem with group codes.

Author: emteknetnz

src/Security/Group.php

@@ -482,16 +482,7 @@ public function getTreeTitle()
      */
     public function setCode($val)
     {
-        $currentGroups = Group::get()
-            ->map('Code', 'Title')
-            ->toArray();
-        $code = Convert::raw2url($val);
-        $count = 2;
-        while (isset($currentGroups[$code])) {
-            $code = Convert::raw2url($val . '-' . $count);
-            $count++;
-        }
-        $this->setField("Code", $code);
+        $this->setField('Code', Convert::raw2url($val));
     }
 
     public function validate()
@@ -522,16 +513,6 @@ public function validate()
             ->map('Code', 'Title')
             ->toArray();
 
-        if (isset($currentGroups[$this->Code])) {
-            $result->addError(
-                _t(
-                    'SilverStripe\\Security\\Group.ValidationIdentifierAlreadyExists',
-                    'A Group ({group}) already exists with the same {identifier}',
-                    ['group' => $this->Code, 'identifier' => 'Code']
-                )
-            );
-        }
-
         if (in_array($this->Title, $currentGroups)) {
             $result->addError(
                 _t(
@@ -566,6 +547,9 @@ public function onBeforeWrite()
         if (!$this->Code && $this->Title != _t(__CLASS__ . '.NEWGROUP', "New Group")) {
             $this->setCode($this->Title);
         }
+
+        // Make sure the code for this group is unique.
+        $this->dedupeCode();
     }
 
     public function onBeforeDelete()
@@ -726,4 +710,25 @@ public function requireDefaultRecords()
 
         // Members are populated through Member->requireDefaultRecords()
     }
+
+    /**
+     * Code needs to be unique as it is used to identify a specific group. Ensure no duplicate
+     * codes are created.
+     *
+     * @deprecated 5.0 Remove deduping in favour of throwing a validation error for duplicates.
+     */
+    private function dedupeCode(): void
+    {
+        $currentGroups = Group::get()
+            ->exclude('ID', $this->ID)
+            ->map('Code', 'Title')
+            ->toArray();
+        $code = $this->Code;
+        $count = 2;
+        while (isset($currentGroups[$code])) {
+            $code = $this->Code . '-' . $count;
+            $count++;
+        }
+        $this->setField('Code', $code);
+    }
 }

tests/php/Security/GroupCsvBulkLoaderTest.php

@@ -24,6 +24,7 @@ public function testNewImport()
 
     public function testOverwriteExistingImport()
     {
+        // This group will be overwritten.
         $existinggroup = new Group();
         $existinggroup->Title = 'Old Group Title';
         $existinggroup->Code = 'newgroup1';
@@ -33,13 +34,15 @@ public function testOverwriteExistingImport()
         $results = $loader->load(__DIR__ . '/GroupCsvBulkLoaderTest/GroupCsvBulkLoaderTest.csv');
 
         $created = $results->Created()->toArray();
-        $this->assertEquals(count($created), 1);
-        $this->assertEquals($created[0]->Code, 'newchildgroup1');
+        $this->assertEquals(1, count($created));
+        $this->assertEquals('newchildgroup1', $created[0]->Code);
+        $this->assertEquals('New Child Group 1', $created[0]->Title);
 
+        // This overrides the group because the code matches, which takes precedence over the ID.
         $updated = $results->Updated()->toArray();
-        $this->assertEquals(count($updated), 1);
-        $this->assertEquals($updated[0]->Code, 'newgroup1-2');
-        $this->assertEquals($updated[0]->Title, 'New Group 1');
+        $this->assertEquals(1, count($updated));
+        $this->assertEquals('newgroup1', $updated[0]->Code);
+        $this->assertEquals('New Group 1', $updated[0]->Title);
     }
 
     public function testImportPermissions()
@@ -48,17 +51,17 @@ public function testImportPermissions()
         $results = $loader->load(__DIR__ . '/GroupCsvBulkLoaderTest/GroupCsvBulkLoaderTest_withExisting.csv');
 
         $created = $results->Created()->toArray();
-        $this->assertEquals(count($created), 1);
-        $this->assertEquals($created[0]->Code, 'newgroup1');
-        $this->assertEquals($created[0]->Permissions()->column('Code'), ['CODE1']);
+        $this->assertEquals(1, count($created));
+        $this->assertEquals('newgroup1', $created[0]->Code);
+        $this->assertEquals(['CODE1'], $created[0]->Permissions()->column('Code'));
 
         $updated = $results->Updated()->toArray();
-        $this->assertEquals(count($updated), 1);
-        $this->assertEquals($updated[0]->Code, 'existinggroup');
-        $array1=$updated[0]->Permissions()->column('Code');
-        $array2=['CODE1', 'CODE2'];
-        sort($array1);
-        sort($array2);
-        $this->assertEquals($array1, $array2);
+        $this->assertEquals(1, count($updated));
+        $this->assertEquals('existinggroup', $updated[0]->Code);
+        $actual = $updated[0]->Permissions()->column('Code');
+        $expected = ['CODE1', 'CODE2'];
+        sort($actual);
+        sort($expected);
+        $this->assertEquals($expected, $actual);
     }
 }

tests/php/Security/GroupTest.php

@@ -334,8 +334,40 @@ public function testGroupTitleDuplication()
         $this->assertEquals('duplicate-2', $group->Code);
 
         $group = new Group();
-        $group->Title = 'Duplicate';
+        $group->Title = 'Any Title';
+        $group->Code = 'duplicate';
         $group->write();
         $this->assertEquals('duplicate-3', $group->Code);
+
+        $group1 = new Group();
+        $group1->Title = 'Any Title1';
+        $group1->Code = 'some-code';
+        $group2 = new Group();
+        $group2->Title = 'Any Title2';
+        $group2->Code = 'some-code';
+        $group1->write();
+        $group2->write();
+        $this->assertEquals('some-code', $group1->Code);
+        $this->assertEquals('some-code-2', $group2->Code);
+    }
+
+    public function testSettingCodeRepeatedly()
+    {
+        // Setting the code to the code it already was doesn't modify it
+        $group = $this->objFromFixture(Group::class, 'group1');
+        $previousCode = $group->Code;
+        $group->Code = $previousCode;
+        $group->write();
+        $this->assertEquals($previousCode, $group->Code);
+
+        // Setting the code to a new code does modify it
+        $group->Code = 'new-code';
+        $group->write();
+        $this->assertEquals('new-code', $group->Code);
+
+        // The old code can be reused
+        $group->Code = $previousCode;
+        $group->write();
+        $this->assertEquals($previousCode, $group->Code);
     }
 }