Merge pull request #94 from atomiix/related_ticket

Make relatedTicket mandatory in the PR's description

app/Resources/views/markdown/pr_table_errors.md.twig

@@ -1,14 +1,16 @@
 <!-- PR_TABLE_DESCRIPTION_ERROR -->
-Hi!
+Hi, thanks for this contribution!
 
 Your pull request description seems to be incomplete or malformed:
 
 {% for error in errors %}
 * {{ 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 )

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;
     }
 
     /**

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;
             }
         }

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());
+    }
 }

tests/AppBundle/PullRequests/ListenerTest.php

@@ -0,0 +1,63 @@
+<?php
+
+namespace Tests\AppBundle\PullRequests;
+
+use AppBundle\PullRequests\BodyParser;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Validator\ValidatorBuilder;
+
+class ListenerTest extends TestCase
+{
+    private $validator;
+
+    public function setUp()
+    {
+        $this->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'],
+            ],
+        ];
+    }
+}

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
 
 <!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->

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
 
 <!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->

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
 
 <!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->

tests/Resources/PullRequestBody/invalid_category.txt

@@ -0,0 +1,23 @@
+<!-- Thank you for contributing to the PrestaShop project!
+
+Please take the time to edit the "Answers" rows with the necessary information:-->
+
+| 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
+
+<!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->
+
+#### 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)!
+

tests/Resources/PullRequestBody/invalid_type.txt

@@ -0,0 +1,23 @@
+<!-- Thank you for contributing to the PrestaShop project!
+
+Please take the time to edit the "Answers" rows with the necessary information:-->
+
+| 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
+
+<!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->
+
+#### 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)!
+

tests/Resources/PullRequestBody/missing_description.txt

@@ -0,0 +1,23 @@
+<!-- Thank you for contributing to the PrestaShop project!
+
+Please take the time to edit the "Answers" rows with the necessary information:-->
+
+| 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
+
+<!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->
+
+#### 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)!
+

tests/Resources/PullRequestBody/no_related_ticket.txt

@@ -0,0 +1,23 @@
+<!-- Thank you for contributing to the PrestaShop project!
+
+Please take the time to edit the "Answers" rows with the necessary information:-->
+
+| 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
+
+<!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->
+
+#### 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)!
+

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
 
 <!-- Click the form's "Preview button" to make sure the table is functional in GitHub. Thank you! -->