From 2bade9c68b8b7e76ece332d8aa4b61dc30544620 Mon Sep 17 00:00:00 2001 From: Art4 Date: Mon, 25 Mar 2024 12:59:58 +0100 Subject: [PATCH 1/5] Add method Group::listNames() --- src/Redmine/Api/Group.php | 20 ++++++++ tests/Unit/Api/Group/ListNamesTest.php | 67 ++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 tests/Unit/Api/Group/ListNamesTest.php diff --git a/src/Redmine/Api/Group.php b/src/Redmine/Api/Group.php index 332acb21..ebf1bd0e 100644 --- a/src/Redmine/Api/Group.php +++ b/src/Redmine/Api/Group.php @@ -43,6 +43,26 @@ final public function list(array $params = []): array } } + /** + * Returns an array of all groups with id/name pairs. + * + * @return array list of groups (id => name) + */ + final public function listNames(): array + { + if (empty($this->groups)) { + $this->groups = $this->list(); + } + + $list = []; + + foreach ($this->groups['groups'] as $e) { + $list[(int) $e['id']] = $e['name']; + } + + return $list; + } + /** * List groups. * diff --git a/tests/Unit/Api/Group/ListNamesTest.php b/tests/Unit/Api/Group/ListNamesTest.php new file mode 100644 index 00000000..a942cde3 --- /dev/null +++ b/tests/Unit/Api/Group/ListNamesTest.php @@ -0,0 +1,67 @@ +assertSame($expectedResponse, $api->listNames()); + } + + public static function getListNamesData(): array + { + return [ + 'test with minimal parameters' => [ + '/groups.json', + 201, + << "Group 1", + ] + ], + ]; + } +} From 1e881ae70509fdba30140fd060f41c1c2d75c016 Mon Sep 17 00:00:00 2001 From: Art4 Date: Mon, 25 Mar 2024 14:40:58 +0100 Subject: [PATCH 2/5] add behat tests for Group::listNames() --- tests/Behat/Bootstrap/FeatureContext.php | 131 +++++++++++++------- tests/Behat/Bootstrap/GroupContextTrait.php | 14 +++ tests/Behat/features/groups.feature | 21 ++++ tests/Unit/Api/Group/ListNamesTest.php | 3 - 4 files changed, 118 insertions(+), 51 deletions(-) diff --git a/tests/Behat/Bootstrap/FeatureContext.php b/tests/Behat/Bootstrap/FeatureContext.php index 01982f18..d72ed4eb 100644 --- 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 index 1a678586..c7933101 100644 --- 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 index d21fc85a..ec72b9c5 100644 --- 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 index a942cde3..25bedd71 100644 --- a/tests/Unit/Api/Group/ListNamesTest.php +++ b/tests/Unit/Api/Group/ListNamesTest.php @@ -8,10 +8,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Redmine\Api\Group; -use Redmine\Exception\MissingParameterException; -use Redmine\Http\HttpClient; use Redmine\Tests\Fixtures\AssertingHttpClient; -use SimpleXMLElement; #[CoversClass(Group::class)] class ListNamesTest extends TestCase From 7f941541b57f8b8292391cab34d332bd352b5338 Mon Sep 17 00:00:00 2001 From: Art4 Date: Mon, 25 Mar 2024 15:21:07 +0100 Subject: [PATCH 3/5] Use internal cache for group names --- src/Redmine/Api/Group.php | 14 ++++++----- tests/Unit/Api/Group/ListNamesTest.php | 33 ++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/Redmine/Api/Group.php b/src/Redmine/Api/Group.php index ebf1bd0e..2ca07690 100644 --- 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. * @@ -50,17 +52,17 @@ final public function list(array $params = []): array */ final public function listNames(): array { - if (empty($this->groups)) { - $this->groups = $this->list(); + if ($this->groupNames !== null) { + return $this->groupNames; } - $list = []; + $this->groupNames = []; - foreach ($this->groups['groups'] as $e) { - $list[(int) $e['id']] = $e['name']; + foreach ($this->list()['groups'] as $group) { + $this->groupNames[(int) $group['id']] = $group['name']; } - return $list; + return $this->groupNames; } /** diff --git a/tests/Unit/Api/Group/ListNamesTest.php b/tests/Unit/Api/Group/ListNamesTest.php index 25bedd71..8da4f08d 100644 --- a/tests/Unit/Api/Group/ListNamesTest.php +++ b/tests/Unit/Api/Group/ListNamesTest.php @@ -61,4 +61,37 @@ public static function getListNamesData(): array ], ]; } + + public function testListNamesCallsHttpClientOnlyOnce() + { + $client = AssertingHttpClient::create( + $this, + [ + 'GET', + '/groups.json', + 'application/json', + '', + 200, + 'application/json', + <<assertSame([1 => 'Group 1'], $api->listNames()); + $this->assertSame([1 => 'Group 1'], $api->listNames()); + $this->assertSame([1 => 'Group 1'], $api->listNames()); + } } From 8f7e4478426a47b1d6b1c901b5c78a7df3c7eb97 Mon Sep 17 00:00:00 2001 From: Art4 Date: Mon, 25 Mar 2024 15:48:59 +0100 Subject: [PATCH 4/5] Improve tests --- tests/Unit/Api/Group/ListNamesTest.php | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/Unit/Api/Group/ListNamesTest.php b/tests/Unit/Api/Group/ListNamesTest.php index 8da4f08d..68197a2b 100644 --- a/tests/Unit/Api/Group/ListNamesTest.php +++ b/tests/Unit/Api/Group/ListNamesTest.php @@ -42,21 +42,32 @@ public function testListNamesReturnsCorrectResponse($expectedPath, $responseCode public static function getListNamesData(): array { return [ - 'test with minimal parameters' => [ + 'test without groups' => [ + '/groups.json', + 201, + << [ '/groups.json', 201, << "Group 1", + 9 => "Group 1", + 8 => "Group 2", + 7 => "Group 3", ] ], ]; From 4764543af7d46fce4c071e01bef331b0863b53a2 Mon Sep 17 00:00:00 2001 From: Art4 Date: Wed, 27 Mar 2024 10:55:25 +0100 Subject: [PATCH 5/5] Deprecate Group::listing() --- CHANGELOG.md | 8 ++++++++ src/Redmine/Api/Group.php | 5 +++++ tests/Unit/Api/GroupTest.php | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a874d967..ab9803b7 100644 --- 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 index 2ca07690..e532d64a 100644 --- a/src/Redmine/Api/Group.php +++ b/src/Redmine/Api/Group.php @@ -101,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/Unit/Api/GroupTest.php b/tests/Unit/Api/GroupTest.php index 71d0782e..ffb9252f 100644 --- a/tests/Unit/Api/GroupTest.php +++ b/tests/Unit/Api/GroupTest.php @@ -113,6 +113,38 @@ public function testAllReturnsClientGetResponseWithParameters() $this->assertSame($expectedReturn, $api->all($parameters)); } + /** + * Test listing(). + */ + public function testListingTriggersDeprecationWarning() + { + $client = $this->createMock(Client::class); + $client->method('requestGet') + ->willReturn(true); + $client->method('getLastResponseBody') + ->willReturn('{"groups":[{"id":1,"name":"Group 1"},{"id":5,"name":"Group 5"}]}'); + $client->method('getLastResponseContentType') + ->willReturn('application/json'); + + $api = new Group($client); + + // PHPUnit 10 compatible way to test trigger_error(). + set_error_handler( + function ($errno, $errstr): bool { + $this->assertSame( + '`Redmine\Api\Group::listing()` is deprecated since v2.7.0, use `Redmine\Api\Group::listNames()` instead.', + $errstr + ); + + restore_error_handler(); + return true; + }, + E_USER_DEPRECATED + ); + + $api->listing(); + } + /** * Test listing(). */