[IMP] runbot_merge: reject review without email

If a reviewer doesn't have an email set, the Signed-Off-By is an
`@users.noreply.github.com` address which just looks weird in the
final result.

Initially the thinking was that emails would be required for users to
*be* reviewers or self-reviewers, but since those are now o2ms / m2ms
it's a bit of a pain in the ass.

Instead, provide an action to easily try and fetch the public email of
a user from github.

Fixes #531

conftest.py

@@ -22,6 +22,8 @@ Configuration:
   ``role_reviewer``, ``role_self_reviewer`` and ``role_other``
     - name (optional, used as partner name when creating that, otherwise github
       login gets used)
+    - email (optional, used as partner email when creating that, otherwise
+      github email gets used, reviewer and self-reviewer must have an email)
     - token, a personal access token with the ``public_repo`` scope (otherwise
       the API can't leave comments), maybe eventually delete_repo (for personal
       forks)
@@ -44,6 +46,7 @@ import collections
 import configparser
 import contextlib
 import copy
+import functools
 import http.client
 import itertools
 import os
@@ -136,14 +139,16 @@ def rolemap(request, config):
 
 @pytest.fixture
 def partners(env, config, rolemap):
-    m = dict.fromkeys(rolemap.keys(), env['res.partner'])
+    m = {}
     for role, u in rolemap.items():
         if role in ('user', 'other'):
             continue
 
         login = u['login']
+        conf = config['role_' + role]
         m[role] = env['res.partner'].create({
-            'name': config['role_' + role].get('name', login),
+            'name': conf.get('name', login),
+            'email': conf.get('email') or u['email'] or False,
             'github_login': login,
         })
     return m

forwardport/tests/test_simple.py

@@ -84,7 +84,7 @@ def test_straightforward_flow(env, config, make_repo, users):
         number=pr.number,
         headers='',
         name=reviewer_name,
-        login=users['reviewer'],
+        email=config['role_reviewer']['email'],
     )
     assert prod.read_tree(p_1_merged) == {
         'f': 'e',
@@ -192,7 +192,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
         number='%s',
         headers='X-original-commit: {}\n'.format(p_1_merged.id),
         name=reviewer_name,
-        login=users['reviewer'],
+        email=config['role_reviewer']['email'],
     )
 
     old_b = prod.read_tree(b_head)
@@ -397,8 +397,6 @@ def test_partially_empty(env, config, make_repo):
         'y': '0',
     }
 
-# reviewer = of the FP sequence, the original PR is always reviewed by `user`
-# set as reviewer
 Case = collections.namedtuple('Case', 'author reviewer delegate success')
 ACL = [
     Case('reviewer', 'reviewer', None, True),
@@ -418,6 +416,9 @@ ACL = [
 ]
 @pytest.mark.parametrize(Case._fields, ACL)
 def test_access_rights(env, config, make_repo, users, author, reviewer, delegate, success):
+    """Validates the review rights *for the forward-port sequence*, the original
+    PR is always reviewed by `user`.
+    """
     prod, other = make_basic(env, config, make_repo)
     project = env['runbot_merge.project'].search([])
 
@@ -425,6 +426,7 @@ def test_access_rights(env, config, make_repo, users, author, reviewer, delegate
     c = env['res.partner'].create({
         'name': users['user'],
         'github_login': users['user'],
+        'email': 'user@example.org',
     })
     c.write({
         'review_rights': [
@@ -432,6 +434,12 @@ def test_access_rights(env, config, make_repo, users, author, reviewer, delegate
             for repo in project.repo_ids
         ]
     })
+    # create a partner for `other` so we can put an email on it
+    env['res.partner'].create({
+        'name': users['other'],
+        'github_login': users['other'],
+        'email': 'other@example.org',
+    })
 
     author_token = config['role_' + author]['token']
     fork = prod.fork(token=author_token)

mergebot_test_utils/utils.py

@@ -8,7 +8,7 @@ MESSAGE_TEMPLATE = """{message}
 
 closes {repo}#{number}
 
-{headers}Signed-off-by: {name} <{login}@users.noreply.github.com>"""
+{headers}Signed-off-by: {name} <{email}>"""
 # target branch '-' source branch '-' base64 unique '-fw'
 REF_PATTERN = r'{target}-{source}-[a-zA-Z0-9_-]{{4}}-fw'
 

runbot_merge/models/pull_requests.py

@@ -1005,12 +1005,14 @@ class PullRequests(models.Model):
                 elif param and is_reviewer:
                     oldstate = self.state
                     newstate = RPLUS.get(self.state)
-                    if newstate:
+                    if not author.email:
+                        msg = "I must know your email before you can review PRs. Please contact an administrator."
+                    elif not newstate:
+                        msg = "This PR is already reviewed, reviewing it again is useless."
+                    else:
                         self.state = newstate
                         self.reviewed_by = author
                         ok = True
-                    else:
-                        msg = "This PR is already reviewed, reviewing it again is useless."
                     _logger.debug(
                         "r+ on %s by %s (%s->%s) status=%s message? %s",
                         self.display_name, author.github_login,

runbot_merge/models/res_partner.py

@@ -1,6 +1,10 @@
+import random
 from email.utils import parseaddr
+
 from odoo import fields, models, tools, api
 
+from .. import github
+
 class CIText(fields.Char):
     type = 'char'
     column_type = ('citext', 'citext')
@@ -32,6 +36,14 @@ class Partner(models.Model):
                 email = ''
             partner.formatted_email = '%s <%s>' % (partner.name, email)
 
+    def fetch_github_email(self):
+        # this requires a token in order to fetch the email field, otherwise
+        # it's just not returned, select a random project to fetch
+        gh = github.GH(random.choice(self.env['runbot_merge.project'].search([])).github_token, None)
+        for p in self.filtered(lambda p: p.github_login and p.email is False):
+            p.email = gh.user(p.github_login)['email'] or False
+        return False
+
 class PartnerMerge(models.TransientModel):
     _inherit = 'base.partner.merge.automatic.wizard'
 

runbot_merge/tests/test_basic.py

@@ -216,6 +216,11 @@ class TestCommitMessage:
     def test_commit_delegate(self, env, repo, users, config):
         """ verify 'signed-off-by ...' is correctly added in the commit message for delegated review
         """
+        env['res.partner'].create({
+            'name': users['other'],
+            'github_login': users['other'],
+            'email': users['other'] + '@example.org'
+        })
         with repo:
             c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'})
             repo.make_ref('heads/master', c1)
@@ -2810,6 +2815,11 @@ class TestReviewing(object):
         """Users should be able to delegate review to either the creator of
         the PR or an other user without review rights
         """
+        env['res.partner'].create({
+            'name': users['user'],
+            'github_login': users['user'],
+            'email': users['user'] + '@example.org',
+        })
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
             m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'})
@@ -2824,10 +2834,7 @@ class TestReviewing(object):
         env.run_crons()
 
         assert prx.user == users['user']
-        assert env['runbot_merge.pull_requests'].search([
+        assert to_pr(env, prx).state == 'ready'
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ]).state == 'ready'
 
     def test_delegate_review_thirdparty(self, env, repo, users, config):
         """Users should be able to delegate review to either the creator of
@@ -2846,23 +2853,19 @@ class TestReviewing(object):
             other = ''.join(c.lower() if c.isupper() else c.upper() for c in users['other'])
             prx.post_comment('hansen delegate=%s' % other, config['role_reviewer']['token'])
         env.run_crons()
+        env['res.partner'].search([('github_login', '=', other)]).email = f'{other}@example.org'
 
         with repo:
             # check this is ignored
             prx.post_comment('hansen r+', config['role_user']['token'])
         assert prx.user == users['user']
-        assert env['runbot_merge.pull_requests'].search([
+        prx_id = to_pr(env, prx)
-            ('repository.name', '=', repo.name),
+        assert prx_id.state == 'validated'
-            ('number', '=', prx.number)
-        ]).state == 'validated'
 
         with repo:
             # check this works
             prx.post_comment('hansen r+', config['role_other']['token'])
-        assert env['runbot_merge.pull_requests'].search([
+        assert prx_id.state == 'ready'
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ]).state == 'ready'
 
     def test_delegate_prefixes(self, env, repo, config):
         with repo:
@@ -2880,7 +2883,6 @@ class TestReviewing(object):
 
         assert {d.github_login for d in pr.delegates} == {'foo', 'bar', 'baz'}
 
-
     def test_actual_review(self, env, repo, config):
         """ treat github reviews as regular comments
         """
@@ -2916,6 +2918,41 @@ class TestReviewing(object):
         assert pr.priority == 1
         assert pr.state == 'approved'
 
+    def test_no_email(self, env, repo, users, config, partners):
+        """A review should be rejected if the reviewer doesn't have an email
+        configured, otherwise the email address will show up
+        @users.noreply.github.com which is *weird*.
+        """
+        with repo:
+            [m] = repo.make_commits(
+                None,
+                Commit('initial', tree={'m': '1'}),
+                ref='heads/master'
+            )
+            [c] = repo.make_commits(m, Commit('first', tree={'m': '2'}))
+            pr = repo.make_pr(target='master', head=c)
+        env.run_crons()
+        with repo:
+            pr.post_comment('hansen delegate+', config['role_reviewer']['token'])
+            pr.post_comment('hansen r+', config['role_user']['token'])
+        env.run_crons()
+
+        user_partner = env['res.partner'].search([('github_login', '=', users['user'])])
+        assert user_partner.email is False
+        assert pr.comments == [
+            seen(env, pr, users),
+            (users['reviewer'], 'hansen delegate+'),
+            (users['user'], 'hansen r+'),
+            (users['user'], f"I'm sorry, @{users['user']}. I must know your email before you can review PRs. Please contact an administrator."),
+        ]
+        user_partner.fetch_github_email()
+        assert user_partner.email
+        with repo:
+            pr.post_comment('hansen r+', config['role_user']['token'])
+        env.run_crons()
+        assert to_pr(env, pr).state == 'approved'
+
+
 class TestUnknownPR:
     """ Sync PRs initially looked excellent but aside from the v4 API not
     being stable yet, it seems to have greatly regressed in performances to

runbot_merge/views/res_partner.xml

@@ -18,6 +18,14 @@
         <field name="model">res.partner</field>
         <field name="inherit_id" ref="base.view_partner_form"/>
         <field name="arch" type="xml">
+            <xpath expr="//sheet" position="before">
+                <header>
+                    <button type="object" name="fetch_github_email"
+                            string="Fetch Github Email" class="oe_highlight"
+                            attrs="{'invisible': ['|', ('email', '!=', False), ('github_login', '=', False)]}"
+                    />
+                </header>
+            </xpath>
             <xpath expr="//notebook" position="inside">
                 <page string="Mergebot" groups="runbot_merge.group_admin">
                     <group>