[ADD] runbot_merge: help command, and help on error

Fixes #898

conftest.py

@@ -481,6 +481,17 @@ def env(request, port, server, db, default_crons):
         if b"Traceback (most recent call last):" in server[1]:
             pytest.fail("unexpected error in logs, fix, or mark function as `expect_log_errors` to require.")
 
+@pytest.fixture
+def reviewer_admin(env, partners):
+    env['res.users'].create({
+        'partner_id': partners['reviewer'].id,
+        'login': 'reviewer',
+        'groups_id': [
+            (4, env.ref("base.group_user").id, 0),
+            (4, env.ref("runbot_merge.group_admin").id, 0),
+        ],
+    })
+
 def check(response):
     assert response.ok, response.text or response.reason
     return response

forwardport/tests/test_limit.py

@@ -131,7 +131,36 @@ def test_disable(env, config, make_repo, users):
     assert set(pr.comments) == {
         seen(env, pr, users),
         (users['reviewer'], "hansen r+ up to"),
-        (users['user'], "@{reviewer} please provide a branch to forward-port to.\n\nFor your own safety I've ignored *everything in your entire comment*.".format_map(users)),
+        (users['user'], """\
+@{reviewer} please provide a branch to forward-port to.
+
+For your own safety I've ignored *everything in your entire comment*.
+
+Currently available commands:
+
+|command||
+|-|-|
+|`help`|displays this help|
+|`r(eview)+`|approves the PR, if it's a forwardport also approves all non-detached parents|
+|`r(eview)=<number>`|only approves the specified parents|
+|`fw=no`|does not forward-port this PR|
+|`fw=default`|forward-ports this PR normally|
+|`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`up to <branch>`|only ports this PR forward to the specified branch (included)|
+|`merge`|integrate the PR with a simple merge commit, using the PR description as message|
+|`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
+|`rebase-ff`|rebases the PR on top of the target branch, then fast-forwards|
+|`squash`|squashes the PR as a single commit on the target branch, using the PR description as message|
+|`delegate+`|grants approval rights to the PR author|
+|`delegate=<...>`|grants approval rights on this PR to the specified github users|
+|`default`|stages the PR normally|
+|`priority`|tries to stage this PR first, then adds `default` PRs if the staging has room|
+|`alone`|stages this PR only with other PRs of the same priority|
+|`cancel=staging`|automatically cancels the current staging when this PR becomes ready|
+|`check`|fetches or refreshes PR metadata, resets mergebot state|
+
+Note: this help text is dynamic and will change with the state of the PR.
+""".format_map(users)),
         (users['reviewer'], "hansen up to b"),
         (users['user'], "@{reviewer} branch 'b' is disabled, it can't be used as a forward port target.".format_map(users)),
         (users['reviewer'], "hansen up to foo"),

runbot_merge/models/commands.py

@@ -3,7 +3,7 @@ from collections.abc import Iterator
 from dataclasses import dataclass, field
 from functools import partial
 from operator import contains
-from typing import Callable, List, Optional, Union
+from typing import Callable, List, Optional, Union, Tuple
 
 
 def tokenize(line: str) -> Iterator[str]:
@@ -72,11 +72,18 @@ class Approve:
             return f"r={ids}"
         return 'review+'
 
+    @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"
+        yield "r(eview)=<number>", "only approves the specified parents"
 
 class Reject:
     def __str__(self) -> str:
         return 'review-'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "r(eview)-", "removes approval of a previously approved PR, if the PR is staged the staging will be cancelled"
 
 class MergeMethod(enum.Enum):
     SQUASH = 'squash'
@@ -87,16 +94,31 @@ class MergeMethod(enum.Enum):
     def __str__(self) -> str:
         return self.value
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield str(cls.MERGE), "integrate the PR with a simple merge commit, using the PR description as message"
+        yield str(cls.REBASE_MERGE), "rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message"
+        yield str(cls.REBASE_FF), "rebases the PR on top of the target branch, then fast-forwards"
+        yield str(cls.SQUASH), "squashes the PR as a single commit on the target branch, using the PR description as message"
+
 
 class Retry:
     def __str__(self) -> str:
         return 'retry'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "retry", 're-tries staging a PR in the "error" state'
+
 
 class Check:
     def __str__(self) -> str:
         return 'check'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "check", "fetches or refreshes PR metadata, resets mergebot state"
+
 
 @dataclass
 class Override:
@@ -105,6 +127,10 @@ class Override:
     def __str__(self) -> str:
         return f"override={','.join(self.statuses)}"
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "override=<...>", "marks overridable statuses as successful"
+
 
 @dataclass
 class Delegate:
@@ -115,6 +141,11 @@ class Delegate:
             return 'delegate+'
         return f"delegate={','.join(self.users)}"
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "delegate+", "grants approval rights to the PR author"
+        yield "delegate=<...>", "grants approval rights on this PR to the specified github users"
+
 
 class Priority(enum.Enum):
     DEFAULT = enum.auto()
@@ -124,16 +155,30 @@ class Priority(enum.Enum):
     def __str__(self) -> str:
         return self.name.lower()
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield str(cls.DEFAULT), "stages the PR normally"
+        yield str(cls.PRIORITY), "tries to stage this PR first, then adds `default` PRs if the staging has room"
+        yield str(cls.ALONE), "stages this PR only with other PRs of the same priority"
+
 
 class CancelStaging:
     def __str__(self) -> str:
         return "cancel=staging"
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "cancel=staging", "automatically cancels the current staging when this PR becomes ready"
+
 
 class SkipChecks:
     def __str__(self) -> str:
         return 'skipchecks'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "skipchecks", "bypasses both statuses and review"
+
 
 class FW(enum.Enum):
     DEFAULT = enum.auto()
@@ -144,6 +189,13 @@ class FW(enum.Enum):
     def __str__(self) -> str:
         return f'fw={self.name.lower()}'
 
+    @classmethod
+    def help(cls, is_reviewer: bool) -> Iterator[Tuple[str, str]]:
+        yield str(cls.NO), "does not forward-port this PR"
+        if is_reviewer:
+            yield str(cls.DEFAULT), "forward-ports this PR normally"
+            yield str(cls.SKIPCI), "does not wait for a forward-port's statuses to succeed before creating the next one"
+
 
 @dataclass
 class Limit:
@@ -154,11 +206,28 @@ class Limit:
             return 'ignore'
         return f'up to {self.branch}'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield "up to <branch>", "only ports this PR forward to the specified branch (included)"
+
 
 class Close:
     def __str__(self) -> str:
         return 'close'
 
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield str(cls()), "closes this forward-port"
+
+
+class Help:
+    def __str__(self) -> str:
+        return 'help'
+
+    @classmethod
+    def help(cls, _: bool) -> Iterator[Tuple[str, str]]:
+        yield str(cls()), "displays this help"
+
 
 Command = Union[
     Approve,
@@ -167,6 +236,7 @@ Command = Union[
     Check,
     Delegate,
     FW,
+    Help,
     Limit,
     MergeMethod,
     Override,
@@ -309,3 +379,6 @@ class Parser:
 
     def parse_close(self) -> Close:
         return Close()
+
+    def parse_help(self) -> Help:
+        return Help()

runbot_merge/models/pull_requests.py

@@ -642,6 +642,51 @@ class PullRequests(models.Model):
                 'message': message,
                 'close': close,
             })
+
+        is_admin, is_reviewer, is_author = self._pr_acl(author)
+        _source_admin, source_reviewer, source_author = self.source_id._pr_acl(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_author and self.reviewed_by and commands.Reject,
+            (is_author or source_author) and self.error and commands.Retry,
+
+            is_author and not self.source_id and commands.FW,
+            is_author and commands.Limit,
+            source_author and self.source_id and commands.Close,
+
+            is_reviewer and commands.MergeMethod,
+            is_reviewer and commands.Delegate,
+
+            is_admin and commands.Priority,
+            super_admin and commands.SkipChecks,
+            is_admin and commands.CancelStaging,
+
+            author.override_rights and commands.Override,
+            is_author and commands.Check,
+        ]))
+        def format_help(warn_ignore: bool, address: bool = True) -> str:
+            s = [
+                'Currently available commands{}:'.format(
+                    f" for @{login}" if address else ""
+                ),
+                '',
+                '|command||',
+                '|-|-|',
+            ]
+            for command_type in help_list:
+                for cmd, text in command_type.help(is_reviewer):
+                    s.append(f"|`{cmd}`|{text}|")
+
+            s.extend(['', 'Note: this help text is dynamic and will change with the state of the PR.'])
+            if warn_ignore:
+                s.extend(["", "Warning: in invoking help, every other command has been ignored."])
+            return "\n".join(s)
+
         try:
             cmds: List[commands.Command] = [
                 ps
@@ -656,11 +701,21 @@ class PullRequests(models.Model):
                 utils.shorten(comment['body'] or '', 50),
                 exc_info=True
             )
-            feedback(message=f"@{login} {e.args[0]}.\n\nFor your own safety I've ignored *everything in your entire comment*.")
+            feedback(message=f"""@{login} {e.args[0]}.
+
+For your own safety I've ignored *everything in your entire comment*.
+
+{format_help(False, address=False)}
+""")
             return 'error'
 
-        is_admin, is_reviewer, is_author = self._pr_acl(author)
+        if any(isinstance(cmd, commands.Help) for cmd in cmds):
-        _source_admin, source_reviewer, source_author = self.source_id._pr_acl(author)
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': self.repository.id,
+                'pull_request': self.number,
+                'message': format_help(len(cmds) != 1),
+            })
+            return "help"
 
         if not (is_author or self.source_id or (any(isinstance(cmd, commands.Override) for cmd in cmds) and author.override_rights)):
             # no point even parsing commands
@@ -775,7 +830,7 @@ class PullRequests(models.Model):
                     delegates.write({'delegate_reviewer': [(4, self.id, 0)]})
                 case commands.Priority() if is_admin:
                     self.batch_id.priority = str(command)
-                case commands.SkipChecks() if is_admin:
+                case commands.SkipChecks() if super_admin:
                     self.batch_id.skipchecks = True
                     self.reviewed_by = author
                     if not (self.squash or self.merge_method):

runbot_merge/tests/test_basic.py

@@ -2801,6 +2801,7 @@ class TestBatching(object):
         ]
         assert not pr22.staging_id
 
+    @pytest.mark.usefixtures("reviewer_admin")
     def test_batching_urgent(self, env, repo, config):
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
@@ -2920,6 +2921,7 @@ class TestBatching(object):
         assert p_01.skipchecks == False
         assert p_01.cancel_staging == True
 
+    @pytest.mark.usefixtures("reviewer_admin")
     def test_batching_urgenter_than_split(self, env, repo, config):
         """ p=alone PRs should take priority over split stagings (processing
         of a staging having CI-failed and being split into sub-stagings)
@@ -2959,6 +2961,7 @@ class TestBatching(object):
         assert not p_1.staging_id
         assert to_pr(env, pr0).staging_id
 
+    @pytest.mark.usefixtures("reviewer_admin")
     def test_urgent_failed(self, env, repo, config):
         """ Ensure pr[p=0,state=failed] don't get picked up
         """
@@ -3306,6 +3309,7 @@ class TestReviewing:
         env.run_crons()
         assert to_pr(env, pr).state == 'approved'
 
+    @pytest.mark.usefixtures("reviewer_admin")
     def test_skipchecks(self, env, repo, users, config):
         """Skipcheck makes the PR immediately ready (if it's not in error or
         something)
@@ -3622,8 +3626,66 @@ class TestRecognizeCommands:
             (users['reviewer'], "hansen do the thing"),
             (users['reviewer'], "hansen @bobby-b r+ :+1:"),
             seen(env, pr, users),
-            (users['user'], "@{reviewer} unknown command 'do'.\n\nFor your own safety I've ignored *everything in your entire comment*.".format_map(users)),
+            (users['user'], """\
-            (users['user'], "@{reviewer} unknown command '@bobby-b'.\n\nFor your own safety I've ignored *everything in your entire comment*.".format_map(users)),
+@{reviewer} unknown command 'do'.
+
+For your own safety I've ignored *everything in your entire comment*.
+
+Currently available commands:
+
+|command||
+|-|-|
+|`help`|displays this help|
+|`r(eview)+`|approves the PR, if it's a forwardport also approves all non-detached parents|
+|`r(eview)=<number>`|only approves the specified parents|
+|`fw=no`|does not forward-port this PR|
+|`fw=default`|forward-ports this PR normally|
+|`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`up to <branch>`|only ports this PR forward to the specified branch (included)|
+|`merge`|integrate the PR with a simple merge commit, using the PR description as message|
+|`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
+|`rebase-ff`|rebases the PR on top of the target branch, then fast-forwards|
+|`squash`|squashes the PR as a single commit on the target branch, using the PR description as message|
+|`delegate+`|grants approval rights to the PR author|
+|`delegate=<...>`|grants approval rights on this PR to the specified github users|
+|`default`|stages the PR normally|
+|`priority`|tries to stage this PR first, then adds `default` PRs if the staging has room|
+|`alone`|stages this PR only with other PRs of the same priority|
+|`cancel=staging`|automatically cancels the current staging when this PR becomes ready|
+|`check`|fetches or refreshes PR metadata, resets mergebot state|
+
+Note: this help text is dynamic and will change with the state of the PR.
+""".format_map(users)),
+            (users['user'], """\
+@{reviewer} unknown command '@bobby-b'.
+
+For your own safety I've ignored *everything in your entire comment*.
+
+Currently available commands:
+
+|command||
+|-|-|
+|`help`|displays this help|
+|`r(eview)+`|approves the PR, if it's a forwardport also approves all non-detached parents|
+|`r(eview)=<number>`|only approves the specified parents|
+|`fw=no`|does not forward-port this PR|
+|`fw=default`|forward-ports this PR normally|
+|`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`up to <branch>`|only ports this PR forward to the specified branch (included)|
+|`merge`|integrate the PR with a simple merge commit, using the PR description as message|
+|`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
+|`rebase-ff`|rebases the PR on top of the target branch, then fast-forwards|
+|`squash`|squashes the PR as a single commit on the target branch, using the PR description as message|
+|`delegate+`|grants approval rights to the PR author|
+|`delegate=<...>`|grants approval rights on this PR to the specified github users|
+|`default`|stages the PR normally|
+|`priority`|tries to stage this PR first, then adds `default` PRs if the staging has room|
+|`alone`|stages this PR only with other PRs of the same priority|
+|`cancel=staging`|automatically cancels the current staging when this PR becomes ready|
+|`check`|fetches or refreshes PR metadata, resets mergebot state|
+
+Note: this help text is dynamic and will change with the state of the PR.
+""".format_map(users)),
         ]
 
 class TestRMinus:

runbot_merge/tests/test_multirepo.py

@@ -761,6 +761,7 @@ class TestMultiBatches:
         assert sp.mapped('batch_ids.prs') == \
             prs[2][0] | prs[2][1] | prs[3][0] | prs[3][1] | prs[4][0]
 
+@pytest.mark.usefixtures("reviewer_admin")
 def test_urgent(env, repo_a, repo_b, config):
     """ Either PR of a co-dependent pair being prioritised leads to the entire
     pair being prioritized
@@ -872,6 +873,7 @@ class TestBlocked:
         p = to_pr(env, pr)
         assert not p.blocked
 
+    @pytest.mark.usefixtures("reviewer_admin")
     def test_linked_unready(self, env, repo_a, repo_b, config):
         """ Create a PR A linked to a non-ready PR B,
         * A is blocked by default

runbot_merge/tests/test_oddities.py

@@ -1,7 +1,7 @@
 import pytest
 import requests
 
-from utils import Commit, to_pr
+from utils import Commit, to_pr, seen
 
 
 def test_partner_merge(env):
@@ -253,8 +253,8 @@ def test_force_ready(env, repo, config):
     with repo:
         [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master")
 
-        [c] = repo.make_commits(m, Commit('first', tree={'m': 'c1'}), ref="heads/other")
+        repo.make_commits(m, Commit('first', tree={'m': 'c1'}), ref="heads/other")
-        pr = repo.make_pr(title='title', body='body', target='master', head=c)
+        pr = repo.make_pr(target='master', head='other')
     env.run_crons()
 
     pr_id = to_pr(env, pr)
@@ -264,3 +264,102 @@ def test_force_ready(env, repo, config):
     assert pr_id.status == 'pending'
     reviewer = env['res.users'].browse([env._uid]).partner_id
     assert pr_id.reviewed_by == reviewer
+
+def test_help(env, repo, config, users, partners):
+    with repo:
+        [m] = repo.make_commits(None, Commit('initial', tree={'m': 'm'}), ref="heads/master")
+
+        repo.make_commits(m, Commit('first', tree={'m': 'c1'}), ref="heads/other")
+        pr = repo.make_pr(target='master', head='other')
+    env.run_crons()
+
+    for role in ['reviewer', 'self_reviewer', 'user', 'other']:
+        v = config[f'role_{role}']
+        with repo:
+            pr.post_comment("hansen help", v['token'])
+    with repo:
+        pr.post_comment("hansen r+ help", config['role_reviewer']['token'])
+
+    assert not partners['reviewer'].user_ids, "the reviewer should not be an internal user"
+
+    group_internal = env.ref("base.group_user")
+    group_admin = env.ref("runbot_merge.group_admin")
+    env['res.users'].create({
+        'partner_id': partners['reviewer'].id,
+        'login': 'reviewer',
+        'groups_id': [(4, group_internal.id, 0), (4, group_admin.id, 0)],
+    })
+
+    with repo:
+        pr.post_comment("hansen help", config['role_reviewer']['token'])
+    env.run_crons()
+
+    assert pr.comments == [
+        seen(env, pr, users),
+        (users['reviewer'], "hansen help"),
+        (users['self_reviewer'], "hansen help"),
+        (users['user'], "hansen help"),
+        (users['other'], "hansen help"),
+        (users['reviewer'], "hansen r+ help"),
+        (users['reviewer'], "hansen help"),
+        (users['user'], REVIEWER.format(user=users['reviewer'], skip="")),
+        (users['user'], RANDO.format(user=users['self_reviewer'])),
+        (users['user'], AUTHOR.format(user=users['user'])),
+        (users['user'], RANDO.format(user=users['other'])),
+        (users['user'],
+         REVIEWER.format(user=users['reviewer'], skip='')
+         + "\n\nWarning: in invoking help, every other command has been ignored."),
+        (users['user'], REVIEWER.format(
+            user=users['reviewer'],
+            skip='|`skipchecks`|bypasses both statuses and review|\n',
+        )),
+    ]
+
+REVIEWER = """\
+Currently available commands for @{user}:
+
+|command||
+|-|-|
+|`help`|displays this help|
+|`r(eview)+`|approves the PR, if it's a forwardport also approves all non-detached parents|
+|`r(eview)=<number>`|only approves the specified parents|
+|`fw=no`|does not forward-port this PR|
+|`fw=default`|forward-ports this PR normally|
+|`fw=skipci`|does not wait for a forward-port's statuses to succeed before creating the next one|
+|`up to <branch>`|only ports this PR forward to the specified branch (included)|
+|`merge`|integrate the PR with a simple merge commit, using the PR description as message|
+|`rebase-merge`|rebases the PR on top of the target branch the integrates with a merge commit, using the PR description as message|
+|`rebase-ff`|rebases the PR on top of the target branch, then fast-forwards|
+|`squash`|squashes the PR as a single commit on the target branch, using the PR description as message|
+|`delegate+`|grants approval rights to the PR author|
+|`delegate=<...>`|grants approval rights on this PR to the specified github users|
+|`default`|stages the PR normally|
+|`priority`|tries to stage this PR first, then adds `default` PRs if the staging has room|
+|`alone`|stages this PR only with other PRs of the same priority|
+{skip}\
+|`cancel=staging`|automatically cancels the current staging when this PR becomes ready|
+|`check`|fetches or refreshes PR metadata, resets mergebot state|
+
+Note: this help text is dynamic and will change with the state of the PR.\
+"""
+AUTHOR = """\
+Currently available commands for @{user}:
+
+|command||
+|-|-|
+|`help`|displays this help|
+|`fw=no`|does not forward-port this PR|
+|`up to <branch>`|only ports this PR forward to the specified branch (included)|
+|`check`|fetches or refreshes PR metadata, resets mergebot state|
+
+Note: this help text is dynamic and will change with the state of the PR.\
+"""
+RANDO = """\
+Currently available commands for @{user}:
+
+|command||
+|-|-|
+|`help`|displays this help|
+
+Note: this help text is dynamic and will change with the state of the PR.\
+"""