Merge pull request #3 from webmozart/fix-edge-cases

Made status matching stricter to reject false matches

Author: weaverryan

src/AppBundle/GitHub/StatusManager.php

@@ -10,10 +10,10 @@ class StatusManager
     const STATUS_REVIEWED = 'reviewed';
 
     private static $triggerWords = [
-        self::STATUS_NEEDS_REVIEW => 'needs review',
-        self::STATUS_NEEDS_WORK => 'needs work',
-        self::STATUS_WORKS_FOR_ME => 'works for me',
-        self::STATUS_REVIEWED => 'reviewed',
+        'needs review' => self::STATUS_NEEDS_REVIEW,
+        'needs work' => self::STATUS_NEEDS_WORK,
+        'works for me' => self::STATUS_WORKS_FOR_ME,
+        'reviewed' => self::STATUS_REVIEWED,
     ];
 
     private static $labels = [
@@ -35,19 +35,19 @@ class StatusManager
      */
     public function getStatusChangeFromComment($comment)
     {
-        // 1) Find the last "status:"
-        $statusPosition = $this->findStatusPosition($comment);
+        $triggerWord = implode('|', array_keys(self::$triggerWords));
+        $formatting = '[\\s\\*]*';
 
-        if ($statusPosition === false) {
-            return null;
-        }
+        // Match first character after "status:"
+        // Case insensitive ("i"), ignores formatting with "*" before or after the ":"
+        $pattern = "~(?=\n|^)${formatting}status${formatting}:${formatting}[\"']?($triggerWord)[\"']?${formatting}[.!]?${formatting}(?<=\r\n|\n|$)~i";
 
-        foreach (self::$triggerWords as $status => $triggerWord) {
-            // status should be right at the beginning of the string
-            if ($triggerWord === strtolower(substr($comment, $statusPosition, strlen($triggerWord)))) {
-                return $status;
-            }
+        if (preg_match_all($pattern, $comment, $matches)) {
+            // Second subpattern = first status character
+            return self::$triggerWords[strtolower(end($matches[1]))];
         }
+
+        return null;
     }
 
     /**
@@ -72,36 +72,4 @@ public static function getLabelToStatusMap()
     {
         return array_flip(self::$labels);
     }
-
-    /**
-     * Finds the position where the status string will start - e.g.
-     * for "Status: Needs review", this would return the position
-     * that points to the "N" in "Needs".
-     *
-     * If there are multiple "Status:" in the string, this returns the
-     * final one.
-     *
-     * This takes into account possible formatting (e.g. **Status**: )
-     *
-     * Returns the position or false if none was found.
-     *
-     * @param string $comment
-     * @return boolean|integer
-     */
-    private function findStatusPosition($comment)
-    {
-        // Match first character after "status:"
-        // Case insensitive ("i"), ignores formatting with "*" before or after the ":"
-        $pattern = '~status(\*+:\s*|:[\s\*]*)(\w)~i';
-
-        if (preg_match_all($pattern, $comment, $matches, PREG_OFFSET_CAPTURE)) {
-            // Second subpattern = first status character
-            $lastMatch = end($matches[2]);
-
-            // [matched string, offset]
-            return $lastMatch[1];
-        }
-
-        return false;
-    }
 }

src/AppBundle/Tests/GitHub/StatusManagerTest.php

@@ -37,30 +37,55 @@ public function getCommentsForStatusChange()
             'Status: needs work',
             StatusManager::STATUS_NEEDS_WORK
         );
+        $tests[] = array(
+            'Status: reviewed',
+            StatusManager::STATUS_REVIEWED
+        );
+
+        // accept quotes
+        $tests[] = array(
+            'Status: "reviewed"',
+            StatusManager::STATUS_REVIEWED
+        );
+        $tests[] = array(
+            "Status: 'reviewed'",
+            StatusManager::STATUS_REVIEWED
+        );
+
+        // accept trailing punctuation
         $tests[] = array(
             'Status: works for me!',
             StatusManager::STATUS_WORKS_FOR_ME
         );
         $tests[] = array(
-            'Status: reviewed',
-            StatusManager::STATUS_REVIEWED
+            'Status: works for me.',
+            StatusManager::STATUS_WORKS_FOR_ME
         );
 
         // play with different formatting
         $tests[] = array(
             'STATUS: REVIEWED',
             StatusManager::STATUS_REVIEWED
         );
-
         $tests[] = array(
             '**Status**: reviewed',
             StatusManager::STATUS_REVIEWED
         );
-
-        // missing the colon - so we do NOT read this
         $tests[] = array(
-            'Status reviewed',
-            null,
+            '**Status:** reviewed',
+            StatusManager::STATUS_REVIEWED
+        );
+        $tests[] = array(
+            '**Status: reviewed**',
+            StatusManager::STATUS_REVIEWED
+        );
+        $tests[] = array(
+            '**Status: reviewed!**',
+            StatusManager::STATUS_REVIEWED
+        );
+        $tests[] = array(
+            '**Status: reviewed**.',
+            StatusManager::STATUS_REVIEWED
         );
         $tests[] = array(
             'Status:reviewed',
@@ -71,9 +96,15 @@ public function getCommentsForStatusChange()
             StatusManager::STATUS_REVIEWED
         );
 
+        // reject missing colon
+        $tests[] = array(
+            'Status reviewed',
+            null,
+        );
+
         // multiple matches - use the last one
         $tests[] = array(
-            "Status: needs review \r\n that is what the issue *was* marked as. Now it should be Status: reviewed",
+            "Status: needs review \r\n that is what the issue *was* marked as.\r\n Status: reviewed",
             StatusManager::STATUS_REVIEWED
         );
         // "needs review" does not come directly after status: , so there is no status change
@@ -82,6 +113,18 @@ public function getCommentsForStatusChange()
             null
         );
 
+        // reject if the status is not on a line of its own
+        // use case: someone posts instructions about how to change a status
+        // in a comment
+        $tests[] = array(
+            'You should include e.g. the line `Status: needs review` in your comment',
+            null
+        );
+        $tests[] = array(
+            'Before the ticket was in state "Status: reviewed", but then the status was changed',
+            null
+        );
+
         return $tests;
     }
 }

src/AppBundle/Tests/webhook_examples/issue_comment.created.json

@@ -67,7 +67,7 @@
     },
     "created_at":"2015-06-28T12:44:21Z",
     "updated_at":"2015-06-28T12:44:21Z",
-    "body":"I'm commenting! Status: Needs Review."
+    "body":"I'm commenting!\nStatus: Needs Review."
   },
   "repository":{
     "id":21151556,