Merge pull request #398 from Art4/338-replace-listing-methods-with-co…

…nsistent-solution Add `Group::listNames()` method as replacement for `Group::listing()`
kbsali · Apr 9, 2024 · ae384f0 · ae384f0
2 parents ece897d + 4764543
commit ae384f0
Show file tree

Hide file tree

Showing 7 changed files with 293 additions and 48 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased](https://github.com/kbsali/php-redmine-api/compare/v2.6.0...v2.x)
 
+### Added
+
+- New method `Redmine\Api\Group::listNames()` for listing the ids and names of all groups.
+
+### Deprecated
+
+- `Redmine\Api\Group::listing()` is deprecated, use `\Redmine\Api\Group::listNames()` instead.
+
 ## [v2.6.0](https://github.com/kbsali/php-redmine-api/compare/v2.5.0...v2.6.0) - 2024-03-25
 
 ### Added

diff --git a/src/Redmine/Api/Group.php b/src/Redmine/Api/Group.php
@@ -23,6 +23,8 @@ class Group extends AbstractApi
 {
     private $groups = [];
 
+    private $groupNames = null;
+
     /**
      * List groups.
      *
@@ -43,6 +45,26 @@ final public function list(array $params = []): array
         }
     }
 
+    /**
+     * Returns an array of all groups with id/name pairs.
+     *
+     * @return array<int,string> list of groups (id => name)
+     */
+    final public function listNames(): array
+    {
+        if ($this->groupNames !== null) {
+            return $this->groupNames;
+        }
+
+        $this->groupNames = [];
+
+        foreach ($this->list()['groups'] as $group) {
+            $this->groupNames[(int) $group['id']] = $group['name'];
+        }
+
+        return $this->groupNames;
+    }
+
     /**
      * List groups.
      *
@@ -79,12 +101,17 @@ public function all(array $params = [])
     /**
      * Returns an array of groups with name/id pairs.
      *
+     * @deprecated v2.7.0 Use listNames() instead.
+     * @see Group::listNames()
+     *
      * @param bool $forceUpdate to force the update of the groups var
      *
      * @return array list of groups (id => name)
      */
     public function listing($forceUpdate = false)
     {
+        @trigger_error('`' . __METHOD__ . '()` is deprecated since v2.7.0, use `' . __CLASS__ . '::listNames()` instead.', E_USER_DEPRECATED);
+
         if (empty($this->groups) || $forceUpdate) {
             $this->groups = $this->list();
         }

diff --git a/tests/Behat/Bootstrap/FeatureContext.php b/tests/Behat/Bootstrap/FeatureContext.php
@@ -205,6 +205,36 @@ public function theReturnedDataIsAnInstanceOf(string $className)
         $this->assertInstanceOf($className, $this->lastReturn);
     }
 
+    /**
+     * @Then the returned data is an array
+     */
+    public function theReturnedDataIsAnArray()
+    {
+        $this->assertIsArray($this->lastReturn);
+    }
+
+    /**
+     * @Then the returned data contains :count items
+     */
+    public function theReturnedDataContainsItems(int $count)
+    {
+        $this->assertCount($count, $this->lastReturn);
+    }
+
+    /**
+     * @Then the returned data contains the following data
+     */
+    public function theReturnedDataContainsTheFollowingData(TableNode $table)
+    {
+        $returnData = $this->lastReturn;
+
+        if (! is_array($returnData)) {
+            throw new RuntimeException('The returned data is not an array.');
+        }
+
+        $this->assertTableNodeIsSameAsArray($table, $returnData);
+    }
+
     /**
      * @Then the returned data has only the following properties
      */
@@ -257,54 +287,7 @@ public function theReturnedDataPropertyContainsTheFollowingData($property, Table
             throw new RuntimeException('The returned data on property "' . $property . '" is not an array.');
         }
 
-        foreach ($table as $row) {
-            $this->assertArrayHasKey($row['property'], $returnData);
-
-            $value = $returnData[$row['property']];
-
-            if ($value instanceof SimpleXMLElement) {
-                $value = strval($value);
-            }
-
-            $expected = $row['value'];
-
-            // Handle expected empty array
-            if ($value === [] && $expected === '[]') {
-                $expected = [];
-            }
-
-            // Handle expected int values
-            if (is_int($value) && ctype_digit($expected)) {
-                $expected = intval($expected);
-            }
-
-            // Handle expected float values
-            if (is_float($value) && is_numeric($expected)) {
-                $expected = floatval($expected);
-            }
-
-            // Handle expected null value
-            if ($value === null && $expected === 'null') {
-                $expected = null;
-            }
-
-            // Handle expected true value
-            if ($value === true && $expected === 'true') {
-                $expected = true;
-            }
-
-            // Handle expected false value
-            if ($value === false && $expected === 'false') {
-                $expected = false;
-            }
-
-            // Handle placeholder %redmine_id%
-            if (is_string($expected)) {
-                $expected = str_replace('%redmine_id%', strval($this->redmine->getVersionId()), $expected);
-            }
-
-            $this->assertSame($expected, $value, 'Error with property "' . $row['property'] . '"');
-        }
+        $this->assertTableNodeIsSameAsArray($table, $returnData);
     }
 
     /**
@@ -384,4 +367,56 @@ private function getItemFromArray(array $array, $key): mixed
 
         return $array;
     }
+
+    private function assertTableNodeIsSameAsArray(TableNode $table, array $data)
+    {
+        foreach ($table as $row) {
+            $this->assertArrayHasKey($row['property'], $data, 'Possible keys are: ' . implode(', ', array_keys($data)));
+
+            $value = $data[$row['property']];
+
+            if ($value instanceof SimpleXMLElement) {
+                $value = strval($value);
+            }
+
+            $expected = $row['value'];
+
+            // Handle expected empty array
+            if ($value === [] && $expected === '[]') {
+                $expected = [];
+            }
+
+            // Handle expected int values
+            if (is_int($value) && ctype_digit($expected)) {
+                $expected = intval($expected);
+            }
+
+            // Handle expected float values
+            if (is_float($value) && is_numeric($expected)) {
+                $expected = floatval($expected);
+            }
+
+            // Handle expected null value
+            if ($value === null && $expected === 'null') {
+                $expected = null;
+            }
+
+            // Handle expected true value
+            if ($value === true && $expected === 'true') {
+                $expected = true;
+            }
+
+            // Handle expected false value
+            if ($value === false && $expected === 'false') {
+                $expected = false;
+            }
+
+            // Handle placeholder %redmine_id%
+            if (is_string($expected)) {
+                $expected = str_replace('%redmine_id%', strval($this->redmine->getVersionId()), $expected);
+            }
+
+            $this->assertSame($expected, $value, 'Error with property "' . $row['property'] . '"');
+        }
+    }
 }
diff --git a/tests/Behat/Bootstrap/GroupContextTrait.php b/tests/Behat/Bootstrap/GroupContextTrait.php
@@ -57,6 +57,20 @@ public function iListAllGroups()
         );
     }
 
+    /**
+     * @When I list the names of all groups
+     */
+    public function iListTheNamesOfAllGroups()
+    {
+        /** @var Group */
+        $api = $this->getNativeCurlClient()->getApi('group');
+
+        $this->registerClientResponse(
+            $api->listNames(),
+            $api->getLastResponse()
+        );
+    }
+
     /**
      * @When I show the group with id :groupId
      */

diff --git a/tests/Behat/features/groups.feature b/tests/Behat/features/groups.feature
@@ -57,6 +57,27 @@ Feature: Interacting with the REST API for groups
             | id                | 4                    |
             | name              | Test Group           |
 
+    @group
+    Scenario: Listing names of all groups
+        Given I have a "NativeCurlClient" client
+        And I create a group with name "Test Group 1"
+        And I create a group with name "Test Group 2"
+        And I create a group with name "Test Group 3"
+        And I create a group with name "Test Group 4"
+        And I create a group with name "Test Group 5"
+        When I list the names of all groups
+        Then the response has the status code "200"
+        And the response has the content type "application/json"
+        And the returned data is an array
+        And the returned data contains "5" items
+        And the returned data contains the following data
+            | property          | value                |
+            | 4                 | Test Group 1         |
+            | 5                 | Test Group 2         |
+            | 6                 | Test Group 3         |
+            | 7                 | Test Group 4         |
+            | 8                 | Test Group 5         |
+
     @group
     Scenario: Showing a specific group
         Given I have a "NativeCurlClient" client

diff --git a/tests/Unit/Api/Group/ListNamesTest.php b/tests/Unit/Api/Group/ListNamesTest.php
@@ -0,0 +1,108 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Redmine\Tests\Unit\Api\Group;
+
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Redmine\Api\Group;
+use Redmine\Tests\Fixtures\AssertingHttpClient;
+
+#[CoversClass(Group::class)]
+class ListNamesTest extends TestCase
+{
+    /**
+     * @dataProvider getListNamesData
+     */
+    #[DataProvider('getListNamesData')]
+    public function testListNamesReturnsCorrectResponse($expectedPath, $responseCode, $response, $expectedResponse)
+    {
+        $client = AssertingHttpClient::create(
+            $this,
+            [
+                'GET',
+                $expectedPath,
+                'application/json',
+                '',
+                $responseCode,
+                'application/json',
+                $response,
+            ]
+        );
+
+        // Create the object under test
+        $api = new Group($client);
+
+        // Perform the tests
+        $this->assertSame($expectedResponse, $api->listNames());
+    }
+
+    public static function getListNamesData(): array
+    {
+        return [
+            'test without groups' => [
+                '/groups.json',
+                201,
+                <<<JSON
+                {
+                    "groups": []
+                }
+                JSON,
+                []
+            ],
+            'test with multiple groups' => [
+                '/groups.json',
+                201,
+                <<<JSON
+                {
+                    "groups": [
+                        {"id": 9, "name": "Group 1"},
+                        {"id": 8, "name": "Group 2"},
+                        {"id": 7, "name": "Group 3"}
+                    ]
+                }
+                JSON,
+                [
+                    9 => "Group 1",
+                    8 => "Group 2",
+                    7 => "Group 3",
+                ]
+            ],
+        ];
+    }
+
+    public function testListNamesCallsHttpClientOnlyOnce()
+    {
+        $client = AssertingHttpClient::create(
+            $this,
+            [
+                'GET',
+                '/groups.json',
+                'application/json',
+                '',
+                200,
+                'application/json',
+                <<<JSON
+                {
+                    "groups": [
+                        {
+                            "id": 1,
+                            "name": "Group 1"
+                        }
+                    ]
+                }
+                JSON,
+            ]
+        );
+
+        // Create the object under test
+        $api = new Group($client);
+
+        // Perform the tests
+        $this->assertSame([1 => 'Group 1'], $api->listNames());
+        $this->assertSame([1 => 'Group 1'], $api->listNames());
+        $this->assertSame([1 => 'Group 1'], $api->listNames());
+    }
+}