Added support for GitHub reviews #58
Conversation
| $newStatus = null; | ||
|
|
||
| // Set status based on review state | ||
| switch (strtolower($data['review']['state'])) { |
There was a problem hiding this comment.
GitHub docs are a bit vague on this: REST api docs shows state in all uppercase, while the webhooks docs show it in lowercase. Just to be sure, I've added this strtolower() call.
| } | ||
| } | ||
|
|
||
| $event->setResponseData(array( |
There was a problem hiding this comment.
What about moving the beginning of the method and those line in the abstract listener too, it could call an abstract method to extract the status from the payload, wdyt?
| protected function parseStatusFromText($body) | ||
| { | ||
| $triggerWord = implode('|', array_keys(static::$triggerWordToStatus)); | ||
| $formatting = '[\\s\\*]*'; |
There was a problem hiding this comment.
FYI you don't have to escape special characters in a character class, except - if it's not at the end and to be used as literal.
| ); | ||
| } | ||
|
|
||
| private function checkUserIsAllowedToReview(array $data) |
There was a problem hiding this comment.
This method is also defined in AbstractStatusChangeSubscriber, and check implies it will throw exception or something. By Symfony conventions it should be isUserIsAllowedToReview 👍
|
|
||
| public static function setUpBeforeClass() | ||
| { | ||
| self::$dispatcher = new EventDispatcher(); |
There was a problem hiding this comment.
this should not be a static property. The dispatcher instance must not be shared between tests, otherwise the listener will run several times for subsequent tests (as listeners registered by previous tests will still be here).
I faced this issue when implementing the same feature before seeing your PR, because I added a check in the subscriber to avoid changing the status if it is already the current one. (making the test fail when a previous listener was running too with the config of the previous test).
|
@weaverryan any news on it ? |
|
@weaverryan ping |
|
I just made some changes! I would appreciate any review - it's still a bit of "guess work" when processing webhooks, and I'd like to not get it wrong :) |
|
This is merged and deployed! But, I don't know if the |
|
This is definitely NOT working on symfony-docs, see symfony/symfony-docs#7296 (review) |
|
@fabpot @nicolas-grekas can you check which webhooks are sent to carsonbot on symfony/symfony and symfony/symfony-docs ? |
|
Pull request reviews was already checked on symfony/symfony, and I've just checked it on symfony/symfony-docs. |
|
Can confirm that it's working now! 👍 symfony/symfony-docs#8899 (review) |
Currently,
status:and such in a review body isn't parsed by Carson. This is a bit annoying, as it means we either have to manually assign the status label or write an issue comment with just thestatus:.This PR adds a new status subscriber for GitHub reviews. Besides parsing review bodies, this subscriber does 2 more things:
needs reviewwhen a GitHub review is requested@weaverryan I'm still not sure how to implement the functional testing bit, if you can do that before/after the merge, I would be very happy :)