From 47e8b5b0142459c460967b45a08c0c4d72fc6eac Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 12 Nov 2020 13:17:37 +0100 Subject: [PATCH] [IMP] runbot_merge: overridable CI Convert overridable CI to an m2m from partners, it's significantly more convenient to manipulate as multiple users can (and likely will) have access to the same overrides, add a name_search so the override is easy to find from a partner, and provide a view for the overrides (with partners as tags). Also make the repository optional on CI overrides. Fixes #420 --- conftest.py | 6 + runbot_merge/__manifest__.py | 2 +- .../migrations/13.0.1.6/pre-migration.py | 39 +++++ runbot_merge/models/pull_requests.py | 2 +- runbot_merge/models/res_partner.py | 25 ++- runbot_merge/tests/test_oddities.py | 64 -------- runbot_merge/tests/test_status_overrides.py | 147 ++++++++++++++++++ runbot_merge/views/mergebot.xml | 20 +++ 8 files changed, 234 insertions(+), 71 deletions(-) create mode 100644 runbot_merge/migrations/13.0.1.6/pre-migration.py create mode 100644 runbot_merge/tests/test_status_overrides.py diff --git a/conftest.py b/conftest.py index b5e3da6c..c9c95044 100644 --- a/conftest.py +++ b/conftest.py @@ -1040,6 +1040,9 @@ class Model: ids = self._env(self._model, 'search', *args, **kwargs) return Model(self._env, self._model, ids) + def name_search(self, *args, **kwargs): + return self._env(self._model, 'name_search', *args, **kwargs) + def create(self, values): return Model(self._env, self._model, [self._env(self._model, 'create', values)]) @@ -1049,6 +1052,9 @@ class Model: def read(self, fields): return self._env(self._model, 'read', self._ids, fields) + def name_get(self): + return self._env(self._model, 'name_get', self._ids) + def unlink(self): return self._env(self._model, 'unlink', self._ids) diff --git a/runbot_merge/__manifest__.py b/runbot_merge/__manifest__.py index 2aeca524..f990e6d1 100644 --- a/runbot_merge/__manifest__.py +++ b/runbot_merge/__manifest__.py @@ -1,6 +1,6 @@ { 'name': 'merge bot', - 'version': '1.5', + 'version': '1.6', 'depends': ['contacts', 'website'], 'data': [ 'security/security.xml', diff --git a/runbot_merge/migrations/13.0.1.6/pre-migration.py b/runbot_merge/migrations/13.0.1.6/pre-migration.py new file mode 100644 index 00000000..a7903018 --- /dev/null +++ b/runbot_merge/migrations/13.0.1.6/pre-migration.py @@ -0,0 +1,39 @@ +import collections + + +def migrate(cr, version): + """ Status overrides: o2m -> m2m + """ + # create link table + cr.execute(''' + CREATE TABLE res_partner_res_partner_override_rel ( + res_partner_id integer not null references res_partner (id) ON DELETE CASCADE, + res_partner_override_id integer not null references res_partner_override (id) ON DELETE CASCADE, + primary key (res_partner_id, res_partner_override_id) + ) + ''') + cr.execute(''' + CREATE UNIQUE INDEX ON res_partner_res_partner_override_rel + (res_partner_override_id, res_partner_id) + ''') + + # deduplicate override rights and insert into link table + cr.execute('SELECT array_agg(id), array_agg(partner_id)' + ' FROM res_partner_override GROUP BY repository_id, context') + links = {} + duplicants = set() + for [keep, *drops], partners in cr.fetchall(): + links[keep] = partners + duplicants.update(drops) + for override_id, partner_ids in links.items(): + for partner_id in partner_ids: + cr.execute('INSERT INTO res_partner_res_partner_override_rel (res_partner_override_id, res_partner_id)' + ' VALUES (%s, %s)', [override_id, partner_id]) + # drop dups + cr.execute('DELETE FROM res_partner_override WHERE id = any(%s)', [list(duplicants)]) + + # remove old partner field + cr.execute('ALTER TABLE res_partner_override DROP COLUMN partner_id') + # add constraint to overrides + cr.execute('CREATE UNIQUE INDEX res_partner_override_unique ON res_partner_override ' + '(context, coalesce(repository_id, 0))') diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 37890cf3..f571e99f 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -991,7 +991,7 @@ class PullRequests(models.Model): }) elif command == 'override': overridable = author.override_rights\ - .filtered(lambda r: r.repository_id == self.repository)\ + .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\ .mapped('context') if param in overridable: self.overrides = json.dumps({ diff --git a/runbot_merge/models/res_partner.py b/runbot_merge/models/res_partner.py index 70860613..cdb56aea 100644 --- a/runbot_merge/models/res_partner.py +++ b/runbot_merge/models/res_partner.py @@ -13,7 +13,7 @@ class Partner(models.Model): 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') - override_rights = fields.One2many('res.partner.override', 'partner_id') + override_rights = fields.Many2many('res.partner.override') def _auto_init(self): res = super(Partner, self)._auto_init() @@ -71,19 +71,34 @@ class ReviewRights(models.Model): for r in self ] + @api.model def name_search(self, name='', args=None, operator='ilike', limit=100): - return self.search(args + [('repository_id.name', operator, name)], limit=limit).name_get() + return self.search((args or []) + [('repository_id.name', operator, name)], limit=limit).name_get() class OverrideRights(models.Model): _name = 'res.partner.override' _description = 'lints which the partner can override' - partner_id = fields.Many2one('res.partner', required=True, ondelete='cascade') - repository_id = fields.Many2one('runbot_merge.repository', required=True) + partner_ids = fields.Many2many('res.partner') + repository_id = fields.Many2one('runbot_merge.repository') context = fields.Char(required=True) + def init(self): + super().init() + tools.create_unique_index( + self.env.cr, 'res_partner_override_unique', self._table, + ['context', 'coalesce(repository_id, 0)'] + ) + + @api.model + def name_search(self, name='', args=None, operator='ilike', limit=100): + return self.search((args or []) + [ + '|', ('context', operator, name), + ('repository_id.name', operator, name) + ], limit=limit).name_get() + def name_get(self): return [ - (r.id, f'{r.repository_id.name}: {r.context}') + (r.id, f'{r.repository_id.name}: {r.context}' if r.repository_id else r.context) for r in self ] diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 6d6e6b98..39526d9f 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -25,67 +25,3 @@ def test_partner_merge(env): assert not p_src.exists() assert p_dest.name == 'Partner P. Partnersson' assert p_dest.github_login == 'xxx' - -def test_override(env, project, make_repo, users, setreviewers, config): - """ - Test that we can override a status on a PR: - - * @mergebot override context=status - * target url should be the comment (?) - * description should be overridden by - """ - repo = make_repo('repo') - repo_id = env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'status_ids': [(0, 0, {'context': 'l/int'})] - }) - setreviewers(*project.repo_ids) - # "other" can override the lint - env['res.partner'].create({ - 'name': config['role_other'].get('name', 'Other'), - 'github_login': users['other'], - 'override_rights': [(0, 0, { - 'repository_id': repo_id.id, - 'context': 'l/int', - })] - }) - - with repo: - m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') - - repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change') - pr = repo.make_pr(target='master', title='super change', head='change') - pr.post_comment('hansen r+', config['role_reviewer']['token']) - env.run_crons() - - pr_id = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', pr.number) - ]) - assert pr_id.state == 'approved' - - with repo: - pr.post_comment('hansen override=l/int', config['role_reviewer']['token']) - env.run_crons() - assert pr_id.state == 'approved' - - with repo: - pr.post_comment('hansen override=l/int', config['role_other']['token']) - env.run_crons() - assert pr_id.state == 'ready' - - comments = pr.comments - assert comments == [ - (users['reviewer'], 'hansen r+'), - (users['reviewer'], 'hansen override=l/int'), - (users['user'], "I'm sorry, @{}. You are not allowed to override this status.".format(users['reviewer'])), - (users['other'], "hansen override=l/int"), - ] - assert pr_id.statuses == '{}' - assert json.loads(pr_id.overrides) == {'l/int': { - 'state': 'success', - 'target_url': comments[-1]['html_url'], - 'description': 'Overridden by @{}'.format(users['other']), - }} - diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py new file mode 100644 index 00000000..6fe50a74 --- /dev/null +++ b/runbot_merge/tests/test_status_overrides.py @@ -0,0 +1,147 @@ +import json + +import pytest + +from utils import Commit + + +def test_no_duplicates(env): + """ Should not have two override records for the same (context, repo) + """ + Overrides = env['res.partner.override'] + Overrides.create({'context': 'a'}) + with pytest.raises(Exception, match=r'already exists'): + Overrides.create({'context': 'a'}) + +def name_search(Model, name): + """ Convenience function to return a recordset instead of a craplist + """ + return Model.browse(id_ for id_, _ in Model.name_search(name)) +def test_finding(env): + project = env['runbot_merge.project'].create({ + 'name': 'test', + 'github_token': 'xxx', 'github_prefix': 'no', + }) + repo_1 = env['runbot_merge.repository'].create({'project_id': project.id, 'name': 'r1'}) + repo_2 = env['runbot_merge.repository'].create({'project_id': project.id, 'name': 'r2'}) + + Overrides = env['res.partner.override'] + a = Overrides.create({'context': 'ctx1'}) + b = Overrides.create({'context': 'ctx1', 'repository_id': repo_1.id}) + c = Overrides.create({'context': 'ctx1', 'repository_id': repo_2.id}) + d = Overrides.create({'context': 'ctx2', 'repository_id': repo_2.id}) + + assert name_search(Overrides, 'ctx1') == a|b|c + assert name_search(Overrides, 'ctx') == a|b|c|d + assert name_search(Overrides, 'r2') == c|d + +def test_basic(env, project, make_repo, users, setreviewers, config): + """ + Test that we can override a status on a PR: + + * @mergebot override context=status + * target url should be the comment (?) + * description should be overridden by + """ + repo = make_repo('repo') + repo_id = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'l/int'})] + }) + setreviewers(*project.repo_ids) + # "other" can override the lint + env['res.partner'].create({ + 'name': config['role_other'].get('name', 'Other'), + 'github_login': users['other'], + 'override_rights': [(0, 0, { + 'repository_id': repo_id.id, + 'context': 'l/int', + })] + }) + + with repo: + m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + + repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change') + pr = repo.make_pr(target='master', title='super change', head='change') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr.number) + ]) + assert pr_id.state == 'approved' + + with repo: + pr.post_comment('hansen override=l/int', config['role_reviewer']['token']) + env.run_crons() + assert pr_id.state == 'approved' + + with repo: + pr.post_comment('hansen override=l/int', config['role_other']['token']) + env.run_crons() + assert pr_id.state == 'ready' + + comments = pr.comments + assert comments == [ + (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen override=l/int'), + (users['user'], "I'm sorry, @{}. You are not allowed to override this status.".format(users['reviewer'])), + (users['other'], "hansen override=l/int"), + ] + assert pr_id.statuses == '{}' + assert json.loads(pr_id.overrides) == {'l/int': { + 'state': 'success', + 'target_url': comments[-1]['html_url'], + 'description': 'Overridden by @{}'.format(users['other']), + }} + +def test_no_repository(env, project, make_repo, users, setreviewers, config): + """ A repo missing from an override allows overriding the status in every repo + """ + repo = make_repo('repo') + env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'l/int'})] + }) + setreviewers(*project.repo_ids) + # "other" can override the lint + env['res.partner'].create({ + 'name': config['role_other'].get('name', 'Other'), + 'github_login': users['other'], + 'override_rights': [(0, 0, {'context': 'l/int'})] + }) + + with repo: + m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + + repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change') + pr = repo.make_pr(target='master', title='super change', head='change') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr.number) + ]) + assert pr_id.state == 'approved' + + with repo: + pr.post_comment('hansen override=l/int', config['role_other']['token']) + env.run_crons() + assert pr_id.state == 'ready' + + comments = pr.comments + assert comments == [ + (users['reviewer'], 'hansen r+'), + (users['other'], "hansen override=l/int"), + ] + assert pr_id.statuses == '{}' + assert json.loads(pr_id.overrides) == {'l/int': { + 'state': 'success', + 'target_url': comments[-1]['html_url'], + 'description': 'Overridden by @{}'.format(users['other']), + }} diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml index 104bbbe4..e3fe9fbb 100644 --- a/runbot_merge/views/mergebot.xml +++ b/runbot_merge/views/mergebot.xml @@ -277,6 +277,22 @@ + + CI / statuses overrides + res.partner.override + + + Overrides List + res.partner.override + + + + + + + + + + +