[FIX] runbot_merge: missing feedback on fw r+

In some cases, feedback to the PR author that an r+ is redundant went
missing.

This turns out to be due to the convolution of the handling of
approval on forward-port, and the fact that the target PR is treated
exactly like its ancestors: if the PR is already approved the approval
is not even attempted (and so no feedback if it's incorrect).

Straighten up this bit and add a special case for the PR being
commented on, it should have the usual feedback if in error or already
commented on.

Furthermore, update `PullRequests._pr_acl` to kinda work out of the
box for forward-port: if the current PR is a forward port,
`is_reviewer` should check delegation on all ancestors, there doesn't
seem to be any reason to split "source_reviewer", "parent_reviewer",
and "is_reviewer".

Fixes #939

forwardport/tests/test_simple.py

@@ -692,6 +692,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 '''.format_map(users)),
         (users['other'], 'hansen r+')
     ]
+    assert pr2_id.reviewed_by
 
 
 def test_redundant_approval(env, config, make_repo, users):

forwardport/tests/test_weird.py

@@ -136,6 +136,64 @@ def test_failed_staging(env, config, make_repo):
     assert pr3_id.state == 'ready'
     assert pr3_id.staging_id
 
+def test_fw_retry(env, config, make_repo, users):
+    prod, _ = make_basic(env, config, make_repo, statuses='default')
+    other_token = config['role_other']['token']
+    fork = prod.fork(token=other_token)
+    with prod, fork:
+        fork.make_commits('a', Commit('c', tree={'a': '0'}), ref='heads/abranch')
+        pr1 = prod.make_pr(
+            title="whatever",
+            target='a',
+            head=f'{fork.owner}:abranch',
+            token=other_token,
+        )
+        prod.post_status(pr1.head, 'success')
+        pr1.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    other_partner = env['res.partner'].search([('github_login', '=', users['other'])])
+    assert len(other_partner) == 1
+    other_partner.email = "foo@example.com"
+
+    with prod:
+        prod.post_status('staging.a', 'success')
+    env.run_crons()
+
+    _pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    pr2 = prod.get_pr(pr2_id.number)
+    with prod:
+        prod.post_status(pr2_id.head, 'success')
+        pr2.post_comment('hansen r+', other_token)
+    env.run_crons()
+    assert not pr2_id.blocked
+
+    with prod:
+        prod.post_status('staging.b', 'failure')
+    env.run_crons()
+
+    assert pr2_id.error
+    with prod:
+        pr2.post_comment('hansen r+', other_token)
+    env.run_crons()
+    assert pr2_id.state == 'error'
+    with prod:
+        pr2.post_comment('hansen retry', other_token)
+    env.run_crons()
+    assert pr2_id.state == 'ready'
+
+    assert pr2.comments == [
+        seen(env, pr2, users),
+        (users['user'], "This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.\n\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n"),
+        (users['other'], 'hansen r+'),
+        (users['user'], "@{other} @{reviewer} staging failed: default".format_map(users)),
+
+        (users['other'], 'hansen r+'),
+        (users['user'], "This PR is already reviewed, it's in error, you might want to `retry` it instead (if you have already confirmed the error is not legitimate)."),
+
+        (users['other'], 'hansen retry'),
+    ]
+
 class TestNotAllBranches:
     """ Check that forward-ports don't behave completely insanely when not all
     branches are supported on all repositories.

runbot_merge/models/commands.py

@@ -72,6 +72,14 @@ class Approve:
             return f"r={ids}"
         return 'review+'
 
+    def __contains__(self, item):
+        if self.ids is None:
+            return True
+        return item in self.ids
+
+    def fmt(self):
+        return ", ".join(f"#{n:d}" for n in (self.ids or ()))
+
     @classmethod
     def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
         yield "r(eview)+", "approves the PR, if it's a forwardport also approves all non-detached parents"
@@ -184,7 +192,7 @@ class FW(enum.Enum):
     DEFAULT = enum.auto()
     NO = enum.auto()
     SKIPCI = enum.auto()
-    SKIPMERGE = enum.auto()
+    # SKIPMERGE = enum.auto()
 
     def __str__(self) -> str:
         return f'fw={self.name.lower()}'

runbot_merge/models/pull_requests.py

@@ -679,14 +679,14 @@ class PullRequests(models.Model):
             })
 
         is_admin, is_reviewer, is_author = self._pr_acl(author)
-        _source_admin, source_reviewer, source_author = self.source_id._pr_acl(author)
+        source_author = self.source_id._pr_acl(author).is_author
         # nota: 15.0 `has_group` completely doesn't work if the recordset is empty
         super_admin = is_admin and author.user_ids and author.user_ids.has_group('runbot_merge.group_admin')
 
         help_list: list[type(commands.Command)] = list(filter(None, [
             commands.Help,
 
-            (self.source_id and (source_author or source_reviewer) or is_reviewer) and not self.reviewed_by and commands.Approve,
+            (is_reviewer or (self.source_id and source_author)) and not self.reviewed_by and commands.Approve,
             (is_author or source_author) and self.reviewed_by and commands.Reject,
             (is_author or source_author) and self.error and commands.Retry,
 
@@ -768,55 +768,21 @@ For your own safety I've ignored *everything in your entire comment*.
             match command:
                 case commands.Approve() if self.draft:
                     msg = "draft PRs can not be approved."
-                case commands.Approve() if self.source_id:
+                case commands.Approve() if self.source_id and (source_author or is_reviewer):
-                    # rules are a touch different for forwardport PRs:
+                    if selected := [p for p in self._iter_ancestors() if p.number in command]:
-                    valid = lambda _: True if command.ids is None else lambda n: n in command.ids
+                        for pr in selected:
-                    _, source_reviewer, source_author = self.source_id._pr_acl(author)
+                            # ignore already reviewed PRs, unless it's the one
-
+                            # being r+'d, this means ancestors in error will not
-                    ancestors = list(self._iter_ancestors())
+                            # be warned about
-                    # - reviewers on the original can approve any forward port
+                            if pr == self or not pr.reviewed_by:
-                    if source_reviewer:
+                                pr._approve(author, login)
-                        approveable = ancestors
-                    elif source_author:
-                        # give full review rights on all forwardports (attached
-                        # or not) to original author
-                        approveable = ancestors
                     else:
-                        # between the first merged ancestor and self
+                        msg = f"tried to approve PRs {command.fmt()} but no such PR is an ancestors of {self.number}"
-                        mergeors = list(itertools.dropwhile(
-                            lambda p: p.state != 'merged',
-                            reversed(ancestors),
-                        ))
-                        # between the first ancestor the current user can review and self
-                        reviewors = list(itertools.dropwhile(
-                            lambda p: not p._pr_acl(author).is_reviewer,
-                            reversed(ancestors),
-                        ))
-
-                        # source author can approve any descendant of a merged
-                        # forward port (or source), people with review rights
-                        # to a forward port have review rights to its
-                        # descendants, if both apply use the most favorable
-                        # (largest number of PRs)
-                        if source_author and len(mergeors) > len(reviewors):
-                            approveable = mergeors
-                        else:
-                            approveable = reviewors
-
-                    if approveable:
-                        for pr in approveable:
-                            if not (pr.state in RPLUS and valid(pr.number)):
-                                continue
-                            msg = pr._approve(author, login)
-                            if msg:
-                                break
-                    else:
-                        msg = f"you can't {command} you silly little bean."
                 case commands.Approve() if is_reviewer:
-                    if command.ids is not None and command.ids != [self.number]:
+                    if command.ids is None or command.ids == [self.number]:
-                        msg = f"tried to approve PRs {command.ids} but the current PR is {self.number}"
-                    else:
                         msg = self._approve(author, login)
+                    else:
+                        msg = f"tried to approve PRs {command.fmt()} but the current PR is {self.number}"
                 case commands.Reject() if is_author or source_author:
                     if self.batch_id.skipchecks or self.reviewed_by:
                         if self.error:
@@ -916,10 +882,10 @@ For your own safety I've ignored *everything in your entire comment*.
                             message = "Disabled forward-porting."
                         case commands.FW.DEFAULT if is_author or source_author:
                             message = "Waiting for CI to create followup forward-ports."
-                        case commands.FW.SKIPCI if is_reviewer or source_reviewer:
+                        case commands.FW.SKIPCI if is_reviewer:
                             message = "Not waiting for CI to create followup forward-ports."
-                        case commands.FW.SKIPMERGE if is_reviewer or source_reviewer:
+                        # case commands.FW.SKIPMERGE if is_reviewer:
-                            message = "Not waiting for merge to create followup forward-ports."
+                        #     message = "Not waiting for merge to create followup forward-ports."
                         case _:
                             msg = f"you don't have the right to {command}."
 
@@ -1142,7 +1108,7 @@ For your own safety I've ignored *everything in your entire comment*.
             kw['bodies'] = {p.id: html_escape(message) for p in self}
         return super()._message_log_batch(**kw)
 
-    def _pr_acl(self, user):
+    def _pr_acl(self, user) -> ACL:
         if not self:
             return ACL(False, False, False)
 
@@ -1151,10 +1117,18 @@ For your own safety I've ignored *everything in your entire comment*.
             ('repository_id', '=', self.repository.id),
             ('review', '=', True) if self.author != user else ('self_review', '=', True),
         ]) == 1
-        is_reviewer = is_admin or self in user.delegate_reviewer
+        if is_admin:
-        # TODO: should delegate reviewers be able to retry PRs?
+            return ACL(True, True, True)
-        is_author = is_reviewer or self.author == user
+
-        return ACL(is_admin, is_reviewer, is_author)
+        # delegate on source = delegate on PR
+        if self.source_id and self.source_id in user.delegate_reviewer:
+            return ACL(False, True, True)
+        # delegate on any ancestors ~ delegate on PR (maybe should be any descendant of source?)
+        if any(p in user.delegate_reviewer for p in self._iter_ancestors()):
+            return ACL(False, True, True)
+
+        # user is probably always False on a forward port
+        return ACL(False, False, self.author == user)
 
     def _validate(self, statuses):
         # could have two PRs (e.g. one open and one closed) at least