diff --git a/runbot_merge/changelog/2022-06/branch.md b/runbot_merge/changelog/2022-06/branch.md new file mode 100644 index 00000000..6c7c302b --- /dev/null +++ b/runbot_merge/changelog/2022-06/branch.md @@ -0,0 +1,4 @@ +IMP: automatically close PRs when their target branch is deactivated + +Leave a message on the PRs to explain, such PRs should also be reopen-able if +the users wants to retarget them. diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 3b0b02c0..7e134af3 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -145,11 +145,11 @@ def handle_pr(env, event): message = None if not branch: - message = f"This PR targets the un-managed branch {r}:{b}, it can not be merged." + message = f"This PR targets the un-managed branch {r}:{b}, it needs to be retargeted before it can be merged." _logger.info("Ignoring event %s on PR %s#%d for un-managed branch %s", event['action'], r, pr['number'], b) elif not branch.active: - message = f"This PR targets the disabled branch {r}:{b}, it can not be merged." + message = f"This PR targets the disabled branch {r}:{b}, it needs to be retargeted before it can be merged." if message and event['action'] not in ('synchronize', 'closed'): feedback(message=message) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 5ced85e1..33a087b6 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -249,6 +249,23 @@ class Branch(models.Model): self._table, ['name', 'project_id']) return res + @api.depends('active') + def _compute_display_name(self): + super()._compute_display_name() + for b in self.filtered(lambda b: not b.active): + b.display_name += ' (inactive)' + + def write(self, vals): + super().write(vals) + if vals.get('active') is False: + self.env['runbot_merge.pull_requests.feedback'].create([{ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'close': True, + 'message': f'{pr.ping()}the target branch {pr.target.name!r} has been disabled, closing this PR.', + } for pr in self.prs]) + return True + @api.depends('staging_ids.active') def _compute_active_staging(self): for b in self: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 5049fcda..f039b606 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -6,7 +6,7 @@ import time from unittest import mock import pytest -from lxml import html +from lxml import html, etree import odoo from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of @@ -26,21 +26,34 @@ def repo(env, project, make_repo, users, setreviewers): def test_trivial_flow(env, repo, page, users, config): # create base branch with repo: - m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) - repo.make_ref('heads/master', m) + [m] = repo.make_commits(None, Commit("initial", tree={'a': 'some content'}), ref='heads/master') # create PR with 2 commits - c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) - c1 = repo.make_commit(c0, 'add file', None, tree={'a': 'some other content', 'b': 'a second file'}) - pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,) + _, c1 = repo.make_commits( + m, + Commit('replace file contents', tree={'a': 'some other content'}), + Commit('add file', tree={'b': 'a second file'}), + ref='heads/other' + ) + pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head='other') pr_id = to_pr(env, pr) assert pr_id.state == 'opened' env.run_crons() assert pr.comments == [seen(env, pr, users)] - s = pr_page(page, pr).cssselect('.alert-info > ul > li') + + pr_dashboard = pr_page(page, pr) + s = pr_dashboard.cssselect('.alert-info > ul > li') assert [it.get('class') for it in s] == ['fail', 'fail', ''],\ "merge method unset, review missing, no CI" + assert dict(zip( + [e.text_content() for e in pr_dashboard.cssselect('dl.runbot-merge-fields dt')], + [e.text_content() for e in pr_dashboard.cssselect('dl.runbot-merge-fields dd')], + )) == { + 'label': f"{config['github']['owner']}:other", + 'head': c1, + 'target': 'master', + } with repo: repo.post_status(c1, 'success', 'legal/cla') @@ -3190,7 +3203,7 @@ class TestUnknownPR: assert prx.comments == [ (users['reviewer'], 'hansen r+'), - (users['user'], "This PR targets the un-managed branch %s:branch, it can not be merged." % repo.name), + (users['user'], "This PR targets the un-managed branch %s:branch, it needs to be retargeted before it can be merged." % repo.name), (users['user'], "Branch `branch` is not within my remit, imma just ignore it."), ] diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index eaaca7c4..1d6be201 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -1,6 +1,6 @@ -from utils import seen, Commit +from utils import seen, Commit, pr_page -def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users): +def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users, page): """ PRs to disabled branches are ignored, but what if the PR exists *before* the branch is disabled? """ @@ -13,7 +13,8 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf repo_id = env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, - 'status_ids': [(0, 0, {'context': 'status'})] + 'status_ids': [(0, 0, {'context': 'status'})], + 'group_id': False, }) setreviewers(*project.repo_ids) @@ -25,42 +26,57 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'})) pr = repo.make_pr(title="title", body='body', target='other', head=c) repo.post_status(c, 'success', 'status') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() pr_id = env['runbot_merge.pull_requests'].search([ ('repository', '=', repo_id.id), ('number', '=', pr.number), ]) + branch_id = pr_id.target + assert pr_id.staging_id + staging_id = branch_id.active_staging_id + assert staging_id == pr_id.staging_id # disable branch "other" - project.branch_ids.filtered(lambda b: b.name == 'other').active = False - - # r+ the PR - with repo: - pr.post_comment('hansen r+', config['role_reviewer']['token']) + branch_id.active = False env.run_crons() - # nothing should happen, the PR should be unstaged forever, maybe? - assert pr_id.state == 'ready' + assert not branch_id.active_staging_id + assert staging_id.state == 'cancelled', \ + "closing the PRs should have canceled the staging" + + p = pr_page(page, pr) + target = dict(zip( + (e.text for e in p.cssselect('dl.runbot-merge-fields dt')), + (p.cssselect('dl.runbot-merge-fields dd')) + ))['target'] + assert target.text_content() == 'other (inactive)' + assert target.get('class') == 'text-muted bg-warning' + + # the PR should have been closed implicitly + assert pr_id.state == 'closed' assert not pr_id.staging_id + with repo: + pr.open() + pr.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr_id.state == 'ready', "pr should be reopenable" + env.run_crons() + + assert pr.comments == [ + (users['reviewer'], "hansen r+"), + seen(env, pr, users), + (users['user'], "@%(user)s @%(reviewer)s the target branch 'other' has been disabled, closing this PR." % users), + (users['reviewer'], "hansen r+"), + (users['user'], "This PR targets the disabled branch %s:other, it needs to be retargeted before it can be merged." % repo.name), + ] + with repo: [c2] = repo.make_commits(ot, Commit('wheee', tree={'b': '3'})) repo.update_ref(pr.ref, c2, force=True) assert pr_id.head == c2, "pr should be aware of its update" - with repo: - pr.close() - assert pr_id.state == 'closed', "pr should be closeable" - with repo: - pr.open() - assert pr_id.state == 'opened', "pr should be reopenable (state reset due to force push" - env.run_crons() - assert pr.comments == [ - (users['reviewer'], "hansen r+"), - seen(env, pr, users), - (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), - ], "reopening a PR to an inactive branch should send feedback, but not closing it" - with repo: pr.base = 'other2' repo.post_status(c2, 'success', 'status') @@ -96,7 +112,7 @@ def test_new_pr_no_branch(env, project, make_repo, setreviewers, users): ('number', '=', pr.number), ]), "the PR should not have been created in the backend" assert pr.comments == [ - (users['user'], "This PR targets the un-managed branch %s:other, it can not be merged." % repo.name), + (users['user'], "This PR targets the un-managed branch %s:other, it needs to be retargeted before it can be merged." % repo.name), ] def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): @@ -131,45 +147,6 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): assert pr_id, "the PR should have been created in the backend" assert pr_id.state == 'opened' assert pr.comments == [ - (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), + (users['user'], "This PR targets the disabled branch %s:other, it needs to be retargeted before it can be merged." % repo.name), seen(env, pr, users), ] - -def test_retarget_from_disabled(env, make_repo, project, setreviewers): - """ Retargeting a PR from a disabled branch should not duplicate the PR - """ - repo = make_repo('repo') - project.write({'branch_ids': [(0, 0, {'name': '1.0'}), (0, 0, {'name': '2.0'})]}) - repo_id = env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'required_statuses': 'legal/cla,ci/runbot', - }) - setreviewers(repo_id) - - with repo: - [c0] = repo.make_commits(None, Commit('0', tree={'a': '0'}), ref='heads/1.0') - [c1] = repo.make_commits(c0, Commit('1', tree={'a': '1'}), ref='heads/2.0') - repo.make_commits(c1, Commit('2', tree={'a': '2'}), ref='heads/master') - - # create PR on 1.0 - repo.make_commits(c0, Commit('c', tree={'a': '0', 'b': '0'}), ref='heads/changes') - prx = repo.make_pr(head='changes', target='1.0') - branch_1 = project.branch_ids.filtered(lambda b: b.name == '1.0') - # there should only be a single PR in the system at this point - [pr] = env['runbot_merge.pull_requests'].search([]) - assert pr.target == branch_1 - - # branch 1 is EOL, disable it - branch_1.active = False - - with repo: - # we forgot we had active PRs for it, and we may want to merge them - # still, retarget them! - prx.base = '2.0' - - # check that we still only have one PR in the system - [pr_] = env['runbot_merge.pull_requests'].search([]) - # and that it's the same as the old one, just edited with a new target - assert pr_ == pr - assert pr.target == project.branch_ids.filtered(lambda b: b.name == '2.0') diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml index 52ca2eed..0e87428d 100644 --- a/runbot_merge/views/templates.xml +++ b/runbot_merge/views/templates.xml @@ -404,11 +404,14 @@ open +
label
head
+
target
+