diff --git a/conftest.py b/conftest.py index 28980399..9572cea3 100644 --- a/conftest.py +++ b/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 diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 5e59c008..3e7bb9ed 100644 --- a/forwardport/tests/test_limit.py +++ b/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)=`|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 `|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"), diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py index 1ebc5fb6..684209e3 100644 --- a/runbot_merge/models/commands.py +++ b/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)=", "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 ", "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() diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 9adb5ae0..a0a44ebc 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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) - _source_admin, source_reviewer, source_author = self.source_id._pr_acl(author) + if any(isinstance(cmd, commands.Help) for cmd in cmds): + 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): diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index d64596e6..9133e48b 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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'], "@{reviewer} unknown command '@bobby-b'.\n\nFor your own safety I've ignored *everything in your entire comment*.".format_map(users)), + (users['user'], """\ +@{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)=`|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 `|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)=`|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 `|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: diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 07e42786..35a6bcbc 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/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 diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 081bc3e3..1454165b 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/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") - pr = repo.make_pr(title='title', body='body', target='master', head=c) + repo.make_commits(m, Commit('first', tree={'m': 'c1'}), ref="heads/other") + 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)=`|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 `|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 `|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.\ +"""