[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

@@ -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),
     }
 
 

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()

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':

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 'do'.\n\nFor your own safety I've ignored *everything in your entire comment*.".format_map(users)),
-            (users['user'], "@{reviewer} unknown command '@bobby-b'".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: