[IMP] runbot_merge: make review rights repo-dependent

As the odds of having more projects or more repos with different
requirements in the same project, the need to have different sets of
reviewers for different repositories increases.

As a result, rather than be trivial boolean flags the review info
should probably depend on the user / partner and the repo. Turns out
the permission checks had already been extracted into their own
function so most of the mess comes from testing utilities which went
and configured their review rights as needed.

Incidentally it might be that the test suite could just use something
like a sequence of commoditized accounts which get configured as
needed and not even looked at unless they're used.

conftest.py

@@ -123,18 +123,37 @@ def rolemap(config):
     return rolemap
 
 @pytest.fixture
-def users(env, config, rolemap):
+def partners(env, config, rolemap):
+    m = dict.fromkeys(rolemap.keys(), env['res.partner'])
     for role, login in rolemap.items():
         if role in ('user', 'other'):
             continue
 
-        env['res.partner'].create({
+        m[role] = env['res.partner'].create({
             'name': config['role_' + role].get('name', login),
             'github_login': login,
-            'reviewer': role == 'reviewer',
-            'self_reviewer': role == 'self_reviewer',
         })
+    return m
 
+@pytest.fixture
+def setreviewers(partners):
+    def _(*repos):
+        partners['reviewer'].write({
+            'review_rights': [
+                (0, 0, {'repository_id': repo.id, 'review': True})
+                for repo in repos
+            ]
+        })
+        partners['self_reviewer'].write({
+            'review_rights': [
+                (0, 0, {'repository_id': repo.id, 'self_review': True})
+                for repo in repos
+            ]
+        })
+    return _
+
+@pytest.fixture
+def users(partners, rolemap):
     return rolemap
 
 @pytest.fixture(scope='session')
@@ -953,18 +972,24 @@ class Environment:
         wait_for_hook()
 
 class Model:
-    __slots__ = ['_env', '_model', '_ids', '_fields']
+    __slots__ = ['env', '_name', '_ids', '_fields']
     def __init__(self, env, model, ids=(), fields=None):
-        object.__setattr__(self, '_env', env)
-        object.__setattr__(self, '_model', model)
+        object.__setattr__(self, 'env', env)
+        object.__setattr__(self, '_name', model)
         object.__setattr__(self, '_ids', tuple(ids or ()))
 
-        object.__setattr__(self, '_fields', fields or self._env(self._model, 'fields_get', attributes=['type', 'relation']))
+        object.__setattr__(self, '_fields', fields or self.env(self._name, 'fields_get', attributes=['type', 'relation']))
 
     @property
     def ids(self):
         return self._ids
 
+    @property
+    def _env(self): return self.env
+
+    @property
+    def _model(self): return self._name
+
     def __bool__(self):
         return bool(self._ids)
 

forwardport/tests/test_simple.py

@@ -725,10 +725,15 @@ def test_access_rights(env, config, make_repo, users, author, reviewer, delegate
     project = env['runbot_merge.project'].search([])
 
     # create a partner for `user`
-    env['res.partner'].create({
+    c = env['res.partner'].create({
         'name': users['user'],
         'github_login': users['user'],
-        'reviewer': True,
+    })
+    c.write({
+        'review_rights': [
+            (0, 0, {'repository_id': repo.id, 'review': True})
+            for repo in project.repo_ids
+        ]
     })
 
     author_token = config['role_' + author]['token']

forwardport/tests/test_weird.py

@@ -55,12 +55,21 @@ def make_basic(env, config, make_repo, *, fp_token, fp_remote):
             ref='heads/c',
         )
     other = prod.fork()
-    project.write({
-        'repo_ids': [(0, 0, {
-            'name': prod.name,
-            'required_statuses': 'legal/cla,ci/runbot',
-            'fp_remote_target': fp_remote and other.name,
-        })],
+    repo = env['runbot_merge.repository'].create({
+        'project_id': project.id,
+        'name': prod.name,
+        'required_statuses': 'legal/cla,ci/runbot',
+        'fp_remote_target': fp_remote and other.name,
+    })
+    env['res.partner'].search([
+        ('github_login', '=', config['role_reviewer']['user'])
+    ]).write({
+        'review_rights': [(0, 0, {'repository_id': repo.id, 'review': True})]
+    })
+    env['res.partner'].search([
+        ('github_login', '=', config['role_self_reviewer']['user'])
+    ]).write({
+        'review_rights': [(0, 0, {'repository_id': repo.id, 'self_review': True})]
     })
 
     return project, prod, other
@@ -219,19 +228,7 @@ class TestNotAllBranches:
           `-> b000       branch c
     """
     @pytest.fixture
-    def repos(self, env, config, make_repo):
-        project = env['runbot_merge.project'].create({
-            'name': 'proj',
-            'github_token': config['github']['token'],
-            'github_prefix': 'hansen',
-            'fp_github_token': config['github']['token'],
-            'branch_ids': [
-                (0, 0, {'name': 'a', 'fp_sequence': 2, 'fp_target': True}),
-                (0, 0, {'name': 'b', 'fp_sequence': 1, 'fp_target': True}),
-                (0, 0, {'name': 'c', 'fp_sequence': 0, 'fp_target': True}),
-            ]
-        })
-
+    def repos(self, env, config, make_repo, setreviewers):
         a = make_repo('A')
         with a:
             _, a_, _ = a.make_commits(
@@ -260,21 +257,32 @@ class TestNotAllBranches:
             )
             b.make_commits(_a, Commit('b000', tree={'c': 'x'}), ref='heads/c')
         b_dev = b.fork()
-        project.write({
-            'repo_ids': [
-                (0, 0, {
-                    'name': a.name,
-                    'required_statuses': 'ci/runbot',
-                    'fp_remote_target': a_dev.name,
-                }),
-                (0, 0, {
-                    'name': b.name,
-                    'required_statuses': 'ci/runbot',
-                    'fp_remote_target': b_dev.name,
-                    'branch_filter': '[("name", "in", ["a", "c"])]',
-                })
+
+        project = env['runbot_merge.project'].create({
+            'name': 'proj',
+            'github_token': config['github']['token'],
+            'github_prefix': 'hansen',
+            'fp_github_token': config['github']['token'],
+            'branch_ids': [
+                (0, 0, {'name': 'a', 'fp_sequence': 2, 'fp_target': True}),
+                (0, 0, {'name': 'b', 'fp_sequence': 1, 'fp_target': True}),
+                (0, 0, {'name': 'c', 'fp_sequence': 0, 'fp_target': True}),
             ]
         })
+        repo_a = env['runbot_merge.repository'].create({
+            'project_id': project.id,
+            'name': a.name,
+            'required_statuses': 'ci/runbot',
+            'fp_remote_target': a_dev.name,
+        })
+        repo_b = env['runbot_merge.repository'].create({
+            'project_id': project.id,
+            'name': b.name,
+            'required_statuses': 'ci/runbot',
+            'fp_remote_target': b_dev.name,
+            'branch_filter': '[("name", "in", ["a", "c"])]',
+        })
+        setreviewers(repo_a, repo_b)
         return project, a, a_dev, b, b_dev
 
     def test_single_first(self, env, repos, config):

forwardport/tests/utils.py

@@ -85,12 +85,21 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje
             ref='heads/c',
         )
     other = prod.fork()
-    project.write({
-        'repo_ids': [(0, 0, {
-            'name': prod.name,
-            'required_statuses': 'legal/cla,ci/runbot',
-            'fp_remote_target': other.name,
-        })],
+    repo = env['runbot_merge.repository'].create({
+        'project_id': project.id,
+        'name': prod.name,
+        'required_statuses': 'legal/cla,ci/runbot',
+        'fp_remote_target': other.name,
+    })
+    env['res.partner'].search([
+        ('github_login', '=', config['role_reviewer']['user'])
+    ]).write({
+        'review_rights': [(0, 0, {'repository_id': repo.id, 'review': True})]
+    })
+    env['res.partner'].search([
+        ('github_login', '=', config['role_self_reviewer']['user'])
+    ]).write({
+        'review_rights': [(0, 0, {'repository_id': repo.id, 'self_review': True})]
     })
 
     return prod, other

runbot_merge/__manifest__.py

@@ -1,6 +1,6 @@
 {
     'name': 'merge bot',
-    'version': '1.1',
+    'version': '1.2',
     'depends': ['contacts', 'website'],
     'data': [
         'security/security.xml',

runbot_merge/controllers/reviewer_provisioning.py

@@ -14,18 +14,18 @@ class MergebotReviewerProvisioning(http.Controller):
     @from_role('accounts')
     @http.route(['/runbot_merge/get_reviewers'], type='json', auth='public')
     def fetch_reviewers(self, **kwargs):
-        partners = request.env['res.partner'].sudo().search(['|', ('self_reviewer', '=', True), ('reviewer', '=', True)])
-        return partners.mapped('github_login')
+        reviewers = request.env['res.partner.review'].sudo().search([
+            '|', ('review', '=', True), ('self_review', '=', True)
+        ]).mapped('partner_id.github_login')
+        return reviewers
 
     @from_role('accounts')
     @http.route(['/runbot_merge/remove_reviewers'], type='json', auth='public', methods=['POST'])
     def update_reviewers(self, github_logins, **kwargs):
         partners = request.env['res.partner'].sudo().search([('github_login', 'in', github_logins)])
-
-        # remove reviewer flag from the partner
         partners.write({
-            'reviewer': False,
-            'self_reviewer': False,
+            'review_rights': [(5, 0, 0)],
+            'delegate_reviewer': [(5, 0, 0)],
         })
 
         # Assign the linked users as portal users

runbot_merge/migrations/13.0.1.2/pre-migration.py

@@ -0,0 +1,16 @@
+def migrate(cr, version):
+    cr.execute("""
+    create table res_partner_review (
+        id serial primary key,
+        partner_id integer not null references res_partner (id),
+        repository_id integer not null references runbot_merge_repository (id),
+        review bool,
+        self_review bool
+    )
+    """)
+    cr.execute("""
+    insert into res_partner_review (partner_id, repository_id, review, self_review)
+    select p.id, r.id, reviewer, self_reviewer
+    from res_partner p, runbot_merge_repository r
+    where p.reviewer or p.self_reviewer
+    """)

runbot_merge/models/pull_requests.py

@@ -888,7 +888,11 @@ class PullRequests(models.Model):
         if not self:
             return ACL(False, False, False)
 
-        is_admin = (user.reviewer and self.author != user) or (user.self_reviewer and self.author == user)
+        is_admin = self.env['res.partner.review'].search_count([
+            ('partner_id', '=', user.id),
+            ('repository_id', '=', self.repository.id),
+            ('review', '=', True) if self.author != user else ('self_review', '=', True),
+        ]) == 1
         is_reviewer = is_admin or self in user.delegate_reviewer
         # TODO: should delegate reviewers be able to retry PRs?
         is_author = is_reviewer or self.author == user

runbot_merge/models/res_partner.py

@@ -5,10 +5,9 @@ class Partner(models.Model):
     _inherit = 'res.partner'
 
     github_login = fields.Char()
-    reviewer = fields.Boolean(default=False, help="Can review PRs (maybe m2m to repos/branches?)")
-    self_reviewer = fields.Boolean(default=False, help="Can review own PRs (independent from reviewer)")
     delegate_reviewer = fields.Many2many('runbot_merge.pull_requests')
     formatted_email = fields.Char(string="commit email", compute='_rfc5322_formatted')
+    review_rights = fields.One2many('res.partner.review', 'partner_id')
 
     def _auto_init(self):
         res = super(Partner, self)._auto_init()
@@ -26,3 +25,17 @@ class Partner(models.Model):
             else:
                 email = ''
             partner.formatted_email = '%s <%s>' % (partner.name, email)
+
+class ReviewRights(models.Model):
+    _name = 'res.partner.review'
+    _description = "mapping of review rights between partners and repos"
+
+    partner_id = fields.Many2one('res.partner', required=True, ondelete='cascade')
+    repository_id = fields.Many2one('runbot_merge.repository', required=True)
+    review = fields.Boolean(default=False)
+    self_review = fields.Boolean(default=False)
+
+    def _auto_init(self):
+        res = super()._auto_init()
+        tools.create_unique_index(self._cr, 'runbot_merge_review_m2m', self._table, ['partner_id', 'repository_id'])
+        return res

runbot_merge/security/ir.model.access.csv

@@ -10,6 +10,7 @@ access_runbot_merge_split_admin,Admin access to splits,model_runbot_merge_split,
 access_runbot_merge_batch_admin,Admin access to batches,model_runbot_merge_batch,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_fetch_job_admin,Admin access to fetch jobs,model_runbot_merge_fetch_job,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_pull_requests_feedback_admin,Admin access to feedback,model_runbot_merge_pull_requests_feedback,runbot_merge.group_admin,1,1,1,1
+access_runbot_merge_review_rights,Admin access to review permissions,model_res_partner_review,runbot_merge.group_admin,1,1,1,1
 access_runbot_merge_project,User access to project,model_runbot_merge_project,base.group_user,1,0,0,0
 access_runbot_merge_repository,User access to repo,model_runbot_merge_repository,base.group_user,1,0,0,0
 access_runbot_merge_branch,User access to branches,model_runbot_merge_branch,base.group_user,1,0,0,0

runbot_merge/tests/test_basic.py

@@ -13,12 +13,13 @@ import odoo
 from test_utils import re_matches, get_partner, _simple_init
 
 @pytest.fixture
-def repo(project, make_repo):
+def repo(env, project, make_repo, users, setreviewers):
     r = make_repo('repo')
     project.write({'repo_ids': [(0, 0, {
         'name': r.name,
         'required_statuses': 'legal/cla,ci/runbot'
     })]})
+    setreviewers(*project.repo_ids)
     return r
 
 def test_trivial_flow(env, repo, page, users, config):

runbot_merge/tests/test_multirepo.py

@@ -12,30 +12,36 @@ import pytest
 from test_utils import re_matches, get_partner
 
 @pytest.fixture
-def repo_a(project, make_repo):
+def repo_a(project, make_repo, setreviewers):
     repo = make_repo('a')
-    project.write({'repo_ids': [(0, 0, {
+    r = project.env['runbot_merge.repository'].create({
+        'project_id': project.id,
         'name': repo.name,
         'required_statuses': 'legal/cla,ci/runbot'
-    })]})
+    })
+    setreviewers(r)
     return repo
 
 @pytest.fixture
-def repo_b(project, make_repo):
+def repo_b(project, make_repo, setreviewers):
     repo = make_repo('b')
-    project.write({'repo_ids': [(0, 0, {
+    r = project.env['runbot_merge.repository'].create({
+        'project_id': project.id,
         'name': repo.name,
         'required_statuses': 'legal/cla,ci/runbot'
-    })]})
+    })
+    setreviewers(r)
     return repo
 
 @pytest.fixture
-def repo_c(project, make_repo):
+def repo_c(project, make_repo, setreviewers):
     repo = make_repo('c')
-    project.write({'repo_ids': [(0, 0, {
+    r = project.env['runbot_merge.repository'].create({
+        'project_id': project.id,
         'name': repo.name,
         'required_statuses': 'legal/cla,ci/runbot'
-    })]})
+    })
+    setreviewers(r)
     return repo
 
 def make_pr(repo, prefix, trees, *, target='master', user,
@@ -744,3 +750,11 @@ def test_different_branches(env, project, repo_a, repo_b, config):
     env.run_crons()
 
     assert to_pr(env, pr_a).state == 'merged'
+
+def test_remove_acl(env, partners, repo_a, repo_b, repo_c):
+    """ Check that our way of deprovisioning works correctly
+    """
+    r = partners['self_reviewer']
+    assert r.mapped('review_rights.repository_id') == repo_a | repo_b | repo_c
+    r.write({'review_rights': [(5, 0, 0)]})
+    assert r.mapped('review_rights.repository_id') == env['runbot_merge.repository']

runbot_merge/views/res_partner.xml

@@ -10,9 +10,16 @@
                         <group>
                             <field name="github_login"/>
                         </group>
-                        <group>
-                            <field name="reviewer"/>
-                            <field name="self_reviewer"/>
+                    </group>
+                    <group>
+                        <group colspan="4" string="Review Rights">
+                            <field name="review_rights" nolabel="1">
+                                <tree string="Review ACLs" editable="bottom">
+                                    <field name="repository_id"/>
+                                    <field name="review"/>
+                                    <field name="self_review"/>
+                                </tree>
+                            </field>
                         </group>
                     </group>
                     <group>