[FIX] *: re-enable notification on status failure

If a PR gets approved *then* fails CI, there should be a notification
warning the author & reviewer since
48e08b657b, it even has a test, which
passes (in fact it has *two*, one of which is redundant, so merge
`test_ci_failure_after_review` into the later `test_ci_approved`).

*However* this is in runbot_merge, turns out in
fafa7ef437 some refactoring was done in
order to override the notification and customise it for *forward
ports* with a failed status... except that override never called its
`super()`, so as soon as forwardport is installed the base
notification stops working, and that's been that since October
2019 (had been added in March that year, ignoring deployment lag).

This can be revealed by adding the corresponding check in the
*forwardport* tests, revealing the failure.

This was a pain to track down, thankfully it reproduced relatively
easily locally.

While this could be resolved in the override, might as well fold it
into the base method in furtherance of #789: the mergebot is only
used by odoo, and only with both modules combined, so splitting them
is not useful. And furthermore it things should work fine with the
forwardport installed but unused.

Fixes #894

forwardport/models/project.py

@@ -275,18 +275,6 @@ class PullRequests(models.Model):
             })
         return r
 
-    def _notify_ci_failed(self, ci):
-        # only care about FP PRs which are not staged / merged yet
-        # NB: probably ignore approved PRs as normal message will handle them?
-        if not (self.state == 'opened' and self.parent_id):
-            return
-
-        self.env.ref('runbot_merge.forwardport.ci.failed')._send(
-            repository=self.repository,
-            pull_request=self.number,
-            token_field='fp_github_token',
-            format_args={'pr': self, 'ci': ci},
-        )
 
     def _validate(self, statuses):
         failed = super()._validate(statuses)

forwardport/tests/test_updates.py

@@ -33,11 +33,22 @@ def test_update_pr(env, config, make_repo, users, merge_parent) -> None:
             ref='heads/hugechange'
         )
         pr = prod.make_pr(target='a', head='hugechange')
-        prod.post_status(p_1, 'success', 'legal/cla')
-        prod.post_status(p_1, 'success', 'ci/runbot')
         pr.post_comment('hansen r+', config['role_reviewer']['token'])
 
+        prod.post_status(p_1, 'success', 'legal/cla')
+        prod.post_status(p_1, 'failure', 'ci/runbot')
     env.run_crons()
+
+    assert pr.comments == [
+        (users['reviewer'], 'hansen r+'),
+        seen(env, pr, users),
+        (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
+    ]
+
+    with prod:
+        prod.post_status(p_1, 'success', 'ci/runbot')
+    env.run_crons()
+
     with prod:
         prod.post_status('staging.a', 'success', 'legal/cla')
         prod.post_status('staging.a', 'success', 'ci/runbot')

runbot_merge/models/pull_requests.py

@@ -1168,6 +1168,14 @@ class PullRequests(models.Model):
                 pull_request=self.number,
                 format_args={'pr': self, 'status': ci}
             )
+        elif self.state == 'opened' and self.parent_id:
+            # only care about FP PRs which are not approved / staged / merged yet
+            self.env.ref('runbot_merge.forwardport.ci.failed')._send(
+                repository=self.repository,
+                pull_request=self.number,
+                token_field='fp_github_token',
+                format_args={'pr': self, 'ci': ci},
+            )
 
     def _auto_init(self):
         for field in self._fields.values():

runbot_merge/tests/test_basic.py

@@ -1042,38 +1042,6 @@ def test_rebase_failure(env, repo, users, config):
         'b': 'b',
     }
 
-def test_ci_failure_after_review(env, repo, users, config):
-    """ If a PR is r+'d but the CI ends up failing afterwards, ping the user
-    so they're aware. This is useful for the more "fire and forget" approach
-    especially small / simple PRs where you assume they're going to pass and
-    just r+ immediately.
-    """
-    with repo:
-        prx = _simple_init(repo)
-        prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
-    env.run_crons()
-
-    for ctx, url in [
-        ('ci/runbot', 'https://a'),
-        ('ci/runbot', 'https://a'),
-        ('legal/cla', 'https://b'),
-        ('foo/bar', 'https://c'),
-        ('ci/runbot', 'https://a'),
-        ('legal/cla', 'https://d'), # url changes so different from the previous
-    ]:
-        with repo:
-            repo.post_status(prx.head, 'failure', ctx, target_url=url)
-        env.run_crons()
-
-    assert prx.comments == [
-        (users['reviewer'], 'hansen r+ rebase-ff'),
-        seen(env, prx, users),
-        (users['user'], "Merge method set to rebase and fast-forward."),
-        (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
-        (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
-        (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
-    ]
-
 def test_reopen_merged_pr(env, repo, config, users):
     """ Reopening a *merged* PR should cause us to immediately close it again,
     and insult whoever did it
@@ -3902,28 +3870,34 @@ class TestFeedback:
     def test_ci_approved(self, repo, env, users, config):
         """CI failing on an r+'d PR sends feedback"""
         with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-            repo.make_ref('heads/master', m)
+            [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master")
 
-            c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
-            prx = repo.make_pr(title='title', body='body', target='master', head=c1)
-        pr = env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', repo.name),
-            ('number', '=', prx.number)
-        ])
-
-        with repo:
-            prx.post_comment('hansen r+', config['role_reviewer']['token'])
-        assert pr.state == 'approved'
-
-        with repo:
-            repo.post_status(prx.head, 'failure', 'ci/runbot')
+            [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'}))
+            pr = repo.make_pr(title='title', body='body', target='master', head=c1)
+            pr.post_comment('hansen r+', config['role_reviewer']['token'])
         env.run_crons()
 
-        assert prx.comments == [
+        pr_id = to_pr(env, pr)
+        assert pr_id.state == 'approved'
+
+        for ctx, url in [
+            ('ci/runbot', 'https://a'),
+            ('ci/runbot', 'https://a'),
+            ('legal/cla', 'https://b'),
+            ('foo/bar', 'https://c'),
+            ('ci/runbot', 'https://a'),
+            ('legal/cla', 'https://d'),  # url changes so different from the previous
+        ]:
+            with repo:
+                repo.post_status(pr_id.head, 'failure', ctx, target_url=url)
+            env.run_crons()
+
+        assert pr.comments == [
             (users['reviewer'], 'hansen r+'),
-            seen(env, prx, users),
-            (users['user'], "@%(user)s @%(reviewer)s 'ci/runbot' failed on this reviewed PR." % users)
+            seen(env, pr, users),
+            (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
+            (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
+            (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
         ]
 
     def test_review_failed(self, repo, env, users, config):