[FIX] runbot_merge: ignore comment edition & deletion

As well as review edition & dismissal.

Closes #53

runbot_merge/controllers/__init__.py

@@ -240,7 +240,9 @@ def handle_comment(env, event):
     issue = event['issue']['number']
     author = event['comment']['user']['login']
     comment = event['comment']['body']
-    _logger.info('comment: %s %s:%s "%s"', author, repo, issue, comment)
+    _logger.info('comment[%s]: %s %s:%s "%s"', event['action'], author, repo, issue, comment)
+    if event['action'] != 'created':
+        return "Ignored: action (%r) is not 'created'" % event['action']
 
     return _handle_comment(env, repo, issue, author, comment)
 
@@ -249,7 +251,10 @@ def handle_review(env, event):
     pr = event['pull_request']['number']
     author = event['review']['user']['login']
     comment = event['review']['body'] or ''
-    _logger.info('review: %s %s:%s "%s"', author, repo, pr, comment)
+
+    _logger.info('review[%s]: %s %s:%s "%s"', event['action'], author, repo, pr, comment)
+    if event['action'] != 'submitted':
+        return "Ignored: action (%r) is not 'submitted'" % event['action']
 
     return _handle_comment(
         env, repo, pr, author, comment,

runbot_merge/models/pull_requests.py

@@ -207,6 +207,7 @@ class Repository(models.Model):
         # get and handle all comments
         for comment in gh.comments(number):
             controllers.handle_comment(self.env, {
+                'action': 'created',
                 'issue': issue,
                 'sender': comment['user'],
                 'comment': comment,
@@ -215,6 +216,7 @@ class Repository(models.Model):
         # get and handle all reviews
         for review in gh.reviews(number):
             controllers.handle_review(self.env, {
+                'action': 'submitted',
                 'review': review,
                 'pull_request': pr,
                 'repository': {'full_name': self.name},

runbot_merge/tests/fake_github/__init__.py

@@ -3,6 +3,7 @@ import datetime
 import hashlib
 import hmac
 import io
+import itertools
 import json
 import logging
 import re
@@ -563,13 +564,29 @@ class Issue(object):
         self._title = title
         self._body = body
         self.number = max(repo.issues or [0]) + 1
-        self.comments = []
+        self._comments = []
         self.labels = set()
         repo.issues[self.number] = self
 
+    @property
+    def comments(self):
+        return [(c.user, c.body) for c in self._comments]
+
     def post_comment(self, body, user):
-        self.comments.append((user, body))
-        self.repo.notify('issue_comment', self, user, body)
+        c = Comment(user, body)
+        self._comments.append(c)
+        self.repo.notify('issue_comment', self, 'created', c)
+        return c.id
+
+    def edit_comment(self, cid, newbody, user):
+        c = next(c for c in self._comments if c.id == cid)
+        c.body = newbody
+        self.repo.notify('issue_comment', self, 'edited', c)
+
+    def delete_comment(self, cid, user):
+        c = next(c for c in self._comments if c.id == cid)
+        self._comments.remove(c)
+        self.repo.notify('issue_comment', self, 'deleted', c)
 
     @property
     def title(self):
@@ -584,6 +601,12 @@ class Issue(object):
     @body.setter
     def body(self, value):
         self._body = value
+class Comment:
+    _cseq = itertools.count()
+    def __init__(self, user, body, id=None):
+        self.user = user
+        self.body = body
+        self.id = id or next(self._cseq)
 
 class PR(Issue):
     def __init__(self, repo, title, body, target, ctid, user, label):
@@ -770,6 +793,7 @@ class Client(werkzeug.test.Client):
         assert action in ('APPROVE', 'REQUEST_CHANGES', 'COMMENT')
         return self.open(self._make_env(
             'pull_request_review', {
+                'action': 'submitted',
                 'review': {
                     'state': 'APPROVED' if action == 'APPROVE' else action,
                     'body': body,
@@ -795,12 +819,13 @@ class Client(werkzeug.test.Client):
             }
         ))
 
-    def issue_comment(self, issue, user, body):
+    def issue_comment(self, issue, action, comment):
+        assert action in ('created', 'edited', 'deleted')
         contents = {
-            'action': 'created',
+            'action': action,
             'issue': { 'number': issue.number },
             'repository': self._repo(issue.repo.name),
-            'comment': { 'body': body, 'user': {'login': user } },
+            'comment': { 'id': comment.id, 'body': comment.body, 'user': {'login': comment.user } },
         }
         if isinstance(issue, PR):
             contents['issue']['pull_request'] = { 'url': 'fake' }

runbot_merge/tests/remote.py

@@ -691,6 +691,24 @@ class PR:
         )
         assert 200 <= r.status_code < 300, r.json()
         wait_for_hook()
+        return r.json()['id']
+
+    def edit_comment(self, cid, body, user):
+        r = self._session.patch(
+            'https://api.github.com/repos/{}/issues/comments/{}'.format(self.repo.name, cid),
+            json={'body': body},
+            headers={'Authorization': 'token {}'.format(self.repo._tokens[user])}
+        )
+        assert 200 <= r.status_code < 300, r.json()
+        wait_for_hook()
+
+    def delete_comment(self, cid, user):
+        r = self._session.delete(
+            'https://api.github.com/repos/{}/issues/comments/{}'.format(self.repo.name, cid),
+            headers={'Authorization': 'token {}'.format(self.repo._tokens[user])}
+        )
+        assert r.status_code == 204, r.json()
+        wait_for_hook()
 
     def open(self):
         self._set_prop('state', 'open')

runbot_merge/tests/test_basic.py

@@ -2210,6 +2210,48 @@ class TestComments:
         assert {p.github_login for p in pr.delegates} \
             == {'foo', 'bar', 'baz'}
 
+    def test_delete(self, repo, env):
+        """ Comments being deleted should be ignored
+        """
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        repo.make_ref('heads/master', m)
+
+        c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+        prx = repo.make_pr('title', 'body', target='master', ctid=c1, user='user')
+        pr = env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', repo.name),
+            ('number', '=', prx.number)
+        ])
+
+        cid = prx.post_comment('hansen r+', user='reviewer')
+        # unreview by pushing a new commit
+        prx.push(repo.make_commit(c1, 'second', None, tree={'m': 'c2'}))
+        assert pr.state == 'opened'
+        prx.delete_comment(cid, 'reviewer')
+        # check that PR is still unreviewed
+        assert pr.state == 'opened'
+
+    def test_edit(self, repo, env):
+        """ Comments being edited should be ignored
+        """
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        repo.make_ref('heads/master', m)
+
+        c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+        prx = repo.make_pr('title', 'body', target='master', ctid=c1, user='user')
+        pr = env['runbot_merge.pull_requests'].search([
+            ('repository.name', '=', repo.name),
+            ('number', '=', prx.number)
+        ])
+
+        cid = prx.post_comment('hansen r+', user='reviewer')
+        # unreview by pushing a new commit
+        prx.push(repo.make_commit(c1, 'second', None, tree={'m': 'c2'}))
+        assert pr.state == 'opened'
+        prx.edit_comment(cid, 'hansen r+ edited', 'reviewer')
+        # check that PR is still unreviewed
+        assert pr.state == 'opened'
+
 class TestInfrastructure:
     def test_protection(self, repo):
         """ force-pushing on a protected ref should fail