[FIX] forwardport: clarify forward port message

The ping message was not clear
- don't know if anything is needed from the author
- don't know if the last step in the chain

Ping the authors only when something needs to be done (error or last
step).

Do not ping non-reviewers as they will not have the rights to r+ or
modify the PR anyway

Closes #192

forwardport/models/project.py

@@ -387,24 +387,6 @@ class PullRequests(models.Model):
             message += "Forward-Port-Of: %s#%s" % (source.repository.name, source.number)
 
             (h, out, err) = conflicts.get(pr) or (None, None, None)
-            if h:
-                message = """Cherrypicking %s of source #%d failed with the following
-
-stdout:
-```
-%s
-```
-
-stderr:
-```
-%s
-```
-
-Either perform the forward-port manually (and push to this branch, proceeding
-as usual) or close this PR (maybe?).
-
-In the former case, you may want to edit this PR message as well.
-""" % (h, source.number, out, err)
 
             r = requests.post(
                 'https://api.github.com/repos/{}/pulls'.format(pr.repository.name), json={
@@ -433,11 +415,49 @@ In the former case, you may want to edit this PR message as well.
                 # only link to previous PR of sequence if cherrypick passed
                 'parent_id': pr.id if not has_conflicts else False,
             })
-            assignees = (new_pr.source_id.author | new_pr.source_id.reviewed_by).mapped('github_login')
+
+            assignees = (new_pr.source_id.author | new_pr.source_id.reviewed_by) \
+                .filtered(lambda p: new_pr.source_id._pr_acl(p).is_reviewer) \
+                .mapped('github_login')
+            ping = "Ping %s" % ', '.join('@' + login for login in assignees if login)
+            if h:
+                sout = serr = ''
+                if out.strip():
+                    sout = "\nstdout:\n```\n%s\n```\n" % out
+                if err.strip():
+                    serr = "\nstderr:\n```\n%s\n```\n" % err
+
+                message = ping + """
+Cherrypicking %s of source #%d failed
+%s%s
+Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
+
+In the former case, you may want to edit this PR message as well.
+""" % (h, source.number, sout, serr)
+            elif base.limit_id == target:
+                ancestors = "".join(
+                    "* %s#%d\n" % (p.repository.name, p.number)
+                    for p in pr._iter_ancestors()
+                    if p.parent_id and p.parent_id != source
+                )
+                message = ping + """
+This PR targets %s and is the last of the forward-port chain%s
+%s
+To merge the full chain, say
+> @%s r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""" % (target.name, 'containing:' if ancestors else '.', ancestors, pr.repository.project_id.fp_github_name)
+            else:
+                message = """\
+This PR targets %s and is part of the forward-port chain. Further PRs will be created up to %s.
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""" % (target.name, base.limit_id.name)
             self.env['runbot_merge.pull_requests.feedback'].create({
                 'repository': new_pr.repository.id,
                 'pull_request': new_pr.number,
-                'message': "Ping %s" % ', '.join('@' + login for login in assignees if login),
+                'message': message,
             })
             # not great but we probably want to avoid the risk of the webhook
             # creating the PR from under us. There's still a "hole" between

forwardport/tests/test_simple.py

@@ -182,11 +182,15 @@ def test_straightforward_flow(env, config, make_repo, users):
         'x': '1'
     }
     assert prod.get_pr(pr2.number).comments == [
-        (users['user'], "Ping @%s, @%s" % (users['other'], users['reviewer'])),
+        (users['user'], """\
-        (users['user'], "This forward port PR is awaiting action"),
+Ping @%s
-    ]
+This PR targets c and is the last of the forward-port chain.
-    assert prod.get_pr(pr2.number).comments == [
+
-        (users['user'], "Ping @%s, @%s" % (users['other'], users['reviewer'])),
+To merge the full chain, say
+> @%s r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""" % (users['reviewer'], project.fp_github_name)),
         (users['user'], "This forward port PR is awaiting action"),
     ]
     with prod:
@@ -591,10 +595,12 @@ def test_empty(env, config, make_repo, users):
     env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron')
     env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron')
     failed_pr = prod.get_pr(fail_id.number)
+    # stop there (criminal scum), checking the entire message is a PITA and
+    # I'd need to look into integrating better diffing / error reporting when
+    # an re_matches doesn't match
+    ping_note = re_matches(r"Ping @%s\nCherrypicking [0-9a-z]{40} of source #%d failed" % (users['reviewer'], pr1.number))
     assert failed_pr.comments == [
-        (users['user'], 'Ping @{}, @{}'.format(
+        (users['user'], ping_note),
-            users['user'], users['reviewer'],
-        )),
         (users['user'], "This forward port PR is awaiting action"),
         (users['user'], "This forward port PR is awaiting action"),
     ]
@@ -602,13 +608,12 @@ def test_empty(env, config, make_repo, users):
     with prod:
         failed_pr.close()
     env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron')
+    # FIXME: carve out git messages as they're not necessarily stable / parseable
     assert failed_pr.comments == [
-        (users['user'], 'Ping @{}, @{}'.format(
+        (users['user'], ping_note),
-            users['user'], users['reviewer'],
-        )),
         (users['user'], "This forward port PR is awaiting action"),
         (users['user'], "This forward port PR is awaiting action"),
-    ], "should not have a third note"
+    ], "should not have a 4th note"
 
 def test_partially_empty(env, config, make_repo):
     """ Check what happens when only some commits of the PR are now empty
@@ -925,7 +930,11 @@ def test_batched(env, config, make_repo, users):
     ])
     # check that relevant users were pinged
     ping = [
-        (users['user'], "Ping @{}, @{}".format(friendo['user'], users['reviewer'])),
+        (users['user'], """\
+This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+"""),
     ]
     pr_remote_1b = main1.get_pr(pr1b.number)
     pr_remote_2b = main2.get_pr(pr2b.number)
@@ -1021,9 +1030,11 @@ class TestClosing:
         assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\
             "closing the PR should suppress the FP sequence"
         assert prod.get_pr(pr1.number).comments == [
-            (users['user'], 'Ping @{}, @{}'.format(
+            (users['user'], """\
-                users['user'], users['reviewer'],
+This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
-            ))
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""")
         ]
 
     def test_closing_after_fp(self, env, config, make_repo):