From 4a40c0338ca0d170300218fb20834af4581bfbf2 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 15 Jul 2024 15:14:23 +0200 Subject: [PATCH] [ADD] runbot_merge: small wizard to split a PR off of its batch --- runbot_merge/models/pull_requests.py | 46 +++++++++++++++++++- runbot_merge/security/ir.model.access.csv | 1 + runbot_merge/tests/test_batch_consistency.py | 41 ++++++++++++++++- runbot_merge/views/mergebot.xml | 20 ++++++++- 4 files changed, 103 insertions(+), 5 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 7e8e312a..649332d6 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -19,7 +19,7 @@ import werkzeug from markupsafe import Markup from odoo import api, fields, models, tools, Command -from odoo.exceptions import AccessError +from odoo.exceptions import AccessError, UserError from odoo.osv import expression from odoo.tools import html_escape, Reverse from . import commands @@ -318,6 +318,19 @@ class Branch(models.Model): b.active_staging_id = b.with_context(active_test=True).staging_ids +class SplitOffWizard(models.TransientModel): + _name = "runbot_merge.pull_requests.split_off" + _description = "wizard to split a PR off of its current batch and into a different one" + + pr_id = fields.Many2one("runbot_merge.pull_requests", required=True) + new_label = fields.Char(string="New Label") + + def button_apply(self): + self.pr_id._split_off(self.new_label) + self.unlink() + return {'type': 'ir.actions.act_window_close'} + + ACL = collections.namedtuple('ACL', 'is_admin is_reviewer is_author') class PullRequests(models.Model): _name = 'runbot_merge.pull_requests' @@ -1605,6 +1618,37 @@ For your own safety I've ignored *everything in your entire comment*. format_args=format_args, ) + def button_split(self): + if len(self.batch_id.prs) == 1: + raise UserError("Splitting a batch with a single PR is dumb") + + w = self.env['runbot_merge.pull_requests.split_off'].create({ + 'pr_id': self.id, + 'new_label': self.label, + }) + return { + 'type': 'ir.actions.act_window', + 'res_model': w._name, + 'res_id': w.id, + 'target': 'new', + 'views': [(False, 'form')], + } + + def _split_off(self, new_label): + # should not be usable to move a PR between batches (maybe later) + batch = self.env['runbot_merge.batch'] + if not re.search(r':patch-\d+$', new_label): + if batch.search([ + ('merge_date', '=', False), + ('prs.label', '=', new_label), + ]): + raise UserError("Can not split off to an existing batch") + + self.write({ + 'label': new_label, + 'batch_id': batch.create({}).id, + }) + # ordering is a bit unintuitive because the lowest sequence (and name) # is the last link of the fp chain, reasoning is a bit more natural the # other way around (highest object is the last), especially with Python diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 9d2da846..76bbf09d 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -9,6 +9,7 @@ access_runbot_merge_repository_status_admin,Admin access to repo statuses,model_ access_runbot_merge_branch_admin,Admin access to branches,model_runbot_merge_branch,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_pull_requests_admin,Admin access to PR,model_runbot_merge_pull_requests,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_pull_requests_tagging_admin,Admin access to tagging,model_runbot_merge_pull_requests_tagging,runbot_merge.group_admin,1,1,1,1 +access_runbot_merge_pull_requests_split_admin,Admin access to batch split wizard,model_runbot_merge_pull_requests_split_off,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_commit_admin,Admin access to commits,model_runbot_merge_commit,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_stagings_admin,Admin access to stagings,model_runbot_merge_stagings,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_stagings_heads_admin,Admin access to staging heads,model_runbot_merge_stagings_heads,runbot_merge.group_admin,1,1,1,1 diff --git a/runbot_merge/tests/test_batch_consistency.py b/runbot_merge/tests/test_batch_consistency.py index 8855bfbb..18e66d4c 100644 --- a/runbot_merge/tests/test_batch_consistency.py +++ b/runbot_merge/tests/test_batch_consistency.py @@ -1,6 +1,8 @@ """This module tests edge cases specific to the batch objects themselves, without wider relevance and thus other location. """ +import pytest + from utils import Commit, to_pr @@ -79,7 +81,7 @@ def test_close_multiple(env, make_repo2): assert not batch_id.active assert Batches.search_count([]) == 0 -def test_inconsistent_target(env, project, make_repo2): +def test_inconsistent_target(env, project, make_repo2, users): """If a batch's PRs have inconsistent targets, - only open PRs should count @@ -87,6 +89,7 @@ def test_inconsistent_target(env, project, make_repo2): - the dash should not get hopelessly lost - there should be a wizard to split the batch / move a PR to a separate batch """ + # region setup Batches = env['runbot_merge.batch'] repo1 = make_repo2('whe') repo2 = make_repo2('whee') @@ -99,6 +102,9 @@ def test_inconsistent_target(env, project, make_repo2): repo1.make_commits('master', Commit('b', tree={"b": "b"}), ref='heads/a_pr') pr1 = repo1.make_pr(head='a_pr', target='master') + repo1.make_commits('master', Commit('b', tree={"c": "c"}), ref='heads/something_else') + pr_other = repo1.make_pr(head='something_else', target='master') + with repo2: repo2.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master') repo2.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/other') @@ -111,7 +117,13 @@ def test_inconsistent_target(env, project, make_repo2): repo3.make_commits('master', Commit('b', tree={"b": "b"}), ref='heads/a_pr') pr3 = repo3.make_pr(head='a_pr', target='master') - [b] = Batches.search([]) + assert repo1.owner == repo2.owner == repo3.owner + owner = repo1.owner + # endregion + + # region closeable consistency + + [b] = Batches.search([('all_prs.label', '=', f'{owner}:a_pr')]) assert b.target.name == 'master' assert len(b.prs) == 3 assert len(b.all_prs) == 3 @@ -127,3 +139,28 @@ def test_inconsistent_target(env, project, make_repo2): assert b.target.name == 'master' assert len(b.prs) == 2 assert len(b.all_prs) == 3 + # endregion + + # region split batch + with repo2: + pr2.base = 'other' + assert b.target.name == False + assert to_pr(env, pr_other).label == f'{owner}:something_else' + pr2_id = to_pr(env, pr2) + act = pr2_id.button_split() + assert act['type'] == 'ir.actions.act_window' + assert act['views'] == [[False, 'form']] + assert act['target'] == 'new' + w = env[act['res_model']].browse([act['res_id']]) + w.new_label = f"{owner}:something_else" + with pytest.raises(Exception): + w.button_apply() + w.new_label = f"{owner}:blah-blah-blah" + w.button_apply() + + assert pr2_id.label == f"{owner}:blah-blah-blah" + assert pr2_id.batch_id != to_pr(env, pr1).batch_id + assert b.target.name == 'master' + assert len(b.prs) == 1, "the PR has been moved off of this batch entirely" + assert len(b.all_prs) == 2 + # endregion diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml index eecec149..66888469 100644 --- a/runbot_merge/views/mergebot.xml +++ b/runbot_merge/views/mergebot.xml @@ -125,8 +125,10 @@
- - +
@@ -261,6 +263,20 @@ + + Split Off Form + runbot_merge.pull_requests.split_off + + + +
+
+ + + + Stagings runbot_merge.stagings