NEW Validate the Title on Group is not empty (#10113)

lang/en.yml

@@ -217,6 +217,7 @@ en:
     GROUPNAME: 'Group name'
     GroupReminder: 'If you choose a parent group, this group will take all it''s roles'
     HierarchyPermsError: 'Can''t assign parent group "{group}" with privileged permissions (requires ADMIN access)'
+    ValidationIdentifierAlreadyExists: 'A Group ({group}) already exists with the same {identifier}'
     Locked: 'Locked?'
     MEMBERS: Members
     NEWGROUP: 'New Group'

src/Dev/SapphireTest.php

@@ -2434,9 +2434,15 @@ class SapphireTest extends PHPUnit_Framework_TestCase implements TestOnly
             $member = $this->cache_generatedMembers[$permCode];
         } else {
             // Generate group with these permissions
-            $group = Group::create();
-            $group->Title = "$permCode group";
-            $group->write();
+            $group = Group::get()->filterAny([
+                'Code' => "$permCode-group",
+                'Title' => "$permCode group",
+            ])->first();
+            if (!$group || !$group->exists()) {
+                $group = Group::create();
+                $group->Title = "$permCode group";
+                $group->write();
+            }
 
             // Create each individual permission
             foreach ($permArray as $permArrayItem) {

src/Security/Group.php

@@ -4,6 +4,7 @@ namespace SilverStripe\Security;
 
 use SilverStripe\Admin\SecurityAdmin;
 use SilverStripe\Core\Convert;
+use SilverStripe\Forms\CompositeValidator;
 use SilverStripe\Forms\DropdownField;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\Forms\Form;
@@ -21,6 +22,7 @@ use SilverStripe\Forms\HiddenField;
 use SilverStripe\Forms\HTMLEditor\HTMLEditorConfig;
 use SilverStripe\Forms\ListboxField;
 use SilverStripe\Forms\LiteralField;
+use SilverStripe\Forms\RequiredFields;
 use SilverStripe\Forms\Tab;
 use SilverStripe\Forms\TabSet;
 use SilverStripe\Forms\TextareaField;
@@ -480,7 +482,16 @@ class Group extends DataObject
      */
     public function setCode($val)
     {
-        $this->setField("Code", Convert::raw2url($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);
     }
 
     public function validate()
@@ -506,9 +517,45 @@ class Group extends DataObject
             }
         }
 
+        $currentGroups = Group::get()
+            ->filter('ID:not', $this->ID)
+            ->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(
+                    'SilverStripe\\Security\\Group.ValidationIdentifierAlreadyExists',
+                    'A Group ({group}) already exists with the same {identifier}',
+                    ['group' => $this->Title, 'identifier' => 'Title']
+                )
+            );
+        }
+
         return $result;
     }
 
+    public function getCMSCompositeValidator(): CompositeValidator
+    {
+        $validator = parent::getCMSCompositeValidator();
+
+        $validator->addValidator(RequiredFields::create([
+            'Title'
+        ]));
+
+        return $validator;
+    }
+
     public function onBeforeWrite()
     {
         parent::onBeforeWrite();

tests/php/Security/GroupCsvBulkLoaderTest.php

@@ -38,7 +38,7 @@ class GroupCsvBulkLoaderTest extends SapphireTest
 
         $updated = $results->Updated()->toArray();
         $this->assertEquals(count($updated), 1);
-        $this->assertEquals($updated[0]->Code, 'newgroup1');
+        $this->assertEquals($updated[0]->Code, 'newgroup1-2');
         $this->assertEquals($updated[0]->Title, 'New Group 1');
     }
 

tests/php/Security/GroupTest.php

@@ -5,6 +5,7 @@ namespace SilverStripe\Security\Tests;
 use InvalidArgumentException;
 use SilverStripe\Control\Controller;
 use SilverStripe\Dev\FunctionalTest;
+use SilverStripe\Forms\RequiredFields;
 use SilverStripe\ORM\ArrayList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Security\Group;
@@ -33,7 +34,7 @@ class GroupTest extends FunctionalTest
         $this->assertEquals('my-title', $g1->Code, 'Custom title gets converted to code if none exists already');
 
         $g2 = new Group();
-        $g2->Title = "My Title";
+        $g2->Title = "My Title and Code";
         $g2->Code = "my-code";
         $g2->write();
         $this->assertEquals('my-code', $g2->Code, 'Custom attributes are not overwritten by Title field');
@@ -101,6 +102,7 @@ class GroupTest extends FunctionalTest
     {
         $member = $this->objFromFixture(TestMember::class, 'admin');
         $group = new Group();
+        $group->Title = 'Title';
 
         // Can save user to unsaved group
         $group->Members()->add($member);
@@ -121,6 +123,7 @@ class GroupTest extends FunctionalTest
         /** @var Group $childGroup */
         $childGroup = $this->objFromFixture(Group::class, 'childgroup');
         $orphanGroup = new Group();
+        $orphanGroup->Title = 'Title';
         $orphanGroup->ParentID = 99999;
         $orphanGroup->write();
 
@@ -280,4 +283,59 @@ class GroupTest extends FunctionalTest
             'Members with only APPLY_ROLES can\'t assign parent groups with inherited ADMIN permission'
         );
     }
+
+    public function testGroupTitleValidation()
+    {
+        $group1 = $this->objFromFixture(Group::class, 'group1');
+
+        $newGroup = new Group();
+
+        $validators = $newGroup->getCMSCompositeValidator()->getValidators();
+        $this->assertCount(1, $validators);
+        $this->assertInstanceOf(RequiredFields::class, $validators[0]);
+        $this->assertTrue(in_array('Title', $validators[0]->getRequired()));
+
+        $newGroup->Title = $group1->Title;
+        $result = $newGroup->validate();
+        $this->assertFalse(
+            $result->isValid(),
+            'Group names cannot be duplicated'
+        );
+
+        $newGroup->Title = 'Title';
+        $result = $newGroup->validate();
+        $this->assertTrue($result->isValid());
+    }
+
+    public function testGroupTitleDuplication()
+    {
+        $group = $this->objFromFixture(Group::class, 'group1');
+        $group->Title = 'Group title modified';
+        $group->write();
+        $this->assertEquals('group-1', $group->Code);
+
+        $group = new Group();
+        $group->Title = 'Group 1';
+        $group->write();
+        $this->assertEquals('group-1-2', $group->Code);
+
+        $group = new Group();
+        $group->Title = 'Duplicate';
+        $group->write();
+        $group->Title = 'Duplicate renamed';
+        $group->write();
+        $this->assertEquals('duplicate', $group->Code);
+
+        $group = new Group();
+        $group->Title = 'Duplicate';
+        $group->write();
+        $group->Title = 'More renaming';
+        $group->write();
+        $this->assertEquals('duplicate-2', $group->Code);
+
+        $group = new Group();
+        $group->Title = 'Duplicate';
+        $group->write();
+        $this->assertEquals('duplicate-3', $group->Code);
+    }
 }

tests/php/Security/GroupTest.yml

@@ -1,12 +1,16 @@
 'SilverStripe\Security\Group':
   admingroup:
+    Title: Admin Group
     Code: admingroup
   parentgroup:
+    Title: Parent Group
     Code: parentgroup
   childgroup:
+    Title: Child Group
     Code: childgroup
     Parent: '=>SilverStripe\Security\Group.parentgroup'
   grandchildgroup:
+    Title: Grandchild Group
     Code: grandchildgroup
     Parent: '=>SilverStripe\Security\Group.childgroup'
   group1: