diff --git a/app/Resources/views/markdown/pr_table_errors.md.twig b/app/Resources/views/markdown/pr_table_errors.md.twig index 6450100..fedb7b3 100644 --- a/app/Resources/views/markdown/pr_table_errors.md.twig +++ b/app/Resources/views/markdown/pr_table_errors.md.twig @@ -1,5 +1,5 @@ -Hi! +Hi, thanks for this contribution! Your pull request description seems to be incomplete or malformed: @@ -7,8 +7,10 @@ Your pull request description seems to be incomplete or malformed: * {{ error.message }} {% endfor %} -Would you mind completing the contribution table ? This would help us understand how interesting your contribution is. +Would you mind completing it? This would help us understand how interesting your contribution is, thank you very much! -Thank you! +{% if missingRelatedTicket %} +Note: it is a good practice to open an issue before submitting a Pull Request as it allows maintainers to verify that the bug is effectively due to a defect in the code (and that it hasn't already been fixed) and to discuss the improvement/feature suggestion before a single line of code is written. Additionally, it may lead the Core Product team to mark that issue as a priority, further attracting the maintainer's attention. +{% endif %} (note: this is an automated message, but answering it will reach a real human ) diff --git a/src/AppBundle/PullRequests/BodyParser.php b/src/AppBundle/PullRequests/BodyParser.php index bcfd32d..b9d3adc 100644 --- a/src/AppBundle/PullRequests/BodyParser.php +++ b/src/AppBundle/PullRequests/BodyParser.php @@ -46,7 +46,7 @@ public function getBody() */ public function getDescription() { - $regex = sprintf(self::DEFAULT_PATTERN, 'Description', '.+'); + $regex = sprintf(self::DEFAULT_PATTERN, 'Description', '[^|]+'); return $this->extractWithRegex($regex); } @@ -142,13 +142,16 @@ public function isARefacto() } /** + * @Assert\NotBlank(message = "Your pull request does not seem to fix any issue, you might consider [creating one](https://github.com/PrestaShop/PrestaShop/issues/new/choose) (see note below).") + * * @return string */ public function getRelatedTicket() { - $regex = sprintf(self::DEFAULT_PATTERN, 'Fixed ticket', '.+'); + $regex = sprintf(self::DEFAULT_PATTERN, 'Fixed ticket', '?:.*(?:#|\/issues\/)([0-9]+)'); + $ticket = $this->extractWithRegex($regex); - return $this->extractWithRegex($regex); + return empty($ticket) ? '' : '#'.$ticket; } /** diff --git a/src/AppBundle/PullRequests/Listener.php b/src/AppBundle/PullRequests/Listener.php index 68be06c..972b183 100644 --- a/src/AppBundle/PullRequests/Listener.php +++ b/src/AppBundle/PullRequests/Listener.php @@ -58,11 +58,12 @@ public function checkForTableDescription(PullRequest $pullRequest) $bodyParser = new BodyParser($pullRequest->getBody()); $validationErrors = $this->validator->validate($bodyParser); - if (count($validationErrors) > 0) { + $missingRelatedTicket = empty($bodyParser->getRelatedTicket()); + if (\count($validationErrors) > 0) { $this->commentApi->sendWithTemplate( $pullRequest, 'markdown/pr_table_errors.md.twig', - ['errors' => $validationErrors] + ['errors' => $validationErrors, 'missingRelatedTicket' => $missingRelatedTicket] ); $this->logger->info(sprintf('[Invalid Table] Pull request n° %s', $pullRequest->getNumber())); @@ -79,7 +80,7 @@ public function removePullRequestValidationComment(PullRequest $pullRequest) $bodyParser = new BodyParser($pullRequest->getBody()); $bodyErrors = $this->validator->validate($bodyParser); - if (0 === count($bodyErrors)) { + if (0 === \count($bodyErrors)) { $this->repository->removeCommentsIfExists( $pullRequest, self::TABLE_ERROR, @@ -104,7 +105,7 @@ public function removePullRequestValidationComment(PullRequest $pullRequest) */ public function removeCommitValidationComment(PullRequest $pullRequest) { - if (0 === count($this->getErrorsFromCommits($pullRequest))) { + if (0 === \count($this->getErrorsFromCommits($pullRequest))) { $this->repository->removeCommentsIfExists( $pullRequest, self::COMMIT_ERROR, @@ -132,7 +133,7 @@ public function welcomePeople(PullRequest $pullRequest, User $sender) { $userCommits = $this->commitRepository->findAllByUser($sender); - if (0 !== count($userCommits)) { + if (0 !== \count($userCommits)) { return false; } @@ -166,7 +167,7 @@ private function getErrorsFromCommits(PullRequest $pullRequest) $commitParser = new CommitParser($commitLabel, $pullRequest); $validationErrors = $this->validator->validate($commitParser); - if (count($validationErrors) > 0) { + if (\count($validationErrors) > 0) { $commitsErrors[] = $commitLabel; } } diff --git a/tests/AppBundle/PullRequests/BodyParserTest.php b/tests/AppBundle/PullRequests/BodyParserTest.php index e470e30..e508ee7 100644 --- a/tests/AppBundle/PullRequests/BodyParserTest.php +++ b/tests/AppBundle/PullRequests/BodyParserTest.php @@ -86,7 +86,13 @@ public function testGetTypeForBugFix() public function testGetTicket() { - $this->assertSame('http://forge.prestashop.com/browse/TEST-1234', $this->bodyParser->getRelatedTicket()); + $this->assertSame('#1234', $this->bodyParser->getRelatedTicket()); + } + + public function testGetTicketUrl() + { + $this->bodyParser = new BodyParser(file_get_contents(__DIR__.'/../../Resources/PullRequestBody/feature.txt')); + $this->assertSame('#1234', $this->bodyParser->getRelatedTicket()); } public function testRepeatBodParserTestsWithSpaces() @@ -100,4 +106,18 @@ public function testRepeatBodParserTestsWithSpaces() $this->testIsDeprecated(); $this->testIsBackwardCompatible(); } + + public function testGetEmptyDescription() + { + $this->bodyParser = new BodyParser(file_get_contents(__DIR__.'/../../Resources/PullRequestBody/missing_description.txt')); + + $this->assertSame($this->bodyParser->getType(), 'bug fix'); + $this->assertContains($this->bodyParser->getType(), $this->bodyParser->getValidTypes()); + $this->assertFalse($this->bodyParser->isAFeature()); + $this->assertFalse($this->bodyParser->isAnImprovement()); + $this->assertTrue($this->bodyParser->isABugFix()); + $this->assertFalse($this->bodyParser->isASmallFix()); + $this->assertFalse($this->bodyParser->isARefacto()); + $this->assertEmpty($this->bodyParser->getDescription()); + } } diff --git a/tests/AppBundle/PullRequests/ListenerTest.php b/tests/AppBundle/PullRequests/ListenerTest.php new file mode 100644 index 0000000..de1bf28 --- /dev/null +++ b/tests/AppBundle/PullRequests/ListenerTest.php @@ -0,0 +1,63 @@ +validator = (new ValidatorBuilder()) + ->enableAnnotationMapping() + ->getValidator(); + } + + /** + * @dataProvider getTests + * + * @param $descriptionFilename + * @param $expected + */ + public function testDescriptions($descriptionFilename, $expected) + { + $body = file_get_contents(__DIR__.'/../../Resources/PullRequestBody/'.$descriptionFilename); + $bodyParser = new BodyParser($body); + + $validations = $this->validator->validate($bodyParser); + $this->assertSame(\count($expected), \count($validations)); + foreach ($validations as $validation) { + $this->assertContains($validation->getPropertyPath(), $expected); + } + } + + public function getTests() + { + return [ + 'Valid description' => [ + 'bug_fix.txt', + [], + ], + 'Missing description' => [ + 'missing_description.txt', + ['description'], + ], + 'Invalid type' => [ + 'invalid_type.txt', + ['type'], + ], + 'Invalid category' => [ + 'invalid_category.txt', + ['category'], + ], + 'No related ticked' => [ + 'no_related_ticket.txt', + ['relatedTicket'], + ], + ]; + } +} diff --git a/tests/Resources/PullRequestBody/bug_fix.txt b/tests/Resources/PullRequestBody/bug_fix.txt index 5c07e96..0c13953 100644 --- a/tests/Resources/PullRequestBody/bug_fix.txt +++ b/tests/Resources/PullRequestBody/bug_fix.txt @@ -10,7 +10,7 @@ Please take the time to edit the "Answers" rows with the necessary information:- | Category? | BO | BC breaks? | no | Deprecations? | yes -| Fixed ticket? | http://forge.prestashop.com/browse/TEST-1234 +| Fixed ticket? | #1234 | How to test? | To test it, launch unit tests diff --git a/tests/Resources/PullRequestBody/feature.txt b/tests/Resources/PullRequestBody/feature.txt index 3a22d55..1d79da9 100644 --- a/tests/Resources/PullRequestBody/feature.txt +++ b/tests/Resources/PullRequestBody/feature.txt @@ -10,7 +10,7 @@ Please take the time to edit the "Answers" rows with the necessary information:- | Category? | BO | BC breaks? | no | Deprecations? | yes -| Fixed ticket? | http://forge.prestashop.com/browse/TEST-1234 +| Fixed ticket? | https://github.com/PrestaShop/PrestaShop/issues/1234 | How to test? | To test it, launch unit tests diff --git a/tests/Resources/PullRequestBody/improvement.txt b/tests/Resources/PullRequestBody/improvement.txt index 5e29457..349b064 100644 --- a/tests/Resources/PullRequestBody/improvement.txt +++ b/tests/Resources/PullRequestBody/improvement.txt @@ -10,7 +10,7 @@ Please take the time to edit the "Answers" rows with the necessary information:- | Category? | BO | BC breaks? | no | Deprecations? | yes -| Fixed ticket? | http://forge.prestashop.com/browse/TEST-1234 +| Fixed ticket? | #1234 | How to test? | To test it, launch unit tests diff --git a/tests/Resources/PullRequestBody/invalid_category.txt b/tests/Resources/PullRequestBody/invalid_category.txt new file mode 100644 index 0000000..fe37153 --- /dev/null +++ b/tests/Resources/PullRequestBody/invalid_category.txt @@ -0,0 +1,23 @@ + + +| Questions | Answers +| ------------- | ------------------------------------------------------- +| Branch? | develop +| Description? | Such a great description +| Type? | bug fix +| Category? | AZ +| BC breaks? | no +| Deprecations? | yes +| Fixed ticket? | #1234 +| How to test? | To test it, launch unit tests + + + +#### Important guidelines + +* Make sure [your local branch is up to date](https://help.github.com/articles/syncing-a-fork/) before commiting your changes! +* Your code MUST respect [our Coding Standards](http://doc.prestashop.com/display/PS16/Coding+Standards) (for code written in PHP, JavaScript, HTML/CSS/Smarty/Twig, SQL)! +* Your commit name MUST respect our [naming convention](http://doc.prestashop.com/display/PS16/How+to+write+a+commit+message)! + diff --git a/tests/Resources/PullRequestBody/invalid_type.txt b/tests/Resources/PullRequestBody/invalid_type.txt new file mode 100644 index 0000000..c29632b --- /dev/null +++ b/tests/Resources/PullRequestBody/invalid_type.txt @@ -0,0 +1,23 @@ + + +| Questions | Answers +| ------------- | ------------------------------------------------------- +| Branch? | develop +| Description? | Such a great description +| Type? | +| Category? | BO +| BC breaks? | no +| Deprecations? | yes +| Fixed ticket? | #1234 +| How to test? | To test it, launch unit tests + + + +#### Important guidelines + +* Make sure [your local branch is up to date](https://help.github.com/articles/syncing-a-fork/) before commiting your changes! +* Your code MUST respect [our Coding Standards](http://doc.prestashop.com/display/PS16/Coding+Standards) (for code written in PHP, JavaScript, HTML/CSS/Smarty/Twig, SQL)! +* Your commit name MUST respect our [naming convention](http://doc.prestashop.com/display/PS16/How+to+write+a+commit+message)! + diff --git a/tests/Resources/PullRequestBody/missing_description.txt b/tests/Resources/PullRequestBody/missing_description.txt new file mode 100644 index 0000000..9f874f1 --- /dev/null +++ b/tests/Resources/PullRequestBody/missing_description.txt @@ -0,0 +1,23 @@ + + +| Questions | Answers +| ------------- | ------------------------------------------------------- +| Branch? | develop +| Description? | +| Type? | bug fix +| Category? | BO +| BC breaks? | no +| Deprecations? | yes +| Fixed ticket? | #1234 +| How to test? | To test it, launch unit tests + + + +#### Important guidelines + +* Make sure [your local branch is up to date](https://help.github.com/articles/syncing-a-fork/) before commiting your changes! +* Your code MUST respect [our Coding Standards](http://doc.prestashop.com/display/PS16/Coding+Standards) (for code written in PHP, JavaScript, HTML/CSS/Smarty/Twig, SQL)! +* Your commit name MUST respect our [naming convention](http://doc.prestashop.com/display/PS16/How+to+write+a+commit+message)! + diff --git a/tests/Resources/PullRequestBody/no_related_ticket.txt b/tests/Resources/PullRequestBody/no_related_ticket.txt new file mode 100644 index 0000000..e9f1857 --- /dev/null +++ b/tests/Resources/PullRequestBody/no_related_ticket.txt @@ -0,0 +1,23 @@ + + +| Questions | Answers +| ------------- | ------------------------------------------------------- +| Branch? | develop +| Description? | Such a great description +| Type? | bug fix +| Category? | BO +| BC breaks? | no +| Deprecations? | yes +| Fixed ticket? | +| How to test? | To test it, launch unit tests + + + +#### Important guidelines + +* Make sure [your local branch is up to date](https://help.github.com/articles/syncing-a-fork/) before commiting your changes! +* Your code MUST respect [our Coding Standards](http://doc.prestashop.com/display/PS16/Coding+Standards) (for code written in PHP, JavaScript, HTML/CSS/Smarty/Twig, SQL)! +* Your commit name MUST respect our [naming convention](http://doc.prestashop.com/display/PS16/How+to+write+a+commit+message)! + diff --git a/tests/Resources/PullRequestBody/with_spaces.txt b/tests/Resources/PullRequestBody/with_spaces.txt index 49d1795..2dfad05 100644 --- a/tests/Resources/PullRequestBody/with_spaces.txt +++ b/tests/Resources/PullRequestBody/with_spaces.txt @@ -10,7 +10,7 @@ Please take the time to edit the "Answers" rows with the necessary information:- | Category? | BO | BC breaks? | no | Deprecations? | yes -| Fixed ticket? | http://forge.prestashop.com/browse/TEST-1234 +| Fixed ticket? | This fixes #1234 | How to test? | To test it, launch unit tests