[IMP] forwardport: outstanding FP reminders

- only remind weekly initially (not daily)
- root reminders on the forward port's creation, not the source's
  merge date
- cap reminder interval at 4 weeks (instead of doubling every time)
- track reminders on forward ports, don't share between siblings
- remove `forwardport_updated_before` from the testing system, it's
  now possible to just update `reminder_next` to a past date and see
  if it gets triggered (it should to nothing on sources, or on forward
  port in a state which does not warrant concern)

Fixes #801

conftest.py

@@ -391,9 +391,7 @@ class Base(models.AbstractModel):
     def run_crons(self):
         builtins.current_date = self.env.context.get('current_date')
         builtins.forwardport_merged_before = self.env.context.get('forwardport_merged_before')
-        builtins.forwardport_updated_before = self.env.context.get('forwardport_updated_before')
         self.env['ir.cron']._process_jobs(self.env.cr.dbname)
-        del builtins.forwardport_updated_before
         del builtins.forwardport_merged_before
         return True
 

forwardport/__manifest__.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 {
     'name': 'forward port bot',
-    'version': '1.4',
+    'version': '1.5',
     'summary': "A port which forward ports successful PRs.",
     'depends': ['runbot_merge'],
     'data': [

forwardport/changelog/2025-02/reminder-individual.md

@@ -0,0 +1,6 @@
+IMP: reminder interval is now tracked by each forward port
+
+Previously it was tracked on the source, which made sense when the notification
+was on the source, but not since the reminder is per-forward-port. However this
+also means brand new forward ports start at 0 instead of wherever the reminder
+source PR last got from their predecessors.

forwardport/changelog/2025-02/reminder-interval.md

@@ -0,0 +1,6 @@
+IMP: the reminder interval has been capped at ~1 month, but increased in the initial phase
+
+Previously the reminder interval would start at 1 day for about a week, then
+double every time, quickly increasing to less than yearly.
+
+The reminder interval is now weekly for the first month, then monthly.

forwardport/migrations/17.0.1.5/pre-migrate.py

@@ -0,0 +1,27 @@
+def migrate(cr, version):
+    """
+    before: source PR has a `reminder_backoff_factor`, specifies the
+            power-of-two number of days between the source's merge date and the
+            next reminder(s)
+    after: forward ports have a `reminder_next` which is the date for sending
+           the next reminder, needs to be at least 7 days after the forward
+           port's creation, if more than that just send a reminder the next
+           time we can (?)
+
+    We don't actually care about the source's anything (technically we could
+    e.g. if we just sent a reminder via the backoff factor then don't send a
+    new one but...)
+    """
+    cr.execute("""
+    ALTER TABLE runbot_merge_pull_requests
+        ADD COLUMN reminder_next varchar;
+
+    UPDATE runbot_merge_pull_requests
+       SET reminder_next = greatest(
+           now(),
+           create_date::timestamp + interval '7 days'
+       )
+      WHERE source_id IS NOT NULL
+        AND state IN ('opened', 'validated', 'approved', 'ready', 'error')
+        AND blocked IS NOT NULL;
+    """)

forwardport/models/project.py

@@ -36,7 +36,7 @@ from odoo.addons.runbot_merge.models.stagings_create import Message
 
 Conflict = tuple[str, str, str, list[str]]
 
-DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3)
+DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=7)
 
 _logger = logging.getLogger('odoo.addons.forwardport')
 
@@ -183,7 +183,10 @@ class PullRequests(models.Model):
     merge_date: datetime.datetime
     parent_id: PullRequests
 
-    reminder_backoff_factor = fields.Integer(default=-4, group_operator=None)
+    reminder_next = fields.Datetime(
+        default=lambda self: self.env.cr.now() + datetime.timedelta(days=7),
+        index=True,
+    )
 
     @api.model_create_multi
     def create(self, vals_list):
@@ -518,41 +521,27 @@ stderr:
         # don't stringify so caller can still perform alterations
         return msg
 
-    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 cutoff: a datetime (ISO-8601 formatted)
-        :returns: an iterator of (source, forward_ports)
-        """
-        return groupby(self.env['runbot_merge.pull_requests'].search([
-            # only FP PRs
-            ('source_id', '!=', False),
-            # active
-            ('state', 'not in', ['merged', 'closed']),
-            # not ready
-            ('blocked', '!=', False),
-            ('source_id.merge_date', '<', cutoff),
-        ], order='source_id, id'), lambda p: p.source_id)
-
     def _reminder(self):
-        cutoff = getattr(builtins, 'forwardport_updated_before', None) \
-              or fields.Datetime.to_string(datetime.datetime.now() - DEFAULT_DELTA)
-        cutoff_dt = fields.Datetime.from_string(cutoff)
-
-        for source, prs in self._outstanding(cutoff):
-            backoff = dateutil.relativedelta.relativedelta(days=2**source.reminder_backoff_factor)
-            if source.merge_date > (cutoff_dt - backoff):
-                continue
-            source.reminder_backoff_factor += 1
-
-            # only keep the PRs which don't have an attached descendant
+        for _, prs in groupby(self.search([
+            ('source_id', '!=', False),
+            ('blocked', '!=', False),
+            ('state', 'in', ['opened', 'validated', 'approved', 'ready', 'error']),
+            ('reminder_next', '<', self.env.cr.now()),
+        ], order='source_id, id'), lambda p: p.source_id):
+            # only remind on the "tip" of every chain of descendants as they
+            # will most likely lead to their parent being validated (?)
             for pr in set(prs).difference(p.parent_id for p in prs):
+                # reminder every 7 days for the first 4 weeks, then every 4 weeks
+                if (pr.reminder_next - pr.create_date) < datetime.timedelta(days=28):
+                    pr.reminder_next += datetime.timedelta(days=7)
+                else:
+                    pr.reminder_next += datetime.timedelta(days=28)
+
                 self.env.ref('runbot_merge.forwardport.reminder')._send(
                     repository=pr.repository,
                     pull_request=pr.number,
                     token_field='fp_github_token',
-                    format_args={'pr': pr, 'source': source},
+                    format_args={'pr': pr, 'source': pr.source_id},
                 )
 
 

forwardport/tests/test_simple.py

@@ -120,9 +120,10 @@ def test_straightforward_flow(env, config, make_repo, users):
         prod.post_status(pr1.head, 'success', 'legal/cla')
 
     env.run_crons()
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
-
     pr0_, pr1_, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+    pr2.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
+
 
     assert pr.comments == [
         (users['reviewer'], 'hansen r+ rebase-ff'),
@@ -323,8 +324,10 @@ def test_empty(env, config, make_repo, users):
     assert project.fp_github_name == users['other']
 
     # check reminder
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    fail_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
+    fail_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
 
     awaiting = (
         users['other'],
@@ -368,7 +371,8 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     # check that this stops if we close the PR
     with prod:
         fail_pr.close()
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    fail_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
     assert pr1.comments == [
         (users['reviewer'], 'hansen r+'),
         seen(env, pr1, users),
@@ -381,13 +385,15 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     with prod:
         prod.post_status(fail_id.head, 'success')
     assert fail_id.state == 'validated'
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    fail_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
     assert fail_pr.comments[2:] == [awaiting]*3, "check that message triggers again"
 
     with prod:
         fail_pr.post_comment('hansen r+', config['role_reviewer']['token'])
     assert fail_id.state == 'ready'
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    fail_id.reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
     assert fail_pr.comments[2:] == [
         awaiting,
         awaiting,

forwardport/tests/test_weird.py

@@ -1209,7 +1209,8 @@ def test_reminder_detached(env, config, make_repo, users):
     pr_c = prod.get_pr(pr_c_id.number)
 
     # region sanity check
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    (pr_a_id | pr_b_id | pr_c_id).reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
 
     assert pr_b.comments == [
         seen(env, pr_b, users),
@@ -1243,7 +1244,8 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 
     # region check detached
     pr_c_id.write({'parent_id': False, 'detach_reason': 'because'})
-    env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK})
+    (pr_a_id | pr_b_id | pr_c_id).reminder_next = datetime.now() - timedelta(days=1)
+    env.run_crons('forwardport.reminder')
 
     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." % (