From 7033952913f26713d88f31b58339b9c1bd42a087 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Tue, 27 Mar 2018 16:39:29 +0200
Subject: [PATCH] [ADD] runbot_merge: reviews support

Reviews are interpreted like comments and can contain any number of
commands, with the difference that APPROVED and REQUEST_CHANGES are
interpreted as (respectively) r+ and r- prefixes.
---
 runbot_merge/controllers.py                | 30 +++++++-
 runbot_merge/tests/fake_github/__init__.py | 89 +++++++++++++++-------
 runbot_merge/tests/test_basic.py           | 24 +++++-
 runbot_merge/tests/test_multirepo.py       |  2 +-
 4 files changed, 113 insertions(+), 32 deletions(-)

diff --git a/runbot_merge/controllers.py b/runbot_merge/controllers.py
index 965033e3..d01c1dc2 100644
--- a/runbot_merge/controllers.py
+++ b/runbot_merge/controllers.py
@@ -10,7 +10,11 @@ class MergebotController(Controller):
     def index(self):
         event = request.httprequest.headers['X-Github-Event']
 
-        return EVENTS.get(event, lambda _: "Unknown event {}".format(event))(request.jsonrequest)
+        c = EVENTS.get(event)
+        if c:
+            return c(request.jsonrequest)
+        _logger.warn('Unknown event %s', event)
+        return 'Unknown event {}'.format(event)
 
 def handle_pr(event):
     if event['action'] in [
@@ -191,14 +195,36 @@ def handle_comment(event):
 
     return pr._parse_commands(partner, event['comment']['body'])
 
+def handle_review(event):
+    env = request.env(user=1)
+
+    partner = env['res.partner'].search([('github_login', '=', event['review']['user']['login'])])
+    if not partner:
+        _logger.info('ignoring comment from %s: not in system', event['review']['user']['login'])
+        return 'ignored'
+
+    pr = env['runbot_merge.pull_requests'].search([
+        ('number', '=', event['pull_request']['number']),
+        ('repository.name', '=', event['repository']['full_name'])
+    ])
+
+    firstline = ''
+    state = event['review']['state'].lower()
+    if state == 'approved':
+        firstline = pr.repository.project_id.github_prefix + ' r+\n'
+    elif state == 'request_changes':
+        firstline = pr.repository.project_id.github_prefix + ' r-\n'
+
+    return pr._parse_commands(partner, firstline + event['review']['body'])
+
 def handle_ping(event):
     print("Got ping! {}".format(event['zen']))
     return "pong"
 
 EVENTS = {
-    # TODO: add review?
     'pull_request': handle_pr,
     'status': handle_status,
     'issue_comment': handle_comment,
+    'pull_request_review': handle_review,
     'ping': handle_ping,
 }
diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py
index 13cdbc2a..f4eac4ba 100644
--- a/runbot_merge/tests/fake_github/__init__.py
+++ b/runbot_merge/tests/fake_github/__init__.py
@@ -327,9 +327,9 @@ class Repo(object):
             pr.base = body.get('base')
 
         if body.get('state') == 'open':
-            self.notify('pull_request', 'reopened', self.name, pr)
+            self.notify('pull_request', 'reopened', pr)
         elif body.get('state') == 'closed':
-            self.notify('pull_request', 'closed', self.name, pr)
+            self.notify('pull_request', 'closed', pr)
 
         return (200, {})
 
@@ -434,20 +434,20 @@ class PR(Issue):
         self.label = label
         self.state = 'open'
 
-        repo.notify('pull_request', 'opened', repo.name, self)
+        repo.notify('pull_request', 'opened', self)
 
     @Issue.title.setter
     def title(self, value):
         old = self.title
         Issue.title.fset(self, value)
-        self.repo.notify('pull_request', 'edited', self.repo.name, self, {
+        self.repo.notify('pull_request', 'edited', self, {
             'title': {'from': old}
         })
     @Issue.body.setter
     def body(self, value):
         old = self.body
         Issue.body.fset(self, value)
-        self.repo.notify('pull_request', 'edited', self.repo.name, self, {
+        self.repo.notify('pull_request', 'edited', self, {
             'body': {'from': old}
         })
     @property
@@ -456,22 +456,22 @@ class PR(Issue):
     @base.setter
     def base(self, value):
         old, self._base = self._base, value
-        self.repo.notify('pull_request', 'edited', self.repo.name, self, {
+        self.repo.notify('pull_request', 'edited', self, {
             'base': {'from': {'ref': old}}
         })
 
     def push(self, sha):
         self.head = sha
-        self.repo.notify('pull_request', 'synchronize', self.repo.name, self)
+        self.repo.notify('pull_request', 'synchronize', self)
 
     def open(self):
         assert self.state == 'closed'
         self.state = 'open'
-        self.repo.notify('pull_request', 'reopened', self.repo.name, self)
+        self.repo.notify('pull_request', 'reopened', self)
 
     def close(self):
         self.state = 'closed'
-        self.repo.notify('pull_request', 'closed', self.repo.name, self)
+        self.repo.notify('pull_request', 'closed', self)
 
     @property
     def commits(self):
@@ -480,6 +480,10 @@ class PR(Issue):
         return len({h for h, _ in git.walk_ancestors(store, self.head, False)}
                    - {h for h, _ in git.walk_ancestors(store, target, False)})
 
+    def post_review(self, state, user, body):
+        self.comments.append((user, "REVIEW %s\n\n%s " % (state, body)))
+        self.repo.notify('pull_request_review', state, self, user, body)
+
 class Commit(object):
     __slots__ = ['tree', 'message', 'author', 'committer', 'parents', 'statuses']
     def __init__(self, tree, message, author, committer, parents):
@@ -524,33 +528,39 @@ class Client(werkzeug.test.Client):
             data=json.dumps(data),
         )
 
-    def pull_request(self, action, repository, pr, changes=None):
+    def pull_request(self, action, pr, changes=None):
         assert action in ('opened', 'reopened', 'closed', 'synchronize', 'edited')
         return self.open(self._make_env(
             'pull_request', {
                 'action': action,
-                'pull_request': {
-                    'number': pr.number,
-                    'head': {
-                        'sha': pr.head,
-                        'label': pr.label,
-                    },
-                    'base': {
-                        'ref': pr.base,
-                        'repo': {
-                            'name': repository.split('/')[1],
-                            'full_name': repository,
-                        },
-                    },
-                    'title': pr.title,
-                    'body': pr.body,
-                    'commits': pr.commits,
-                    'user': { 'login': pr.user },
-                },
+                'pull_request': self._pr(pr),
                 **({'changes': changes} if changes else {})
             }
         ))
 
+    def pull_request_review(self, action, pr, user, body):
+        """
+        :type action: 'APPROVED' | 'REQUEST_CHANGES' | 'COMMENT'
+        :type pr: PR
+        :type user: str
+        :type body: str
+        """
+        assert action in ('APPROVED', 'REQUEST_CHANGES', 'COMMENT')
+        return self.open(self._make_env(
+            'pull_request_review', {
+                'review': {
+                    'state': action,
+                    'body': body,
+                    'user': {'login': user},
+                },
+                'pull_request': self._pr(pr),
+                'repository': {
+                    'name': pr.repo.name.split('/')[1],
+                    'full_name': pr.repo.name,
+                }
+            }
+        ))
+
     def status(self, repository, context, state, sha):
         assert state in ('success', 'failure', 'pending')
         return self.open(self._make_env(
@@ -573,3 +583,26 @@ class Client(werkzeug.test.Client):
         if isinstance(issue, PR):
             contents['issue']['pull_request'] = { 'url': 'fake' }
         return self.open(self._make_env('issue_comment', contents))
+
+    def _pr(self, pr):
+        """
+        :type pr: PR
+        """
+        return {
+            'number': pr.number,
+            'head': {
+                'sha': pr.head,
+                'label': pr.label,
+            },
+            'base': {
+                'ref': pr.base,
+                'repo': {
+                    'name': pr.repo.name.split('/')[1],
+                    'full_name': pr.repo.name,
+                },
+            },
+            'title': pr.title,
+            'body': pr.body,
+            'commits': pr.commits,
+            'user': {'login': pr.user},
+        }
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index f657877d..5be607fa 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -32,7 +32,9 @@ def repo(gh, env):
     })
     # need to create repo & branch in env so hook will allow processing them
     return gh.repo('odoo/odoo', hooks=[
-        ((odoo.http.root, '/runbot_merge/hooks'), ['pull_request', 'issue_comment', 'status'])
+        ((odoo.http.root, '/runbot_merge/hooks'), [
+            'pull_request', 'issue_comment', 'status', 'pull_request_review',
+        ])
     ])
 
 def test_trivial_flow(env, repo):
@@ -1120,3 +1122,23 @@ class TestReviewing(object):
             ('repository.name', '=', 'odoo/odoo'),
             ('number', '=', prx.number)
         ]).state == 'ready'
+
+    def test_actual_review(self, env, repo):
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'})
+        repo.make_ref('heads/master', m2)
+
+        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', '=', 'odoo/odoo'),
+            ('number', '=', prx.number)
+        ])
+
+        prx.post_review('COMMENT', 'reviewer', "hansen priority=1")
+        assert pr.priority == 1
+        assert pr.state == 'opened'
+
+        prx.post_review('APPROVED', 'reviewer', "hansen priority=2")
+        assert pr.priority == 2
+        assert pr.state == 'approved'
diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py
index edeefb14..e3c2a7f7 100644
--- a/runbot_merge/tests/test_multirepo.py
+++ b/runbot_merge/tests/test_multirepo.py
@@ -35,7 +35,7 @@ def project(env):
 def repo_a(gh, project):
     project.write({'repo_ids': [(0, 0, {'name': "odoo/a"})]})
     return gh.repo('odoo/a', hooks=[
-        ((odoo.http.root, '/runbot_merge/hooks'), ['pull_request', 'issue_comment', 'status'])
+        ((odoo.http.root, '/runbot_merge/hooks'), ['pull_request', 'issue_comment', 'status', 'pull_request_review'])
     ])
 
 @pytest.fixture