FIX Sort without specifying a table name

Using a table name in sort() is not allowed in CMS 5. We could use orderBy() here but member is the table it will sort on by default anyway so there's no need. Also added unit tests, which should have caught this ages ago.
silverstripe · Jan 31, 2023 · 57c9ac2 · 57c9ac2
1 parent 2274b3e
commit 57c9ac2
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 3 deletions.
diff --git a/src/Security/Member.php b/src/Security/Member.php
@@ -1196,7 +1196,7 @@ public static function map_in_groups($groups = null)
     public static function mapInCMSGroups($groups = null)
     {
         // non-countable $groups will issue a warning when using count() in PHP 7.2+
-        if (!$groups) {
+        if ($groups === null) {
             $groups = [];
         }
 
@@ -1205,7 +1205,7 @@ public static function mapInCMSGroups($groups = null)
             return ArrayList::create()->map();
         }
 
-        if (count($groups ?? []) == 0) {
+        if (count($groups) === 0) {
             $perms = ['ADMIN', 'CMS_ACCESS_AssetAdmin'];
 
             if (class_exists(CMSMain::class)) {
@@ -1246,7 +1246,7 @@ public static function mapInCMSGroups($groups = null)
             ]);
         }
 
-        return $members->sort('"Member"."Surname", "Member"."FirstName"')->map();
+        return $members->sort('"Surname", "FirstName"')->map();
     }
 
 

diff --git a/tests/php/Security/MemberTest.php b/tests/php/Security/MemberTest.php
@@ -2,6 +2,8 @@
 
 namespace SilverStripe\Security\Tests;
 
+use InvalidArgumentException;
+use SilverStripe\Admin\LeftAndMain;
 use SilverStripe\Control\Cookie;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Convert;
@@ -12,6 +14,7 @@
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DB;
 use SilverStripe\ORM\FieldType\DBDatetime;
+use SilverStripe\ORM\Map;
 use SilverStripe\ORM\ValidationException;
 use SilverStripe\ORM\ValidationResult;
 use SilverStripe\Security\Group;
@@ -1618,4 +1621,111 @@ public function testChangePasswordToBlankIsValidated()
         $result = $member->changePassword('');
         $this->assertFalse($result->isValid());
     }
+
+    public function testMapInCMSGroupsNoLeftAndMain()
+    {
+        if (class_exists(LeftAndMain::class)) {
+            $this->markTestSkipped('LeftAndMain must not exist for this test.');
+        }
+        $result = Member::mapInCMSGroups();
+        $this->assertInstanceOf(Map::class, $result);
+
+        $this->assertEmpty($result, 'Without LeftAndMain, no groups are CMS groups.');
+    }
+
+    /**
+     * @dataProvider mapInCMSGroupsProvider
+     */
+    public function testMapInCMSGroups(array $groupFixtures, array $groupCodes, array $expectedValue)
+    {
+        if (!empty($groupFixtures) && !empty($groupCodes)) {
+            throw new InvalidArgumentException('Data provider is misconfigured for this test.');
+        }
+
+        if (!class_exists(LeftAndMain::class)) {
+            $this->markTestSkipped('LeftAndMain must exist for this test.');
+        }
+
+        $groups = [];
+
+        // Convert fixture names to IDs
+        if (!empty($groupFixtures)) {
+            foreach ($groupFixtures as $index => $groupFixtureName) {
+                $groups[$index] = $this->objFromFixture(Group::class, $groupFixtureName)->ID;
+            }
+        }
+
+        // Convert codes to DataList
+        if (!empty($groupCodes)) {
+            $groups = Group::get()->filter(['Code' => $groupCodes]);
+        }
+
+        $result = Member::mapInCMSGroups($groups);
+        $this->assertInstanceOf(Map::class, $result);
+
+        $this->assertSame($expectedValue, $result->values());
+    }
+
+    public function mapInCMSGroupsProvider()
+    {
+        // Note: "ADMIN User" is not from the fixtures, that user is created by $this->logInWithPermission('ADMIN')
+        return [
+            'defaults' => [
+                'groupFixtures' => [],
+                'groupCodes' => [],
+                'expectedValue' => [
+                    'Admin ',
+                    'OtherAdmin ',
+                    'ADMIN User',
+                ],
+            ],
+            'single group in a list' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'staffgroup'
+                ],
+                'expectedValue' => [
+                    'Staff User',
+                ],
+            ],
+            'single group in IDs array' => [
+                'groups' => [
+                    'staffgroup',
+                ],
+                'groupCodes' => [],
+                'expectedValue' => [
+                    'Staff User',
+                ],
+            ],
+            'multiple groups in a list' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'staffgroup',
+                    'securityadminsgroup',
+                ],
+                'expectedValue' => [
+                    'Staff User',
+                    'Test User',
+                ],
+            ],
+            'multiple groups in IDs array' => [
+                'groupFixtures' => [
+                    'staffgroup',
+                    'securityadminsgroup',
+                ],
+                'groupCodes' => [],
+                'expectedValue' => [
+                    'Staff User',
+                    'Test User',
+                ],
+            ],
+            'group with no members' => [
+                'groupFixtures' => [],
+                'groupCodes' => [
+                    'memberless',
+                ],
+                'expectedValue' => [],
+            ],
+        ];
+    }
 }
diff --git a/tests/php/Security/MemberTest.yml b/tests/php/Security/MemberTest.yml
@@ -58,6 +58,8 @@
     Email: [email protected]
     Password: 1nitialPassword
   staffmember:
+    FirstName: Staff
+    Surname: User
     Email: [email protected]
     Groups: '=>SilverStripe\Security\Group.staffgroup'
   managementmember: