From 302fd42cae2cbe54778d018aa8fafa51c201dfec Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 29 Aug 2023 15:59:05 +0200 Subject: [PATCH] [ADD] forwardport: message on parent of detached PR Currently a user is not notified that the parent of a detached PR needs to be independently approved and may miss that information. Add a notification to *that* PR as well. Fixes #788 --- forwardport/models/project.py | 38 ++++++++----- forwardport/tests/test_updates.py | 56 ++++++++++++++++++- ..._merge.pull_requests.feedback.template.csv | 4 ++ 3 files changed, 82 insertions(+), 16 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index dd91d07a..498c8476 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -29,7 +29,7 @@ from pathlib import Path import dateutil.relativedelta import requests -from odoo import _, models, fields, api +from odoo import models, fields, api from odoo.osv import expression from odoo.exceptions import UserError from odoo.tools.misc import topological_sort, groupby @@ -303,7 +303,11 @@ class PullRequests(models.Model): # also a bit odd to only handle updating 1 head at a time, but then # again 2 PRs with same head is weird so... newhead = vals.get('head') - with_parents = self.filtered('parent_id') + with_parents = { + p: p.parent_id + for p in self + if p.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) @@ -323,14 +327,21 @@ class PullRequests(models.Model): vals['merge_date'] = fields.Datetime.now() r = super().write(vals) if self.env.context.get('forwardport_detach_warn', True): - for p in with_parents: - if not p.parent_id: - self.env.ref('runbot_merge.forwardport.update.detached')._send( - repository=p.repository, - pull_request=p.number, - token_field='fp_github_token', - format_args={'pr': p}, - ) + for p, parent in with_parents.items(): + if p.parent_id: + continue + self.env.ref('runbot_merge.forwardport.update.detached')._send( + repository=p.repository, + pull_request=p.number, + token_field='fp_github_token', + format_args={'pr': p}, + ) + self.env.ref('runbot_merge.forwardport.update.parent')._send( + repository=parent.repository, + pull_request=parent.number, + token_field='fp_github_token', + format_args={'pr': parent, 'child': p}, + ) for p in closed_fp.filtered(lambda p: p.state != 'closed'): self.env.ref('runbot_merge.forwardport.reopen.detached')._send( repository=p.repository, @@ -927,11 +938,8 @@ class PullRequests(models.Model): head_commit = commits[-1]['commit'] to_tuple = operator.itemgetter('name', 'email') - to_dict = lambda term, vals: { - 'GIT_%s_NAME' % term: vals[0], - 'GIT_%s_EMAIL' % term: vals[1], - 'GIT_%s_DATE' % term: vals[2], - } + def to_dict(term, vals): + return {'GIT_%s_NAME' % term: vals[0], 'GIT_%s_EMAIL' % term: vals[1], 'GIT_%s_DATE' % term: vals[2]} authors, committers = set(), set() for c in (c['commit'] for c in commits): authors.add(to_tuple(c['author'])) diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 1635fc5e..c83afde0 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -15,6 +15,14 @@ def test_update_pr(env, config, make_repo, users): only this one and its dependent should be updated? """ prod, _ = make_basic(env, config, make_repo) + # create a branch d from c so we can have 3 forward ports PRs, not just 2, + # for additional checks + env['runbot_merge.project'].search([]).write({ + 'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})] + }) + with prod: + prod.make_commits('c', Commit('1111', tree={'i': 'a'}), ref='heads/d') + with prod: [p_1] = prod.make_commits( 'a', @@ -37,7 +45,7 @@ def test_update_pr(env, config, make_repo, users): pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') fp_intermediate = (users['user'], '''\ -This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. +This PR targets b and is part of the forward-port chain. Further PRs will be created up to d. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port ''') @@ -121,6 +129,52 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port 'h': 'a', 'x': '5' }, "the followup FP should also have the update" + + with prod: + prod.post_status(pr2_id.head, 'success', 'ci/runbot') + prod.post_status(pr2_id.head, 'success', 'legal/cla') + env.run_crons() + + _0, _1, _2, pr3_id = env['runbot_merge.pull_requests'].search([], order='number') + assert pr3_id.parent_id == pr2_id + # don't bother updating heads (?) + pr3_id.write({'parent_id': False, 'detach_reason': "testing"}) + # pump feedback messages + env.run_crons() + + pr3 = prod.get_pr(pr3_id.number) + assert pr3.comments == [ + seen(env, pr3, users), + (users['user'], f"""\ +@{users['user']} @{users['reviewer']} this PR targets d and is the last of the forward-port chain containing: +* {pr2_id.display_name} + +To merge the full chain, use +> @{pr3_id.repository.project_id.fp_github_name} r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +"""), + (users['user'], f"@{users['user']} @{users['reviewer']} this PR was " + f"modified / updated and has become a normal PR. It " + f"should be merged the normal way " + f"(via @{pr3_id.repository.project_id.github_prefix})" + ) + ] + pr2 = prod.get_pr(pr2_id.number) + assert pr2.comments == [ + seen(env, pr2, users), + (users['user'], """\ +This PR targets c and is part of the forward-port chain. Further PRs will be created up to d. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +"""), + (users['user'], f"@{users['user']} @{users['reviewer']} child PR " + f"{pr3_id.display_name} was modified / updated and has " + f"become a normal PR. This PR (and any of its parents) " + f"will need to be merged independently as approvals " + f"won't cross."), + ] + def test_update_merged(env, make_repo, config, users): """ Strange things happen when an FP gets closed / merged but then its diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv index 70ed7397..8524d290 100644 --- a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv +++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv @@ -110,6 +110,10 @@ stderr: markdown-formatted stderr of git, if any" runbot_merge.forwardport.update.detached,{pr.ping}this PR was modified / updated and has become a normal PR. It should be merged the normal way (via @{pr.repository.project_id.github_prefix}),"Comment when a forwardport PR gets updated, documents that the PR now needs to be merged the “normal” way. pr: the pr in question " +runbot_merge.forwardport.update.parent,{pr.ping}child PR {child.display_name} was modified / updated and has become a normal PR. This PR (and any of its parents) will need to be merged independently as approvals won't cross.,"Sent to an open PR when its direct child has been detached. + +pr: the pr +child: its detached child" runbot_merge.forwardport.reopen.detached,{pr.ping}this PR was closed then reopened. It should be merged the normal way (via @{pr.repository.project_id.github_prefix}),"Comment when a forwardport PR gets closed then reopened, documents that the PR is now in a detached state. pr: the pr in question"