[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

@@ -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)
+            # only keep the PRs which don't have an attached descendant
-            pr_ids = {p.id for p in prs}
+            for pr in set(prs).difference(p.parent_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):
                 self.env.ref('runbot_merge.forwardport.reminder')._send(
                     repository=pr.repository,
                     pull_request=pr.number,

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')
-        prod.post_status(p_0, 'success', 'ci/runbot')
         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')
-        prod.post_status('staging.b', 'success', 'ci/runbot')
 
     # 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')
-        prod.post_status(p_1, 'success', 'ci/runbot')
         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')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-
     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 == [
+    assert fail_pr.comments[2:] == [awaiting]*2,\
-        seen(env, fail_pr, users),
+        "message should not be triggered on closed PR"
-        conflict,
+
+    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