[ADD] runbot_merge: various feedback messages

Send reponse comments when users mis-interact with robodoo e.g.
send comments they don't have the right to, or commands which don't make
sense in the PR's state, or tentative interactions with robodoo from
unmanaged PRs.

runbot_merge/controllers/__init__.py

@@ -225,43 +225,18 @@ def handle_comment(env, event):
     comment = event['comment']['body']
     _logger.info('comment: %s %s:%s "%s"', author, repo, issue, comment)
 
-    partner = env['res.partner'].search([('github_login', '=', author), ])
+    return _handle_comment(env, repo, issue, author, comment)
-    if not partner:
-        _logger.info("ignoring comment from %s: not in system", author)
-        return 'ignored'
-
-    repository = env['runbot_merge.repository'].search([('name', '=', repo)])
-    if not repository.project_id._find_commands(comment):
-        return "No commands, ignoring"
-
-    pr = env['runbot_merge.pull_requests']._get_or_schedule(repo, issue)
-    if not pr:
-        return "Unknown PR, scheduling fetch"
-
-    return pr._parse_commands(partner, comment)
 
 def handle_review(env, event):
     repo = event['repository']['full_name']
-    comment = event['review']['body'] or ''
+    pr = event['pull_request']['number']
     author = event['review']['user']['login']
+    comment = event['review']['body'] or ''
+    _logger.info('review: %s %s:%s "%s"', author, repo, pr, comment)
 
-    partner = env['res.partner'].search([('github_login', '=', author)])
+    return _handle_comment(
-    if not partner:
+        env, repo, pr, author, comment,
-        _logger.info('ignoring comment from %s: not in system', author)
+        target=event['pull_request']['base']['ref'])
-        return 'ignored'
-
-    repository = env['runbot_merge.repository'].search([('name', '=', repo)])
-    if not repository.project_id._find_commands(comment):
-        return "No commands, ignoring"
-
-    pr = env['runbot_merge.pull_requests']._get_or_schedule(
-        repo, event['pull_request']['number'],
-        event['pull_request']['base']['ref']
-    )
-    if not pr:
-        return "Unknown PR, scheduling fetch"
-
-    return pr._parse_commands(partner, comment)
 
 def handle_ping(env, event):
     print("Got ping! {}".format(event['zen']))
@@ -274,3 +249,15 @@ EVENTS = {
     'pull_request_review': handle_review,
     'ping': handle_ping,
 }
+
+def _handle_comment(env, repo, issue, author, comment, target=None):
+    repository = env['runbot_merge.repository'].search([('name', '=', repo)])
+    if not repository.project_id._find_commands(comment):
+        return "No commands, ignoring"
+
+    pr = env['runbot_merge.pull_requests']._get_or_schedule(repo, issue, target=target)
+    if not pr:
+        return "Unknown PR, scheduling fetch"
+
+    partner = env['res.partner'].search([('github_login', '=', author), ])
+    return pr._parse_commands(partner, comment, author)

runbot_merge/models/pull_requests.py

@@ -109,6 +109,25 @@ class Project(models.Model):
                 to_remove.extend(ids)
         self.env['runbot_merge.pull_requests.tagging'].browse(to_remove).unlink()
 
+        to_remove = []
+        for f in self.env['runbot_merge.pull_requests.feedback'].search([]):
+            repo = f.repository
+            gh = ghs.get(repo)
+            if not gh:
+                gh = ghs[repo] = repo.github()
+
+            try:
+                gh.comment(f.pull_request, f.message)
+            except Exception:
+                _logger.exception(
+                    "Error while trying to send a comment to %s:%s (%s)",
+                    repo.name, f.pull_request,
+                    f.message and f.message[:200]
+                )
+            else:
+                to_remove.append(f.id)
+        self.env['runbot_merge.pull_requests.feedback'].browse(to_remove).unlink()
+
     def is_timed_out(self, staging):
         return fields.Datetime.from_string(staging.staged_at) + datetime.timedelta(minutes=self.ci_timeout) < datetime.datetime.now()
 
@@ -164,6 +183,11 @@ class Repository(models.Model):
 
         if not self.project_id._has_branch(pr['base']['ref']):
             _logger.info("Tasked with loading PR %d for un-managed branch %s, ignoring", pr['number'], pr['base']['ref'])
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': self.id,
+                'pull_request': number,
+                'message': "I'm sorry. Branch `{}` is not within my remit.".format(pr['base']['ref']),
+            })
             return
 
         controllers.handle_pr(self.env, {
@@ -403,6 +427,11 @@ class PullRequests(models.Model):
             return
 
         if target and not repo.project_id._has_branch(target):
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': repo.id,
+                'pull_request': number,
+                'message': "I'm sorry. Branch `{}` is not within my remit.".format(target),
+            })
             return
 
         pr = self.search([
@@ -427,7 +456,7 @@ class PullRequests(models.Model):
 
         name, flag, param = m.groups()
         if name == 'retry':
-            return ('retry', True)
+            return ('retry', None)
         elif name in ('r', 'review'):
             if flag == '+':
                 return ('review', True)
@@ -449,7 +478,7 @@ class PullRequests(models.Model):
 
         return None
 
-    def _parse_commands(self, author, comment):
+    def _parse_commands(self, author, comment, login):
         """Parses a command string prefixed by Project::github_prefix.
 
         A command string can contain any number of space-separated commands:
@@ -471,18 +500,13 @@ class PullRequests(models.Model):
         """
         assert self, "parsing commands must be executed in an actual PR"
 
+        (login, name) = (author.github_login, author.display_name) if author else (login, 'not in system')
+
         is_admin = (author.reviewer and self.author != author) or (author.self_reviewer and self.author == author)
         is_reviewer = is_admin or self in author.delegate_reviewer
         # TODO: should delegate reviewers be able to retry PRs?
         is_author = is_reviewer or self.author == author
 
-        if not is_author:
-            # no point even parsing commands
-            _logger.info("ignoring comment of %s (%s): no ACL to %s:%s",
-                          author.github_login, author.display_name,
-                          self.repository.name, self.number)
-            return 'ignored'
-
         commands = dict(
             ps
             for m in self.repository.project_id._find_commands(comment)
@@ -497,13 +521,42 @@ class PullRequests(models.Model):
             )
             return 'ok'
 
+        Feedback = self.env['runbot_merge.pull_requests.feedback']
+        if not is_author:
+            # no point even parsing commands
+            _logger.info("ignoring comment of %s (%s): no ACL to %s:%s",
+                          login, name,
+                          self.repository.name, self.number)
+            Feedback.create({
+                'repository': self.repository.id,
+                'pull_request': self.number,
+                'message': "I'm sorry, @{}. I'm afraid I can't do that.".format(login)
+            })
+            return 'ignored'
+
         applied, ignored = [], []
+        def reformat(command, param):
+            if param is None:
+                pstr = ''
+            elif isinstance(param, bool):
+                pstr = '+' if param else '-'
+            elif isinstance(param, list):
+                pstr = '=' + ','.join(param)
+            else:
+                pstr = '={}'.format(param)
+
+            return '%s%s' % (command, pstr)
+        msgs = []
         for command, param in commands.items():
             ok = False
+            msg = []
             if command == 'retry':
-                if is_author and self.state == 'error':
+                if is_author:
+                    if self.state == 'error':
                         ok = True
                         self.state = 'ready'
+                    else:
+                        msg = "Retry makes no sense when the PR is not in error."
             elif command == 'review':
                 if param and is_reviewer:
                     newstate = RPLUS.get(self.state)
@@ -520,6 +573,8 @@ class PullRequests(models.Model):
                                 author.github_login
                             )
                         ok = True
+                    else:
+                        msg = "r- makes no sense in the current PR state."
             elif command == 'delegate':
                 if is_reviewer:
                     ok = True
@@ -546,6 +601,7 @@ class PullRequests(models.Model):
             elif command == 'rebase':
                 # anyone can rebase- their PR I guess?
                 self.rebase = param
+                ok = True
 
             _logger.info(
                 "%s %s(%s) on %s:%s by %s (%s)",
@@ -555,14 +611,24 @@ class PullRequests(models.Model):
                 author.github_login, author.display_name,
             )
             if ok:
-                applied.append('{}({})'.format(command, param))
+                applied.append(reformat(command, param))
             else:
-                ignored.append('{}({})'.format(command, param))
+                ignored.append(reformat(command, param))
+                msgs.append(msg or "You can't {}.".format(reformat(command, param)))
         msg = []
         if applied:
             msg.append('applied ' + ' '.join(applied))
         if ignored:
-            msg.append('ignored ' + ' '.join(ignored))
+            ignoredstr = ' '.join(ignored)
+            msg.append('ignored ' + ignoredstr)
+
+        if msgs:
+            msgs.insert(0, "I'm sorry, @{}.".format(login))
+            Feedback.create({
+                'repository': self.repository.id,
+                'pull_request': self.number,
+                'message': ' '.join(msgs),
+            })
         return '\n'.join(msg)
 
     def _validate(self, statuses):
@@ -744,6 +810,17 @@ class Tagging(models.Model):
         ('error', 'Error'),
     ])
 
+class Feedback(models.Model):
+    """ Queue of feedback comments to send to PR users
+    """
+    _name = 'runbot_merge.pull_requests.feedback'
+
+    repository = fields.Many2one('runbot_merge.repository', required=True)
+    # store the PR number (not id) as we may want to send feedback to PR
+    # objects on non-handled branches
+    pull_request = fields.Integer()
+    message = fields.Char()
+
 class Commit(models.Model):
     """Represents a commit onto which statuses might be posted,
     independent of everything else as commits can be created by

runbot_merge/tests/remote.py

@@ -348,6 +348,10 @@ class Model:
         assert self._model == 'runbot_merge.project'
         self._run_cron('runbot_merge.fetch_prs_cron')
 
+    def _send_feedback(self):
+        assert self._model == 'runbot_merge.project'
+        self._run_cron('runbot_merge.feedback_cron')
+
     def _check_linked_prs_statuses(self):
         assert self._model == 'runbot_merge.pull_requests'
         self._run_cron('runbot_merge.check_linked_prs_status')

runbot_merge/tests/test_basic.py

@@ -672,6 +672,25 @@ class TestRetry:
             ('number', '=', prx.number)
         ]).state == 'merged'
 
+    def test_retry_ignored(self, env, repo, users):
+        """ Check feedback in case of ignored retry command on a non-error PR.
+        """
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        repo.make_ref('heads/master', m)
+
+        c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+        c2 = repo.make_commit(c1, 'second', None, tree={'m': 'c2'})
+        prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user')
+        prx.post_comment('hansen r+', 'reviewer')
+        prx.post_comment('hansen retry', 'reviewer')
+
+        env['runbot_merge.project']._send_feedback()
+        assert prx.comments == [
+            (users['reviewer'], 'hansen r+'),
+            (users['reviewer'], 'hansen retry'),
+            (users['user'], "I'm sorry, @{}. Retry makes no sense when the PR is not in error.".format(users['reviewer'])),
+        ]
+
     @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer'])
     def test_retry_disable(self, env, repo, disabler, users):
         m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
@@ -1579,7 +1598,7 @@ class TestBatching(object):
         assert pr2.state == 'merged'
 
 class TestReviewing(object):
-    def test_reviewer_rights(self, env, repo):
+    def test_reviewer_rights(self, env, repo, users):
         """Only users with review rights will have their r+ (and other
         attributes) taken in account
         """
@@ -1603,6 +1622,13 @@ class TestReviewing(object):
             ('number', '=', prx.number)
         ]).state == 'ready'
 
+        env['runbot_merge.project']._send_feedback()
+        assert prx.comments == [
+            (users['other'], 'hansen r+'),
+            (users['reviewer'], 'hansen r+'),
+            (users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])),
+        ]
+
     def test_self_review_fail(self, env, repo, users):
         """ Normal reviewers can't self-review
         """
@@ -1622,6 +1648,12 @@ class TestReviewing(object):
             ('number', '=', prx.number)
         ]).state == 'validated'
 
+        env['runbot_merge.project']._send_feedback()
+        assert prx.comments == [
+            (users['reviewer'], 'hansen r+'),
+            (users['user'], "I'm sorry, @{}. You can't review+.".format(users['reviewer'])),
+        ]
+
     def test_self_review_success(self, env, repo, users):
         """ Some users are allowed to self-review
         """
@@ -1789,6 +1821,50 @@ class TestUnknownPR:
         env['runbot_merge.project']._check_progress()
         assert pr.staging_id
 
+    def test_rplus_unmanaged(self, env, repo, users):
+        """ r+ on an unmanaged target should notify about
+        """
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'})
+        repo.make_ref('heads/branch', m2)
+
+        c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+        prx = repo.make_pr('title', 'body', target='branch', ctid=c1, user='user')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+
+        prx.post_comment('hansen r+', "reviewer")
+
+        env['runbot_merge.project']._check_fetch()
+        env['runbot_merge.project']._send_feedback()
+
+        assert prx.comments == [
+            (users['reviewer'], 'hansen r+'),
+            (users['user'], "I'm sorry. Branch `branch` is not within my remit."),
+        ]
+
+    def test_rplus_review_unmanaged(self, env, repo, users):
+        """ r+ reviews can take a different path than comments
+        """
+        m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+        m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'})
+        repo.make_ref('heads/branch', m2)
+
+        c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+        prx = repo.make_pr('title', 'body', target='branch', ctid=c1, user='user')
+        repo.post_status(prx.head, 'success', 'legal/cla')
+        repo.post_status(prx.head, 'success', 'ci/runbot')
+
+        prx.post_review('APPROVE', "reviewer", 'hansen r+')
+
+        env['runbot_merge.project']._check_fetch()
+        env['runbot_merge.project']._send_feedback()
+
+        # FIXME: either split out reviews in local or merge reviews & comments in remote
+        assert prx.comments[-1:] == [
+            (users['user'], "I'm sorry. Branch `branch` is not within my remit."),
+        ]
+
 class TestRMinus:
     def test_rminus_approved(self, repo, env):
         """ approved -> r- -> opened