diff --git a/forwardport/changelog/2022-06/closed.md b/forwardport/changelog/2022-06/closed.md new file mode 100644 index 00000000..51b53e60 --- /dev/null +++ b/forwardport/changelog/2022-06/closed.md @@ -0,0 +1 @@ +IMP: notifications when reopening a closed forward-port (e.g. indicate that they're detached) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index f9653cbb..3c2742ab 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -240,6 +240,7 @@ class PullRequests(models.Model): # again 2 PRs with same head is weird so... newhead = vals.get('head') with_parents = self.filtered('parent_id') + closed_fp = self.filtered(lambda p: p.state == 'closed' and p.source_id) if newhead and not self.env.context.get('ignore_head_update') and newhead != self.head: vals.setdefault('parent_id', False) # if any children, this is an FP PR being updated, enqueue @@ -268,6 +269,17 @@ class PullRequests(models.Model): ), 'token_field': 'fp_github_token', }) + for p in closed_fp.filtered(lambda p: p.state != 'closed'): + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': p.repository.id, + 'pull_request': p.number, + 'message': "%sthis PR was closed then reopened. " + "It should be merged the normal way (via @%s)" % ( + p.source_id.ping(), + p.repository.project_id.github_prefix, + ), + 'token_field': 'fp_github_token', + }) if vals.get('state') == 'merged': for p in self: self.env['forwardport.branch_remover'].create({ @@ -285,6 +297,7 @@ class PullRequests(models.Model): r = super()._try_closing(by) if r: self.with_context(forwardport_detach_warn=False).parent_id = False + self.search([('parent_id', '=', self.id)]).parent_id = False return r def _parse_commands(self, author, comment, login): @@ -330,6 +343,11 @@ class PullRequests(models.Model): msg = "I can only do this on forward-port PRs and this is not one, see {}.".format( self.repository.project_id.github_prefix ) + elif not self.parent_id: + ping = True + msg = "I can only do this on unmodified forward-port PRs, ask {}.".format( + self.repository.project_id.github_prefix + ) else: merge_bot = self.repository.project_id.github_prefix # don't update the root ever diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 43fb17c5..172cee51 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -798,11 +798,12 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port """) ] - def test_closing_after_fp(self, env, config, make_repo): + def test_closing_after_fp(self, env, config, make_repo, users): """ Closing a PR which has been forward-ported should not touch the followups """ prod, other = make_basic(env, config, make_repo) + project = env['runbot_merge.project'].search([]) with prod: [p_1] = prod.make_commits( 'a', @@ -822,23 +823,51 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port # should merge the staging then create the FP PR env.run_crons() - pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') with prod: - prod.post_status(pr1.head, 'success', 'legal/cla') - prod.post_status(pr1.head, 'success', 'ci/runbot') + prod.post_status(pr1_id.head, 'success', 'legal/cla') + prod.post_status(pr1_id.head, 'success', 'ci/runbot') # should create the second staging env.run_crons() - pr0_1, pr1_1, pr2_1 = env['runbot_merge.pull_requests'].search([], order='number') - assert pr0_1 == pr0 - assert pr1_1 == pr1 + pr0_id2, pr1_id2, pr2_id = env['runbot_merge.pull_requests'].search([], order='number') + assert pr0_id2 == pr0_id + assert pr1_id2 == pr1_id + + pr1 = prod.get_pr(pr1_id.number) + with prod: + pr1.close() + + assert pr1_id.state == 'closed' + assert not pr1_id.parent_id + assert pr2_id.state == 'opened' + assert not pr2_id.parent_id, \ + "the descendant of a closed PR doesn't really make sense, maybe?" with prod: - prod.get_pr(pr1.number).close() + pr1.open() + assert pr1_id.state == 'validated' + env.run_crons() + assert pr1.comments[-1] == ( + users['user'], + "@{} @{} this PR was closed then reopened. " + "It should be merged the normal way (via @{})".format( + users['user'], + users['reviewer'], + project.github_prefix, + ) + ) - assert pr1_1.state == 'closed' - assert not pr1_1.parent_id - assert pr2_1.state == 'opened' + with prod: + pr1.post_comment(f'{project.fp_github_name} r+', config['role_reviewer']['token']) + env.run_crons() + assert pr1.comments[-1] == ( + users['user'], + "@{} I can only do this on unmodified forward-port PRs, ask {}.".format( + users['reviewer'], + project.github_prefix, + ), + ) class TestBranchDeletion: def test_delete_normal(self, env, config, make_repo):