Added missing api methods for the integrations installation api #527
Conversation
acrobat
force-pushed
the
integration-installation-api
branch
from
61182f4 to
1e8d95a
Compare
There was a problem hiding this comment.
Thank you. Really good PR. I like that you added tests and docs.
I just had some minor comments. Also, you should add a link in the Integration class header to https://developer.github.com/v3/integrations/installations/
|
|
||
| /** | ||
| * Find all installations for the authenticated integration. | ||
| * |
There was a problem hiding this comment.
Add @link https://developer.github.com/v3/integrations/#find-installations
| * | ||
| * @return array | ||
| */ | ||
| public function listRepositories($userId) |
There was a problem hiding this comment.
$userId should be optional, right?
There was a problem hiding this comment.
No if you call the api without the userId you just get a 403 error irc. The repositories are also always checked for the current installation and the repo's the user has access to. I guess github will change this api route in the future so the userId is part of the uri.
There was a problem hiding this comment.
Okey. Good. I guess I misunderstood the docs.
|
|
||
| /** | ||
| * List repositories that are accessible to the authenticated installation. | ||
| * |
There was a problem hiding this comment.
|
|
||
| /** | ||
| * Add a single repository to an installation. | ||
| * |
There was a problem hiding this comment.
|
|
||
| /** | ||
| * Remove a single repository from an installation. | ||
| * |
There was a problem hiding this comment.
| ->method('get') | ||
| ->with('/integration/installations'); | ||
|
|
||
| $api->findInstallations(); |
There was a problem hiding this comment.
Test return value as well.
https://github.com/KnpLabs/php-github-api/blob/master/doc/testing.md
| ->method('get') | ||
| ->with('/installation/repositories'); | ||
|
|
||
| $api->listRepositories('1234'); |
There was a problem hiding this comment.
Test that the parameter is used. Also make a second test where you not using any $userId
There was a problem hiding this comment.
$userIdis required so this test should do. But can you give me some example or info on how to test the parameters content? I've thought about this test but I couldn't find on how to retrieve the passed parameters and check if $params['user_id'] was equal to the passed value
There was a problem hiding this comment.
Try something like:
$api->expects($this->once())
->method('get')
->with('/installation/repositories', ['user_id'=>'1234']);
acrobat
force-pushed
the
integration-installation-api
branch
from
1e8d95a to
4e5449b
Compare
|
I've added the links to the docblocks. I will extend the tests tonight! |
acrobat
force-pushed
the
integration-installation-api
branch
from
4e5449b to
6f89e35
Compare
|
@Nyholm I've extended the tests and other comments should be fixed! |
|
@Nyholm any more work needed here? |
|
Sorry for the delay. I will review this PR shortly |
|
Thanks a lot @Nyholm! |
Missing methds for the integrations installation api. See https://developer.github.com/v3/integrations/installations/#installations
And added the
findInstallationsapi method. Closes #516