[IMP] forwardport: don't ping *every* forwardport

98aaa910 updated the forwardport notifications system to notify on the
forward ports rather than the source, to try and mitigate or at least
shift some of the spam: spam the followers of the original
source (which might be many people) somewhat less, at the possible
cost of spamming the author and reviewer more because they get a
message per forgotten forward port.

This change aims to alleviate part of the latter, by only notifying on
PRs which actually need to be r+'d, and not notifying on those which
will implicitly "inherit" the reviews. This should cut down on
redundant notifications and let users focus on the important ones.

forwardport/models/project.py

@@ -11,13 +11,14 @@ means PR creation is trickier (as mergebot assumes opened event will always
 lead to PR creation but fpbot wants to attach meaning to the PR when setting
 it up), ...
 """
+from __future__ import annotations
+
 import datetime
 import itertools
 import json
 import logging
 import operator
 import subprocess
-import sys
 import tempfile
 import typing
 from pathlib import Path
@@ -181,6 +182,8 @@ class PullRequests(models.Model):
     reviewed_by: Partner
     head: str
     state: str
+    merge_date: datetime.datetime
+    parent_id: PullRequests
 
     reminder_backoff_factor = fields.Integer(default=-4, group_operator=None)
 
@@ -529,11 +532,11 @@ stderr:
         # don't stringify so caller can still perform alterations
         return msg
 
-    def _outstanding(self, cutoff):
+    def _outstanding(self, cutoff: str) -> typing.ItemsView[PullRequests, list[PullRequests]]:
         """ Returns "outstanding" (unmerged and unclosed) forward-ports whose
         source was merged before ``cutoff`` (all of them if not provided).
 
-        :param str cutoff: a datetime (ISO-8601 formatted)
+        :param cutoff: a datetime (ISO-8601 formatted)
         :returns: an iterator of (source, forward_ports)
         """
         return groupby(self.env['runbot_merge.pull_requests'].search([
@@ -551,11 +554,17 @@ stderr:
 
         for source, prs in self._outstanding(cutoff):
             backoff = dateutil.relativedelta.relativedelta(days=2**source.reminder_backoff_factor)
-            prs = list(prs)
             if source.merge_date > (cutoff_dt - backoff):
                 continue
             source.reminder_backoff_factor += 1
+
+            # only keep the PRs which don't have an attached descendant)
+            pr_ids = {p.id for p in prs}
             for pr in prs:
+                pr_ids.discard(pr.parent_id.id)
+            print(source, prs, [p.parent_id for p in prs],
+                  '\n\t->', pr_ids, flush=True)
+            for pr in (p for p in prs if p.id in pr_ids):
                 self.env.ref('runbot_merge.forwardport.reminder')._send(
                     repository=pr.repository,
                     pull_request=pr.number,

forwardport/tests/test_simple.py

@@ -139,13 +139,7 @@ def test_straightforward_flow(env, config, make_repo, users):
 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
-"""),
-        (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % (
-            users['other'],
-            users['reviewer'],
-            pr0.display_name,
-        ))
-    ]
+""")]
 
     assert pr0_ == pr0
     assert pr1_ == pr1

forwardport/tests/test_weird.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-from datetime import datetime
+from datetime import datetime, timedelta
 
 import pytest
 
@@ -1164,3 +1164,104 @@ def test_maintain_batch_history(env, config, make_repo, users):
     assert pr1_b_id in b_batch.all_prs, "the PR is still in the batch"
     assert pr1_b_id not in b_batch.prs, "the PR is not in the open/active batch PRs"
     # endregion
+
+FMT = '%Y-%m-%d %H:%M:%S'
+FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
+def test_reminder_detached(env, config, make_repo, users):
+    """On detached forward ports, both sides of the detachment should be notified.
+    """
+    # region setup
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+    with prod:
+        prod.make_commits('a', Commit('c', tree={'x': '0'}), ref="heads/abranch")
+        pr_a = prod.make_pr(target='a', head='abranch')
+        prod.post_status('abranch', 'success')
+        pr_a.post_comment('hansen r+ fw=skipci', config['role_reviewer']['token'])
+    env.run_crons()
+
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+
+    pr_a_id = to_pr(env, pr_a)
+    pr_b_id = env['runbot_merge.pull_requests'].search([
+        ('target.name', '=', 'b'),
+        ('parent_id', '=', pr_a_id.id),
+    ])
+    assert pr_b_id
+    with prod:
+        prod.post_status(pr_b_id.head, 'success')
+    env.run_crons()
+    pr_c_id = env['runbot_merge.pull_requests'].search([
+        ('target.name', '=', 'c'),
+        ('parent_id', '=', pr_b_id.id),
+    ])
+    assert pr_c_id
+    # endregion
+
+    pr_b = prod.get_pr(pr_b_id.number)
+    pr_c = prod.get_pr(pr_c_id.number)
+
+    # region sanity check
+    env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+
+    assert pr_b.comments == [
+        seen(env, pr_b, users),
+        (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
+""")], "the intermediate PR should not be reminded"
+
+    assert pr_c.comments == [
+        seen(env, pr_c, users),
+        (users['user'], """\
+@%s @%s this PR targets c and is the last of the forward-port chain containing:
+* %s
+
+To merge the full chain, use
+> @hansen r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
+""" % (
+            users['user'], users['reviewer'],
+            pr_b_id.display_name,
+    )),
+        (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % (
+            users['user'],
+            users['reviewer'],
+            pr_a_id.display_name,
+        ))
+    ], "the final PR should be reminded"
+    # endregion
+
+    # region check detached
+    pr_c_id.write({'parent_id': False, 'detach_reason': 'because'})
+    env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+
+    for p in env['runbot_merge.pull_requests'].search_read(
+        [], ['id', 'parent_id', 'source_id', 'display_name']
+    ):
+        print(p, flush=True)
+
+    assert pr_b.comments[2:] == [
+        (users['user'], "@%s @%s child PR %s 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." % (
+            users['user'],
+            users['reviewer'],
+            pr_c_id.display_name,
+        )),
+        (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % (
+            users['user'],
+            users['reviewer'],
+            pr_a_id.display_name,
+        ))
+    ], "the detached-from intermediate PR should now be reminded"
+    assert pr_c.comments[3:] == [
+        (users['user'], "@%(user)s @%(reviewer)s this PR was modified / updated and has become a normal PR. It must be merged directly." % users),
+        (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % (
+            users['user'],
+            users['reviewer'],
+            pr_a_id.display_name,
+        ))
+    ], "the final forward port should be reminded as before"
+    # endregion