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 · 21d7c91 · 21d7c91
1 parent 2274b3e
commit 21d7c91
Show file tree

Hide file tree

Showing 3 changed files with 117 additions and 5 deletions.
diff --git a/src/Security/Member.php b/src/Security/Member.php
@@ -1188,15 +1188,15 @@ public static function map_in_groups($groups = null)
      *
      * If no groups are passed, all groups with CMS permissions will be used.
      *
-     * @param array $groups Groups to consider or NULL to use all groups with
+     * @param SS_List|array|null $groups Groups to consider or NULL to use all groups with
      *                      CMS permissions.
      * @return Map Returns a map of all members in the groups given that
      *                have CMS permissions.
      */
-    public static function mapInCMSGroups($groups = null)
+    public static function mapInCMSGroups(SS_List|array|null $groups = null): Map
     {
         // 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: