From e468d7116eaaa03574c4db8bfaa1984fe4f2474e Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Mon, 26 Nov 2018 10:28:27 +0100 Subject: [PATCH] [FIX] runbot_merge: ignore comment edition & deletion As well as review edition & dismissal. Closes #53 --- runbot_merge/controllers/__init__.py | 9 +++-- runbot_merge/models/pull_requests.py | 2 ++ runbot_merge/tests/fake_github/__init__.py | 37 +++++++++++++++---- runbot_merge/tests/remote.py | 18 ++++++++++ runbot_merge/tests/test_basic.py | 42 ++++++++++++++++++++++ 5 files changed, 100 insertions(+), 8 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 29d17f21..c9ba7910 100644 --- a/runbot_merge/controllers/__init__.py +++ b/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, diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index efe18265..aa3cd5d1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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}, diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 78e2061a..042b7716 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/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' } diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index 386de55b..f1663e98 100644 --- a/runbot_merge/tests/remote.py +++ b/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') diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ad43598b..2286e2a3 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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