From 017b8d4bb04588d5760de21a149b1fa0e38b2126 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Fri, 6 Sep 2024 12:33:51 +0200
Subject: [PATCH] [FIX] forwardport: don't post reminder on PRs waiting to be
 staged

The FW reminder is useful to remind people of the outstanding forward
ports they need to process, as taking too long can be an issue.

They are, however, not useful if the developer has already done
everything, the PR is ready and unblocked, but the mergebot has fallen
behind and has a hard time catching up. In that case there is nothing
for the developer to do, so pinging them is not productive, it's only
frustrating.

Fixes #930
---
 forwardport/models/project.py    |  9 +++----
 forwardport/tests/test_simple.py | 43 +++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 72630453..c3d4806d 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -484,6 +484,8 @@ stderr:
             ('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)
 
@@ -498,11 +500,8 @@ stderr:
                 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)
-            for pr in (p for p in prs if p.id in pr_ids):
+            # only keep the PRs which don't have an attached descendant
+            for pr in set(prs).difference(p.parent_id for p in prs):
                 self.env.ref('runbot_merge.forwardport.reminder')._send(
                     repository=pr.repository,
                     pull_request=pr.number,
diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index 187a1688..0c4260c2 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -251,7 +251,7 @@ def test_empty(env, config, make_repo, users):
     """ Cherrypick of an already cherrypicked (or separately implemented)
     commit -> conflicting pr.
     """
-    prod, other = make_basic(env, config, make_repo)
+    prod, other = make_basic(env, config, make_repo, statuses="default")
     # merge change to b
     with prod:
         [p_0] = prod.make_commits(
@@ -259,13 +259,11 @@ def test_empty(env, config, make_repo, users):
             ref='heads/early'
         )
         pr0 = prod.make_pr(target='b', head='early')
-        prod.post_status(p_0, 'success', 'legal/cla')
-        prod.post_status(p_0, 'success', 'ci/runbot')
+        prod.post_status(p_0, 'success')
         pr0.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
     with prod:
-        prod.post_status('staging.b', 'success', 'legal/cla')
-        prod.post_status('staging.b', 'success', 'ci/runbot')
+        prod.post_status('staging.b', 'success')
 
     # merge same change to a afterwards
     with prod:
@@ -274,15 +272,13 @@ def test_empty(env, config, make_repo, users):
             ref='heads/late'
         )
         pr1 = prod.make_pr(target='a', head='late')
-        prod.post_status(p_1, 'success', 'legal/cla')
-        prod.post_status(p_1, 'success', 'ci/runbot')
+        prod.post_status(p_1, 'success')
         pr1.post_comment('hansen r+', config['role_reviewer']['token'])
     env.run_crons()
     with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-
+        prod.post_status('staging.a', 'success')
     env.run_crons()
+
     assert prod.read_tree(prod.commit('a')) == {
         'f': 'e',
         'x': '0',
@@ -315,7 +311,8 @@ def test_empty(env, config, make_repo, users):
     }
 
     with prod:
-        validate_all([prod], [fp_id.head, fail_id.head])
+        prod.post_status(fp_id.head, 'success')
+        prod.post_status(fail_id.head, 'success')
     env.run_crons()
 
     # should not have created any new PR
@@ -379,12 +376,28 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
         (users['reviewer'], 'hansen r+'),
         seen(env, pr1, users),
     ]
-    assert fail_pr.comments == [
-        seen(env, fail_pr, users),
-        conflict,
+    assert fail_pr.comments[2:] == [awaiting]*2,\
+        "message should not be triggered on closed PR"
+
+    with prod:
+        fail_pr.open()
+    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})
+    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})
+    assert fail_pr.comments[2:] == [
         awaiting,
         awaiting,
-    ], "each cron run should trigger a new message"
+        awaiting,
+        (users['reviewer'], "hansen r+"),
+    ],"if a PR is ready (unblocked), the reminder should not trigger as "\
+        "we're likely waiting for the mergebot"
 
 def test_partially_empty(env, config, make_repo):
     """ Check what happens when only some commits of the PR are now empty