From 313d405a26f509967ba0b99dd53e1eb4f115d029 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 16 Oct 2018 12:40:45 +0200 Subject: [PATCH] [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 | 51 +++++-------- runbot_merge/models/pull_requests.py | 107 +++++++++++++++++++++++---- runbot_merge/tests/remote.py | 4 + runbot_merge/tests/test_basic.py | 78 ++++++++++++++++++- 4 files changed, 192 insertions(+), 48 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index f2b52972..49a6323b 100644 --- a/runbot_merge/controllers/__init__.py +++ b/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), ]) - 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) + return _handle_comment(env, repo, issue, author, 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)]) - 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, event['pull_request']['number'], - event['pull_request']['base']['ref'] - ) - if not pr: - return "Unknown PR, scheduling fetch" - - return pr._parse_commands(partner, comment) + return _handle_comment( + env, repo, pr, author, comment, + target=event['pull_request']['base']['ref']) 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) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 89597e07..4285e9bd 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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': - ok = True - self.state = 'ready' + 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 diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index dff70c0b..a518e51e 100644 --- a/runbot_merge/tests/remote.py +++ b/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') diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 8deaa87e..1b573558 100644 --- a/runbot_merge/tests/test_basic.py +++ b/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