diff --git a/conftest.py b/conftest.py index 84a687d1..c0fe27ba 100644 --- a/conftest.py +++ b/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 diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index d2698dbb..46042730 100644 --- a/forwardport/tests/test_simple.py +++ b/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) diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 0cadbc11..e3d09349 100644 --- a/mergebot_test_utils/utils.py +++ b/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' diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index b91f3466..b82f20dd 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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, diff --git a/runbot_merge/models/res_partner.py b/runbot_merge/models/res_partner.py index cdb56aea..3956af9c 100644 --- a/runbot_merge/models/res_partner.py +++ b/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' diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 22bc5913..ec7a428d 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]).state == 'ready' + assert to_pr(env, prx).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([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]).state == 'validated' + prx_id = to_pr(env, prx) + assert prx_id.state == 'validated' with repo: # check this works prx.post_comment('hansen r+', config['role_other']['token']) - assert env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number) - ]).state == 'ready' + assert prx_id.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 diff --git a/runbot_merge/views/res_partner.xml b/runbot_merge/views/res_partner.xml index 50ac7b6d..8a7e6429 100644 --- a/runbot_merge/views/res_partner.xml +++ b/runbot_merge/views/res_partner.xml @@ -18,6 +18,14 @@ res.partner + +
+
+