diff --git a/conftest.py b/conftest.py index 370a521e..140a2b67 100644 --- a/conftest.py +++ b/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) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index e09053b0..7f5bd2b5 100644 --- a/forwardport/tests/test_simple.py +++ b/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'] diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 990bcf72..1037a27d 100644 --- a/forwardport/tests/test_weird.py +++ b/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): diff --git a/forwardport/tests/utils.py b/forwardport/tests/utils.py index f126f053..4c95dd93 100644 --- a/forwardport/tests/utils.py +++ b/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 diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index 5d38d16f..ba8809ba 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -1,6 +1,6 @@ { 'name': 'merge bot', - 'version': '1.1', + 'version': '1.2', 'depends': ['contacts', 'website'], 'data': [ 'security/security.xml', diff --git a/runbot_merge/controllers/reviewer_provisioning.py b/runbot_merge/controllers/reviewer_provisioning.py index 785001c9..75abf463 100644 --- a/runbot_merge/controllers/reviewer_provisioning.py +++ b/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 diff --git a/runbot_merge/migrations/13.0.1.2/pre-migration.py b/runbot_merge/migrations/13.0.1.2/pre-migration.py new file mode 100644 index 00000000..840df84c --- /dev/null +++ b/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 + """) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 98287b86..2f00484c 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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 diff --git a/runbot_merge/models/res_partner.py b/runbot_merge/models/res_partner.py index 6fddbb06..80417fc9 100644 --- a/runbot_merge/models/res_partner.py +++ b/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 diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index fb31ca5b..87191796 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/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 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index a5bbe1f4..5443a4ba 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 0aceee03..5c10fd86 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/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'] diff --git a/runbot_merge/views/res_partner.xml b/runbot_merge/views/res_partner.xml index ea552d97..c1f62627 100644 --- a/runbot_merge/views/res_partner.xml +++ b/runbot_merge/views/res_partner.xml @@ -10,9 +10,16 @@ - - - + + + + + + + + + +