From b109225f44d0d9548e08637203dd4354a2d4837c Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 21 Jun 2024 14:38:47 +0200 Subject: [PATCH] [IMP] runbot_merge: quality of feedback on errorneous commands - When a redundant approval is sent to a PR, notify but don't ignore the entire command set, there's no actual risk. - Indicate that the entire comment was ignored when finding something which does not parse. Fixes #892, fixes #893 --- forwardport/tests/test_limit.py | 4 ++-- runbot_merge/models/commands.py | 2 +- runbot_merge/models/pull_requests.py | 18 +++++++++++++----- runbot_merge/tests/test_basic.py | 6 +++--- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 14645c26..5e59c008 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -129,15 +129,15 @@ def test_disable(env, config, make_repo, users): # use a set because git webhooks delays might lead to mis-ordered # responses and we don't care that much 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.".format_map(users)), + (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['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"), (users['user'], "@{reviewer} there is no branch 'foo', it can't be used as a forward port target.".format_map(users)), (users['reviewer'], "hansen up to c"), (users['user'], "Forward-porting to 'c'."), - seen(env, pr, users), } diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py index 2911638b..1ebc5fb6 100644 --- a/runbot_merge/models/commands.py +++ b/runbot_merge/models/commands.py @@ -305,7 +305,7 @@ class Parser: if limit := next(self.it, None): return Limit(limit) else: - raise CommandError("please provide a branch to forward-port to.") + raise CommandError("please provide a branch to forward-port to") def parse_close(self) -> Close: return Close() diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 9b57924b..d090eef4 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -656,7 +656,7 @@ class PullRequests(models.Model): utils.shorten(comment['body'] or '', 50), exc_info=True ) - feedback(message=f"@{login} {e.args[0]}") + feedback(message=f"@{login} {e.args[0]}.\n\nFor your own safety I've ignored *everything in your entire comment*.") return 'error' is_admin, is_reviewer, is_author = self._pr_acl(author) @@ -1002,17 +1002,25 @@ class PullRequests(models.Model): def _approve(self, author, login): oldstate = self.state - newstate = RPLUS.get(self.state) + newstate = RPLUS.get(oldstate) if not author.email: return "I must know your email before you can review PRs. Please contact an administrator." - elif not newstate: - return "this PR is already reviewed, reviewing it again is useless." + + if not newstate: + # Don't fail the entire command if someone tries to approve an + # already-approved PR. + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': "This PR is already reviewed, reviewing it again is useless.", + }) + return None self.reviewed_by = author _logger.debug( "r+ on %s by %s (%s->%s) status=%s message? %s", self.display_name, author.github_login, - oldstate, newstate or oldstate, + oldstate, newstate, self.status, self.status == 'failure' ) if self.status == 'failure': diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 0bf57dd3..79ce42e2 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -3105,7 +3105,7 @@ class TestReviewing: (users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), - (users['user'], "@{} this PR is already reviewed, reviewing it again is useless.".format( + (users['user'], "This PR is already reviewed, reviewing it again is useless.".format( users['reviewer'])), ] @@ -3613,8 +3613,8 @@ 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'".format_map(users)), - (users['user'], "@{reviewer} unknown command '@bobby-b'".format_map(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)), ] class TestRMinus: