[IMP] fwbot: warning on r+ for failed PR

approving a PR which failed CI should trigger a feedback message since
6cb58a322d (#158), the code has not been
removed and the tests still pass.

However fwbot r+ would go through its own process for r+ which would
explain why that feedback is sometimes gone / lost (cf #327 and #336).

* make fwbot r+ delegate to mergebot r+
* add dedicated logging for this operation to better analyze
  post-mortem
* automatically ping the reviewer to specifically tell them they're idiots
* move the feedback item out of the state change bit, send it even if
  it's a useless r+ (because it's already r+'d)
* add a test for forward-ports

Closes #327, closes #336

forwardport/models/project.py

@@ -317,13 +317,11 @@ class PullRequests(models.Model):
                         'message': "I'm sorry, @{}. You can't review+.".format(login),
                     })
                     continue
+                merge_bot = self.repository.project_id.github_prefix
                 # don't update the root ever
                 for pr in filter(lambda p: p.parent_id, self._iter_ancestors()):
-                    newstate = RPLUS.get(pr.state)
-                    if newstate:
-                        pr.state = newstate
-                        pr.reviewed_by = author
-                        # TODO: logging & feedback
+                    # only the author is delegated explicitely on the
+                    pr._parse_commands(author, merge_bot + ' r+', login)
             elif token == 'close':
                 close = False
                 message = "I'm sorry, @{}. I can't close this PR for you."
@@ -653,19 +651,16 @@ class PullRequests(models.Model):
                 _logger.info("Created forward-port PR %s", new_pr)
             new_batch |= new_pr
 
+            # delegate original author on merged original PR & on new PR so
+            # they can r+ the forward ports (via mergebot or forwardbot)
+            source.delegates |= source.author
             new_pr.write({
                 'merge_method': pr.merge_method,
                 'source_id': source.id,
                 # only link to previous PR of sequence if cherrypick passed
                 'parent_id': pr.id if not has_conflicts else False,
-            })
-            # delegate original author on merged original PR & on new PR so
-            # they can r+ the forward ports (via mergebot or forwardbot)
-            source.author.write({
-                'delegate_reviewer': [
-                    (4, source.id, False),
-                    (4, new_pr.id, False),
-                ]
+                # copy all delegates of source to new
+                'delegates': [(6, False, source.delegates.ids)]
             })
             # not great but we probably want to avoid the risk of the webhook
             # creating the PR from under us. There's still a "hole" between

forwardport/tests/test_simple.py

@@ -246,7 +246,7 @@ def test_update_pr(env, config, make_repo, users):
     # should merge the staging then create the FP PR
     env.run_crons()
 
-    pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
+    pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
 
     fp_intermediate = (users['user'], '''\
 This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
@@ -260,23 +260,23 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p
     # some delivery lag allowing for the cron to run between each delivery
     for st, ctx in [('failure', 'ci/runbot'), ('failure', 'ci/runbot'), ('success', 'legal/cla'), ('success', 'legal/cla')]:
         with prod:
-            prod.post_status(pr1.head, st, ctx)
+            prod.post_status(pr1_id.head, st, ctx)
         env.run_crons()
     with prod: # should be ignored because the description doesn't matter
-        prod.post_status(pr1.head, 'failure', 'ci/runbot', description="HAHAHAHAHA")
+        prod.post_status(pr1_id.head, 'failure', 'ci/runbot', description="HAHAHAHAHA")
     env.run_crons()
     # check that FP did not resume & we have a ping on the PR
-    assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\
+    assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
         "forward port should not continue on CI failure"
-    pr1_remote = prod.get_pr(pr1.number)
+    pr1_remote = prod.get_pr(pr1_id.number)
     assert pr1_remote.comments == [fp_intermediate, ci_warning]
 
     # it was a false positive, rebuild... it fails again!
     with prod:
-        prod.post_status(pr1.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890')
+        prod.post_status(pr1_id.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890')
     env.run_crons()
     # check that FP did not resume & we have a ping on the PR
-    assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\
+    assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
         "ensure it still hasn't restarted"
     assert pr1_remote.comments == [fp_intermediate, ci_warning, ci_warning]
 
@@ -285,44 +285,44 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p
 
     # rebuild again, finally passes
     with prod:
-        prod.post_status(pr1.head, 'success', 'ci/runbot')
+        prod.post_status(pr1_id.head, 'success', 'ci/runbot')
     env.run_crons()
 
-    pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
-    assert pr1.parent_id == pr0
-    assert pr2.parent_id == pr1
-    pr1_head = pr1.head
-    pr2_head = pr2.head
+    pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    assert pr1_id.parent_id == pr0_id
+    assert pr2_id.parent_id == pr1_id
+    pr1_head = pr1_id.head
+    pr2_head = pr2_id.head
 
     # turns out branch b is syntactically but not semantically compatible! It
     # needs x to be 5!
-    pr_repo, pr_ref = prod.get_pr(pr1.number).branch
+    pr_repo, pr_ref = prod.get_pr(pr1_id.number).branch
     with pr_repo:
         # force-push correct commit to PR's branch
         [new_c] = pr_repo.make_commits(
-            pr1.target.name,
+            pr1_id.target.name,
             Commit('whop whop', tree={'x': '5'}),
             ref='heads/%s' % pr_ref,
             make=False
         )
     env.run_crons()
 
-    assert pr1.head == new_c != pr1_head, "the FP PR should be updated"
-    assert not pr1.parent_id, "the FP PR should be detached from the original"
+    assert pr1_id.head == new_c != pr1_head, "the FP PR should be updated"
+    assert not pr1_id.parent_id, "the FP PR should be detached from the original"
     assert pr1_remote.comments == [
         fp_intermediate, ci_warning, ci_warning,
-        (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1.repository.project_id.github_prefix),
+        (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1_id.repository.project_id.github_prefix),
     ], "users should be warned that the PR has become non-FP"
     # NOTE: should the followup PR wait for pr1 CI or not?
-    assert pr2.head != pr2_head
-    assert pr2.parent_id == pr1, "the followup PR should still be linked"
+    assert pr2_id.head != pr2_head
+    assert pr2_id.parent_id == pr1_id, "the followup PR should still be linked"
 
-    assert prod.read_tree(prod.commit(pr1.head)) == {
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
         'f': 'c',
         'g': 'b',
         'x': '5'
     }, "the FP PR should have the new code"
-    assert prod.read_tree(prod.commit(pr2.head)) == {
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
         'f': 'c',
         'g': 'a',
         'h': 'a',
@@ -331,18 +331,18 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p
 
     with pr_repo:
         pr_repo.make_commits(
-            pr1.target.name,
+            pr1_id.target.name,
             Commit('fire!', tree={'h': '0'}),
             ref='heads/%s' % pr_ref,
         )
     env.run_crons()
     # since there are PRs, this is going to update pr2 as broken
-    assert prod.read_tree(prod.commit(pr1.head)) == {
+    assert prod.read_tree(prod.commit(pr1_id.head)) == {
         'f': 'c',
         'g': 'b',
         'h': '0'
     }
-    assert prod.read_tree(prod.commit(pr2.head)) == {
+    assert prod.read_tree(prod.commit(pr2_id.head)) == {
         'f': 'c',
         'g': 'a',
         'h': re_matches(r'''<<<\x3c<<< HEAD
@@ -352,6 +352,32 @@ a
 >>>\x3e>>> [0-9a-f]{7,}(...)? temp
 '''),
     }
+    [project] = env['runbot_merge.project'].search([])
+    pr2 = prod.get_pr(pr2_id.number)
+    # fail pr2 then fwbot r+ to check that we get a warning
+    with prod:
+        prod.post_status(pr2_id.head, 'failure', 'ci/runbot')
+    env.run_crons() # parse commit statuses
+    with prod:
+        pr2.post_comment(project.fp_github_name + ' r+', config['role_reviewer']['token'])
+    env.run_crons() # send feedback
+
+    assert pr2.comments == [
+        (users['user'], """Ping @{}, @{}
+This PR targets c and is the last of the forward-port chain containing:
+* {}
+
+To merge the full chain, say
+> @{} r+
+
+More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-port
+""".format(users['user'], users['reviewer'], pr1_id.display_name, project.fp_github_name)),
+        (users['user'], 'Ping @{}, @{}\n\nci/runbot failed on this forward-port PR'.format(
+            users['user'], users['reviewer']
+        )),
+        (users['reviewer'], project.fp_github_name + ' r+'),
+        (users['user'], '@{}, you may want to rebuild or fix this PR as it has failed CI.'.format(users['reviewer'])),
+    ]
 
 def test_update_merged(env, make_repo, config, users):
     """ Strange things happen when an FP gets closed / merged but then its

runbot_merge/models/pull_requests.py

@@ -815,21 +815,28 @@ class PullRequests(models.Model):
                     })
             elif command == 'review':
                 if param and is_reviewer:
+                    oldstate = self.state
                     newstate = RPLUS.get(self.state)
                     if newstate:
                         self.state = newstate
                         self.reviewed_by = author
                         ok = True
-                        if self.status == 'failure':
-                            # the normal infrastructure is for failure and
-                            # prefixes messages with "I'm sorry"
-                            Feedback.create({
-                                'repository': self.repository.id,
-                                'pull_request': self.number,
-                                'message': "You may want to rebuild or fix this PR as it has failed CI.",
-                            })
                     else:
                         msg = "This PR is already reviewed, reviewing it again is useless."
+                    _logger.debug(
+                        "r+ on %s by %s (%s->%s) status=%s message? %s",
+                        self.display_name, author.github_login,
+                        oldstate, newstate or oldstate,
+                        self.status, self.status == 'failure'
+                    )
+                    if self.status == 'failure':
+                        # the normal infrastructure is for failure and
+                        # prefixes messages with "I'm sorry"
+                        Feedback.create({
+                            'repository': self.repository.id,
+                            'pull_request': self.number,
+                            'message': "@{}, you may want to rebuild or fix this PR as it has failed CI.".format(author.github_login),
+                        })
                 elif not param and is_author:
                     newstate = RMINUS.get(self.state)
                     if self.priority == 0 or newstate:

runbot_merge/tests/test_basic.py

@@ -3158,7 +3158,7 @@ class TestFeedback:
             (users['user'], "'ci/runbot' failed on this reviewed PR.")
         ]
 
-    def test_review_unvalidated(self, repo, env, users, config):
+    def test_review_failed(self, repo, env, users, config):
         """r+-ing a PR with failed CI sends feedback"""
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
@@ -3184,7 +3184,7 @@ class TestFeedback:
 
         assert prx.comments == [
             (users['reviewer'], 'hansen r+'),
-            (users['user'], "You may want to rebuild or fix this PR as it has failed CI.")
+            (users['user'], "@%s, you may want to rebuild or fix this PR as it has failed CI." % users['reviewer'])
         ]
 class TestInfrastructure:
     def test_protection(self, repo):