From f430c014c17ba462829bfba908f99b358c4916ab Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 23 Jun 2022 14:25:07 +0200 Subject: [PATCH] [IMP] *: review mergebot & forwardbot messages for pinging Old messages were quite inconsistent in their pinging of the PR author and reviewer. Reviewed messages (probably missed some but...) and try to more consistently ping when the feedback requires some sort of action in order to proceed. Fixes #592 --- conftest.py | 11 -- forwardport/models/forwardport.py | 16 +-- forwardport/models/project.py | 107 ++++++++++---------- forwardport/tests/test_conflicts.py | 11 +- forwardport/tests/test_limit.py | 33 +++--- forwardport/tests/test_simple.py | 28 +++-- forwardport/tests/test_updates.py | 20 ++-- forwardport/tests/test_weird.py | 20 ++-- runbot_merge/changelog/2022-06/pinging.md | 3 + runbot_merge/controllers/__init__.py | 2 +- runbot_merge/models/pull_requests.py | 97 +++++++++++------- runbot_merge/tests/test_basic.py | 61 +++++------ runbot_merge/tests/test_multirepo.py | 28 +++-- runbot_merge/tests/test_status_overrides.py | 2 +- 14 files changed, 245 insertions(+), 194 deletions(-) create mode 100644 runbot_merge/changelog/2022-06/pinging.md diff --git a/conftest.py b/conftest.py index d1b9b427..e800258c 100644 --- a/conftest.py +++ b/conftest.py @@ -582,17 +582,6 @@ class Repo: parents=[p['sha'] for p in gh_commit['parents']], ) - def log(self, ref_or_sha): - for page in itertools.count(1): - r = self._session.get( - 'https://api.github.com/repos/{}/commits'.format(self.name), - params={'sha': ref_or_sha, 'page': page} - ) - assert 200 <= r.status_code < 300, r.json() - yield from map(self._commit_from_gh, r.json()) - if not r.links.get('next'): - return - def read_tree(self, commit): """ read tree object from commit diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 9519e8ef..38479d1f 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -79,12 +79,12 @@ class ForwardPortTasks(models.Model, Queue): batch.active = False -CONFLICT_TEMPLATE = "WARNING: the latest change ({previous.head}) triggered " \ +CONFLICT_TEMPLATE = "{ping}WARNING: the latest change ({previous.head}) triggered " \ "a conflict when updating the next forward-port " \ "({next.display_name}), and has been ignored.\n\n" \ "You will need to update this pull request differently, " \ "or fix the issue by hand on {next.display_name}." -CHILD_CONFLICT = "WARNING: the update of {previous.display_name} to " \ +CHILD_CONFLICT = "{ping}WARNING: the update of {previous.display_name} to " \ "{previous.head} has caused a conflict in this pull request, " \ "data may have been lost." class UpdateQueue(models.Model, Queue): @@ -118,14 +118,15 @@ class UpdateQueue(models.Model, Queue): Feedback.create({ 'repository': child.repository.id, 'pull_request': child.number, - 'message': "Ancestor PR %s has been updated but this PR" + 'message': "%sancestor PR %s has been updated but this PR" " is %s and can't be updated to match." "\n\n" "You may want or need to manually update any" " followup PR." % ( - self.new_root.display_name, - child.state, - ) + child.ping(), + self.new_root.display_name, + child.state, + ) }) return @@ -137,6 +138,7 @@ class UpdateQueue(models.Model, Queue): 'repository': previous.repository.id, 'pull_request': previous.number, 'message': CONFLICT_TEMPLATE.format( + ping=previous.ping(), previous=previous, next=child ) @@ -144,7 +146,7 @@ class UpdateQueue(models.Model, Queue): Feedback.create({ 'repository': child.repository.id, 'pull_request': child.number, - 'message': CHILD_CONFLICT.format(previous=previous, next=child)\ + 'message': CHILD_CONFLICT.format(ping=child.ping(), previous=previous, next=child)\ + (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else '') + (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else '') }) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 49f3e202..af22ef46 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -261,8 +261,11 @@ class PullRequests(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': p.repository.id, 'pull_request': p.number, - 'message': "This PR was modified / updated and has become a normal PR. " - "It should be merged the normal way (via @%s)" % p.repository.project_id.github_prefix, + 'message': "%sthis PR was modified / updated and has become a normal PR. " + "It should be merged the normal way (via @%s)" % ( + p.source_id.ping(), + p.repository.project_id.github_prefix, + ), 'token_field': 'fp_github_token', }) if vals.get('state') == 'merged': @@ -298,7 +301,6 @@ class PullRequests(models.Model): ) return - Feedback = self.env['runbot_merge.pull_requests.feedback'] # TODO: don't use a mutable tokens iterator tokens = iter(tokens) while True: @@ -306,6 +308,7 @@ class PullRequests(models.Model): if token is None: break + ping = False close = False msg = None if token in ('ci', 'skipci'): @@ -314,7 +317,8 @@ class PullRequests(models.Model): pr.fw_policy = token msg = "Not waiting for CI to create followup forward-ports." if token == 'skipci' else "Waiting for CI to create followup forward-ports." else: - msg = "I don't trust you enough to do that @{}.".format(login) + ping = True + msg = "you can't configure ci." if token == 'ignore': # replace 'ignore' by 'up to ' token = 'up' @@ -322,55 +326,51 @@ class PullRequests(models.Model): if token in ('r+', 'review+'): if not self.source_id: - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "I'm sorry, @{}. I can only do this on forward-port PRs and this ain't one.".format(login), - 'token_field': 'fp_github_token', - }) - continue - merge_bot = self.repository.project_id.github_prefix - # don't update the root ever - for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS): - # only the author is delegated explicitely on the - pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login) + ping = True + msg = "I can only do this on forward-port PRs and this is not one, see {}.".format( + self.repository.project_id.github_prefix + ) + else: + merge_bot = self.repository.project_id.github_prefix + # don't update the root ever + for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS): + # only the author is delegated explicitely on the + pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login) elif token == 'close': - msg = "I'm sorry, @{}. I can't close this PR for you.".format( - login) if self.source_id._pr_acl(author).is_reviewer: close = True - msg = None + else: + ping = True + msg = "you can't close PRs." + elif token == 'up' and next(tokens, None) == 'to': limit = next(tokens, None) + ping = True if not self._pr_acl(author).is_author: - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "I'm sorry, @{}. You can't set a forward-port limit.".format(login), - 'token_field': 'fp_github_token', - }) - continue - if not limit: - msg = "Please provide a branch to forward-port to." + msg = "you can't set a forward-port limit.".format(login) + elif not limit: + msg = "please provide a branch to forward-port to." else: limit_id = self.env['runbot_merge.branch'].with_context(active_test=False).search([ ('project_id', '=', self.repository.project_id.id), ('name', '=', limit), ]) if self.source_id: - msg = "Sorry, forward-port limit can only be set on " \ + msg = "forward-port limit can only be set on " \ f"an origin PR ({self.source_id.display_name} " \ "here) before it's merged and forward-ported." elif self.state in ['merged', 'closed']: - msg = "Sorry, forward-port limit can only be set before the PR is merged." + msg = "forward-port limit can only be set before the PR is merged." elif not limit_id: - msg = "There is no branch %r, it can't be used as a forward port target." % limit + msg = "there is no branch %r, it can't be used as a forward port target." % limit elif limit_id == self.target: + ping = False msg = "Forward-port disabled." self.limit_id = limit_id elif not limit_id.fp_enabled: - msg = "Branch %r is disabled, it can't be used as a forward port target." % limit_id.name + msg = "branch %r is disabled, it can't be used as a forward port target." % limit_id.name else: + ping = False msg = "Forward-porting to %r." % limit_id.name self.limit_id = limit_id @@ -382,7 +382,7 @@ class PullRequests(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': self.repository.id, 'pull_request': self.number, - 'message': msg, + 'message': f'@{author.github_login} {msg}' if msg and ping else msg, 'close': close, 'token_field': 'fp_github_token', }) @@ -397,8 +397,8 @@ class PullRequests(models.Model): 'repository': self.repository.id, 'pull_request': self.number, 'token_field': 'fp_github_token', - 'message': '%s\n\n%s failed on this forward-port PR' % ( - self.source_id._pingline(), + 'message': '%s%s failed on this forward-port PR' % ( + self.source_id.ping(), ci, ) }) @@ -578,10 +578,10 @@ class PullRequests(models.Model): 'repository': pr.repository.id, 'pull_request': pr.number, 'token_field': 'fp_github_token', - 'message': "This pull request can not be forward ported: " + 'message': "%sthis pull request can not be forward ported: " "next branch is %r but linked pull request %s " "has a next branch %r." % ( - t.name, linked.display_name, other.name + pr.ping(), t.name, linked.display_name, other.name ) }) _logger.warning( @@ -678,8 +678,8 @@ class PullRequests(models.Model): 'delegates': [(6, False, (source.delegates | pr.delegates).ids)] }) if has_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'): - message = source._pingline() + """ -The next pull request (%s) is in conflict. You can merge the chain up to here by saying + message = source.ping() + """\ +the next pull request (%s) is in conflict. You can merge the chain up to here by saying > @%s r+ %s""" % (new_pr.display_name, pr.repository.project_id.fp_github_name, footer) self.env['runbot_merge.pull_requests.feedback'].create({ @@ -710,19 +710,21 @@ The next pull request (%s) is in conflict. You can merge the chain up to here by '* %s%s\n' % (sha, ' <- on this commit' if sha == h else '') for sha in hh ) - message = f"""{source._pingline()} cherrypicking of pull request {source.display_name} failed. + message = f"""{source.ping()}cherrypicking of pull request {source.display_name} failed. {lines}{sout}{serr} Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). In the former case, you may want to edit this PR message as well. """ elif has_conflicts: - message = """%s -While this was properly forward-ported, at least one co-dependent PR (%s) did not succeed. You will need to fix it before this can be merged. + message = """%s\ +while this was properly forward-ported, at least one co-dependent PR (%s) did \ +not succeed. You will need to fix it before this can be merged. -Both this PR and the others will need to be approved via `@%s r+` as they are all considered "in conflict". +Both this PR and the others will need to be approved via `@%s r+` as they are \ +all considered "in conflict". %s""" % ( - source._pingline(), + source.ping(), ', '.join(p.display_name for p in (new_batch - new_pr)), proj.github_prefix, footer @@ -733,8 +735,8 @@ Both this PR and the others will need to be approved via `@%s r+` as they are al for p in pr._iter_ancestors() if p.parent_id ) - message = source._pingline() + """ -This PR targets %s and is the last of the forward-port chain%s + message = source.ping() + """\ +this PR targets %s and is the last of the forward-port chain%s %s To merge the full chain, say > @%s r+ @@ -772,14 +774,6 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr b.prs[0]._schedule_fp_followup() return b - def _pingline(self): - assignees = (self.author | self.reviewed_by).mapped('github_login') - return "Ping %s" % ', '.join( - '@' + login - for login in assignees - if login - ) - def _create_fp_branch(self, target_branch, fp_branch_name, cleanup): """ Creates a forward-port for the current PR to ``target_branch`` under ``fp_branch_name``. @@ -1084,8 +1078,9 @@ stderr: self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': source.repository.id, 'pull_request': source.number, - 'message': "This pull request has forward-port PRs awaiting action (not merged or closed): %s" % ', '.join( - pr.display_name for pr in sorted(prs, key=lambda p: p.number) + 'message': "%sthis pull request has forward-port PRs awaiting action (not merged or closed):\n%s" % ( + source.ping(), + '\n- '.join(pr.display_name for pr in sorted(prs, key=lambda p: p.number)) ), 'token_field': 'fp_github_token', }) diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index 739c27b9..ea6c2844 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -73,8 +73,8 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port '''), - (users['user'], """Ping @%s, @%s -The next pull request (%s) is in conflict. You can merge the chain up to here by saying + (users['user'], """@%s @%s the next pull request (%s) is in conflict. \ +You can merge the chain up to here by saying > @%s r+ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port @@ -343,11 +343,12 @@ b assert pr2.comments == [ seen(env, pr2, users), - (users['user'], re_matches(r'Ping.*CONFLICT', re.DOTALL)), + (users['user'], re_matches(r'@%s @%s .*CONFLICT' % (users['user'], users['reviewer']), re.DOTALL)), (users['reviewer'], 'hansen r+'), - (users['user'], f"All commits must have author and committer email, " + (users['user'], f"@{users['user']} @{users['reviewer']} unable to stage: " + "All commits must have author and committer email, " f"missing email on {pr2_id.head} indicates the " - f"authorship is most likely incorrect."), + "authorship is most likely incorrect."), ] assert pr2_id.state == 'error' assert not pr2_id.staging_id, "staging should have been rejected" diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 4da92537..8310619d 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import collections +import time import pytest @@ -157,12 +158,12 @@ def test_disable(env, config, make_repo, users, enabled): # responses and we don't care that much assert set(pr.comments) == { (users['reviewer'], "hansen r+\n%s up to" % bot_name), + (users['other'], "@%s please provide a branch to forward-port to." % users['reviewer']), (users['reviewer'], "%s up to b" % bot_name), + (users['other'], "@%s branch 'b' is disabled, it can't be used as a forward port target." % users['reviewer']), (users['reviewer'], "%s up to foo" % bot_name), + (users['other'], "@%s there is no branch 'foo', it can't be used as a forward port target." % users['reviewer']), (users['reviewer'], "%s up to c" % bot_name), - (users['other'], "Please provide a branch to forward-port to."), - (users['other'], "Branch 'b' is disabled, it can't be used as a forward port target."), - (users['other'], "There is no branch 'foo', it can't be used as a forward port target."), (users['other'], "Forward-porting to 'c'."), seen(env, pr, users), } @@ -201,14 +202,13 @@ def test_default_disabled(env, config, make_repo, users): assert pr2.comments == [ seen(env, pr2, users), (users['user'], """\ -Ping @%s, @%s -This PR targets b and is the last of the forward-port chain. +@%(user)s @%(reviewer)s this PR targets b and is the last of the forward-port chain. To merge the full chain, say -> @%s r+ +> @%(user)s r+ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""" % (users['user'], users['reviewer'], users['user'])), +""" % users) ] def test_limit_after_merge(env, config, make_repo, users): @@ -247,7 +247,7 @@ def test_limit_after_merge(env, config, make_repo, users): (users['reviewer'], "hansen r+"), seen(env, pr1, users), (users['reviewer'], bot_name + ' up to b'), - (bot_name, "Sorry, forward-port limit can only be set before the PR is merged."), + (bot_name, "@%s forward-port limit can only be set before the PR is merged." % users['reviewer']), ] assert pr2.comments == [ seen(env, pr2, users), @@ -257,9 +257,11 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port """), (users['reviewer'], bot_name + ' up to b'), - (bot_name, "Sorry, forward-port limit can only be set on an origin PR" - " (%s here) before it's merged and forward-ported." % p1.display_name - ), + (bot_name, "@%s forward-port limit can only be set on an origin PR" + " (%s here) before it's merged and forward-ported." % ( + users['reviewer'], + p1.display_name, + )), ] # update pr2 to detach it from pr1 @@ -279,10 +281,13 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port env.run_crons() assert pr2.comments[4:] == [ - (bot_name, "This PR was modified / updated and has become a normal PR. " - "It should be merged the normal way (via @hansen)"), + (bot_name, "@%s @%s this PR was modified / updated and has become a normal PR. " + "It should be merged the normal way (via @%s)" % ( + users['user'], users['reviewer'], + p2.repository.project_id.github_prefix + )), (users['reviewer'], bot_name + ' up to b'), - (bot_name, "Sorry, forward-port limit can only be set on an origin PR " + (bot_name, f"@{users['reviewer']} forward-port limit can only be set on an origin PR " f"({p1.display_name} here) before it's merged and forward-ported." ), ] diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 871d6904..43fb17c5 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -125,8 +125,11 @@ def test_straightforward_flow(env, config, make_repo, users): assert pr.comments == [ (users['reviewer'], 'hansen r+ rebase-ff'), seen(env, pr, users), - (users['user'], 'Merge method set to rebase and fast-forward'), - (users['user'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + ', '.join((pr1 | pr2).mapped('display_name'))), + (users['user'], 'Merge method set to rebase and fast-forward.'), + (users['user'], '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % ( + users['other'], users['reviewer'], + '\n- '.join((pr1 | pr2).mapped('display_name')) + )), ] assert pr0_ == pr0 @@ -148,8 +151,7 @@ def test_straightforward_flow(env, config, make_repo, users): assert pr2_remote.comments == [ seen(env, pr2_remote, users), (users['user'], """\ -Ping @%s, @%s -This PR targets c and is the last of the forward-port chain containing: +@%s @%s this PR targets c and is the last of the forward-port chain containing: * %s To merge the full chain, say @@ -314,11 +316,18 @@ def test_empty(env, config, make_repo, users): env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + awaiting = ( + users['other'], + '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % ( + users['user'], users['reviewer'], + fail_id.display_name + ) + ) assert pr1.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1, users), - (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), - (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), + awaiting, + awaiting, ], "each cron run should trigger a new message on the ancestor" # check that this stops if we close the PR with prod: @@ -327,8 +336,8 @@ def test_empty(env, config, make_repo, users): assert pr1.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1, users), - (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), - (users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name), + awaiting, + awaiting, ] def test_partially_empty(env, config, make_repo): @@ -562,8 +571,7 @@ def test_delegate_fw(env, config, make_repo, users): assert pr2.comments == [ seen(env, pr2, users), - (users['user'], '''Ping @{self_reviewer}, @{reviewer} -This PR targets c and is the last of the forward-port chain. + (users['user'], '''@{self_reviewer} @{reviewer} this PR targets c and is the last of the forward-port chain. To merge the full chain, say > @{user} r+ diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index 9a35c96a..4f689246 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -44,7 +44,7 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port ''') - ci_warning = (users['user'], 'Ping @%(user)s, @%(reviewer)s\n\nci/runbot failed on this forward-port PR' % users) + ci_warning = (users['user'], '@%(user)s @%(reviewer)s ci/runbot failed on this forward-port PR' % users) # oh no CI of the first FP PR failed! # simulate status being sent multiple times (e.g. on multiple repos) with @@ -103,7 +103,11 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port assert pr1_remote.comments == [ seen(env, pr1_remote, users), fp_intermediate, ci_warning, ci_warning, - (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1_id.repository.project_id.github_prefix), + (users['user'], "@%s @%s this PR was modified / updated and has become a normal PR. " + "It should be merged the normal way (via @%s)" % ( + users['user'], users['reviewer'], + pr1_id.repository.project_id.github_prefix + )), ], "users should be warned that the PR has become non-FP" # NOTE: should the followup PR wait for pr1 CI or not? assert pr2_id.head != pr2_head @@ -210,9 +214,13 @@ def test_update_merged(env, make_repo, config, users): More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port '''), (users['reviewer'], 'hansen r+'), - (users['user'], """Ancestor PR %s has been updated but this PR is merged and can't be updated to match. + (users['user'], """@%s @%s ancestor PR %s has been updated but this PR is merged and can't be updated to match. -You may want or need to manually update any followup PR.""" % pr1_id.display_name) +You may want or need to manually update any followup PR.""" % ( + users['user'], + users['reviewer'], + pr1_id.display_name, + )) ] def test_duplicate_fw(env, make_repo, setreviewers, config, users): @@ -377,7 +385,7 @@ conflict! # 2. "forward port chain" bit # 3. updated / modified & got detached assert pr2.comments[3:] == [ - (users['user'], f"WARNING: the latest change ({pr2_id.head}) triggered " + (users['user'], f"@{users['user']} WARNING: the latest change ({pr2_id.head}) triggered " f"a conflict when updating the next forward-port " f"({pr3_id.display_name}), and has been ignored.\n\n" f"You will need to update this pull request " @@ -389,7 +397,7 @@ conflict! # 2. forward-port chain thing assert repo.get_pr(pr3_id.number).comments[2:] == [ (users['user'], re_matches(f'''\ -WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \ +@{users['user']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \ conflict in this pull request, data may have been lost. stdout: diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index b569f66d..346039fa 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -399,18 +399,20 @@ class TestNotAllBranches: assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_a, users), - (users['user'], "This pull request can not be forward ported: next " - "branch is 'b' but linked pull request %s#%d has a" - " next branch 'c'." % (b.name, pr_b.number) - ) + (users['user'], "@%s @%s this pull request can not be forward ported:" + " next branch is 'b' but linked pull request %s " + "has a next branch 'c'." % ( + users['user'], users['reviewer'], pr_b_id.display_name, + )), ] assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_b, users), - (users['user'], "This pull request can not be forward ported: next " - "branch is 'c' but linked pull request %s#%d has a" - " next branch 'b'." % (a.name, pr_a.number) - ) + (users['user'], "@%s @%s this pull request can not be forward ported:" + " next branch is 'c' but linked pull request %s " + "has a next branch 'b'." % ( + users['user'], users['reviewer'], pr_a_id.display_name, + )), ] def test_new_intermediate_branch(env, config, make_repo): @@ -755,7 +757,7 @@ def test_approve_draft(env, config, make_repo, users): assert pr.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr, users), - (users['user'], f"I'm sorry, @{users['reviewer']}. Draft PRs can not be approved."), + (users['user'], f"I'm sorry, @{users['reviewer']}: draft PRs can not be approved."), ] with prod: diff --git a/runbot_merge/changelog/2022-06/pinging.md b/runbot_merge/changelog/2022-06/pinging.md new file mode 100644 index 00000000..d82cc56a --- /dev/null +++ b/runbot_merge/changelog/2022-06/pinging.md @@ -0,0 +1,3 @@ +IMP: review pinging (`@`-notification) of users by the mergebot and forwardbot + +The bots should more consistently ping users when they need some sort of action to proceed. diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index d96d703d..6dad2666 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -218,7 +218,7 @@ def handle_pr(env, event): if pr_obj.state == 'merged': feedback( close=True, - message="@%s ya silly goose you can't reopen a PR that's been merged PR." % event['sender']['login'] + message="@%s ya silly goose you can't reopen a merged PR." % event['sender']['login'] ) if pr_obj.state == 'closed': diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index f2c5f792..93c5a378 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -3,6 +3,7 @@ import ast import base64 import collections +import contextlib import datetime import io import itertools @@ -116,7 +117,7 @@ All substitutions are tentatively applied sequentially to the input. feedback({ 'repository': self.id, 'pull_request': number, - 'message': "I'm sorry. Branch `{}` is not within my remit.".format(pr['base']['ref']), + 'message': "Branch `{}` is not within my remit, imma just ignore it.".format(pr['base']['ref']), }) return @@ -142,8 +143,9 @@ All substitutions are tentatively applied sequentially to the input. feedback({ 'repository': self.id, 'pull_request': number, - 'message': "Sorry, I didn't know about this PR and had to retrieve " - "its information, you may have to re-approve it." + 'message': "%sI didn't know about this PR and had to retrieve " + "its information, you may have to re-approve it as " + "I didn't see previous commands." % pr_id.ping() }) # init the PR to the null commit so we can later synchronise it back # back to the "proper" head while resetting reviews @@ -338,14 +340,20 @@ class Branch(models.Model): _logger.exception("Failed to merge %s into staging branch", pr.display_name) if first or isinstance(e, exceptions.Unmergeable): if len(e.args) > 1 and e.args[1]: - message = e.args[1] + reason = e.args[1] else: - message = "Unable to stage PR (%s)" % e.__context__ + reason = e.__context__ + # if the reason is a json document, assume it's a github + # error and try to extract the error message to give it to + # the user + with contextlib.suppress(Exception): + reason = json.loads(str(reason))['message'].lower() + pr.state = 'error' self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message': message, + 'message': f'{pr.ping()}unable to stage: {reason}', }) else: first = False @@ -534,6 +542,17 @@ class PullRequests(models.Model): repo_name = fields.Char(related='repository.name') message_title = fields.Char(compute='_compute_message_title') + def ping(self, author=True, reviewer=True): + P = self.env['res.partner'] + s = ' '.join( + f'@{p.github_login}' + for p in (self.author if author else P) | (self.reviewed_by if reviewer else P) + if p + ) + if s: + s += ' ' + return s + @api.depends('repository.name', 'number') def _compute_url(self): base = werkzeug.urls.url_parse(self.env['ir.config_parameter'].sudo().get_param('web.base.url', 'http://localhost:8069')) @@ -791,14 +810,14 @@ class PullRequests(models.Model): msgs = [] for command, param in commands: ok = False - msg = [] + msg = None if command == 'retry': 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." + msg = "retry makes no sense when the PR is not in error." elif command == 'check': if is_author: self.env['runbot_merge.fetch_job'].create({ @@ -808,14 +827,14 @@ class PullRequests(models.Model): ok = True elif command == 'review': if self.draft: - msg = "Draft PRs can not be approved." + msg = "draft PRs can not be approved." elif param and is_reviewer: oldstate = self.state newstate = RPLUS.get(self.state) if not author.email: msg = "I must know your email before you can review PRs. Please contact an administrator." elif not newstate: - msg = "This PR is already reviewed, reviewing it again is useless." + msg = "this PR is already reviewed, reviewing it again is useless." else: self.state = newstate self.reviewed_by = author @@ -832,7 +851,7 @@ class PullRequests(models.Model): Feedback.create({ 'repository': self.repository.id, 'pull_request': self.number, - 'message': "@{}, you may want to rebuild or fix this PR as it has failed CI.".format(author.github_login), + 'message': "@{} you may want to rebuild or fix this PR as it has failed CI.".format(login), }) elif not param and is_author: newstate = RMINUS.get(self.state) @@ -846,7 +865,7 @@ class PullRequests(models.Model): 'pull_request': self.number, 'message': "PR priority reset to 1, as pull requests with priority 0 ignore review state.", }) - self.unstage("unreviewed (r-) by %s", author.github_login) + self.unstage("unreviewed (r-) by %s", login) ok = True else: msg = "r- makes no sense in the current PR state." @@ -875,7 +894,7 @@ class PullRequests(models.Model): elif command == 'method': if is_reviewer: if param == 'squash' and not self.squash: - msg = "Squash can only be used with a single commit at this time." + msg = "squash can only be used with a single commit at this time." else: self.merge_method = param ok = True @@ -883,7 +902,7 @@ class PullRequests(models.Model): Feedback.create({ 'repository': self.repository.id, 'pull_request': self.number, - 'message':"Merge method set to %s" % explanation + 'message':"Merge method set to %s." % explanation }) elif command == 'override': overridable = author.override_rights\ @@ -905,7 +924,7 @@ class PullRequests(models.Model): c.create({'sha': self.head, 'statuses': '{}'}) ok = True else: - msg = f"You are not allowed to override this status." + msg = "you are not allowed to override this status." else: # ignore unknown commands continue @@ -920,21 +939,23 @@ class PullRequests(models.Model): applied.append(reformat(command, param)) else: ignored.append(reformat(command, param)) - msgs.append(msg or "You can't {}.".format(reformat(command, param))) + msgs.append(msg or "you can't {}.".format(reformat(command, param))) + + if msgs: + joiner = ' ' if len(msgs) == 1 else '\n- ' + msgs.insert(0, "I'm sorry, @{}:".format(login)) + Feedback.create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': joiner.join(msgs), + }) + msg = [] if applied: msg.append('applied ' + ' '.join(applied)) if 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 _pr_acl(self, user): @@ -1021,7 +1042,7 @@ class PullRequests(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': self.repository.id, 'pull_request': self.number, - 'message': "%r failed on this reviewed PR." % ci, + 'message': "%s%r failed on this reviewed PR." % (self.ping(), ci), }) def _auto_init(self): @@ -1158,11 +1179,9 @@ class PullRequests(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': r.repository.id, 'pull_request': r.number, - 'message': "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( - ', '.join(map( - '{0.display_name}'.format, - unready - )) + 'message': "{}linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( + r.ping(), + ', '.join(map('{0.display_name}'.format, unready)) ) }) r.link_warned = True @@ -1171,6 +1190,11 @@ class PullRequests(models.Model): # send feedback for multi-commit PRs without a merge_method (which # we've not warned yet) + methods = ''.join( + '* `%s` to %s\n' % pair + for pair in type(self).merge_method.selection + if pair[0] != 'squash' + ) for r in self.search([ ('state', '=', 'ready'), ('squash', '=', False), @@ -1180,10 +1204,9 @@ class PullRequests(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': r.repository.id, 'pull_request': r.number, - 'message': "Because this PR has multiple commits, I need to know how to merge it:\n\n" + ''.join( - '* `%s` to %s\n' % pair - for pair in type(self).merge_method.selection - if pair[0] != 'squash' + 'message': "%sbecause this PR has multiple commits, I need to know how to merge it:\n\n%s" % ( + r.ping(), + methods, ) }) r.method_warned = True @@ -1740,7 +1763,7 @@ class Stagings(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message':"Staging failed: %s" % message + 'message': "%sstaging failed: %s" % (pr.ping(), message), }) self.batch_ids.write({'active': False}) @@ -2042,11 +2065,11 @@ class Batch(models.Model): self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message': "We apparently missed an update to this PR " + 'message': "%swe apparently missed an update to this PR " "and tried to stage it in a state which " "might not have been approved. PR has been " "updated to %s, please check and approve or " - "re-approve." % new_head + "re-approve." % (pr.ping(), new_head) }) return self.env['runbot_merge.batch'] diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 4ebaa6f2..fe053bb4 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -63,7 +63,6 @@ def test_trivial_flow(env, repo, page, users, config): ] assert statuses == [('legal/cla', 'ok'), ('ci/runbot', 'ok')] - with repo: pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token']) assert pr_id.state == 'ready' @@ -443,8 +442,8 @@ def test_staging_conflict_first(env, repo, users, config, page): assert pr.comments == [ (users['reviewer'], 'hansen r+ rebase-merge'), seen(env, pr, users), - (users['user'], 'Merge method set to rebase and merge, using the PR as merge commit message'), - (users['user'], re_matches('^Unable to stage PR')), + (users['user'], 'Merge method set to rebase and merge, using the PR as merge commit message.'), + (users['user'], '@%(user)s @%(reviewer)s unable to stage: merge conflict' % users), ] dangerbox = pr_page(page, pr).cssselect('.alert-danger span') @@ -566,8 +565,8 @@ def test_staging_ci_failure_single(env, repo, users, config, page): assert pr.comments == [ (users['reviewer'], 'hansen r+ rebase-merge'), seen(env, pr, users), - (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"), - (users['user'], 'Staging failed: ci/runbot') + (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message."), + (users['user'], '@%(user)s @%(reviewer)s staging failed: ci/runbot' % users) ] dangerbox = pr_page(page, pr).cssselect('.alert-danger span') @@ -970,9 +969,9 @@ def test_ci_failure_after_review(env, repo, users, config): assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)), - (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), - (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)), + (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)), + (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), + (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), ] def test_reopen_merged_pr(env, repo, config, users): @@ -1016,7 +1015,7 @@ def test_reopen_merged_pr(env, repo, config, users): assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], "@%s ya silly goose you can't reopen a PR that's been merged PR." % users['other']) + (users['user'], "@%s ya silly goose you can't reopen a merged PR." % users['other']) ] class TestNoRequiredStatus: @@ -1212,7 +1211,7 @@ class TestRetry: (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen retry'), seen(env, prx, users), - (users['user'], "I'm sorry, @{}. Retry makes no sense when the PR is not in error.".format(users['reviewer'])), + (users['user'], "I'm sorry, @{reviewer}: retry makes no sense when the PR is not in error.".format_map(users)), ] @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer']) @@ -1408,12 +1407,13 @@ class TestMergeMethod: assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], """Because this PR has multiple commits, I need to know how to merge it: + (users['user'], """@{user} @{reviewer} because this PR has multiple \ +commits, I need to know how to merge it: * `merge` to merge directly, using the PR as merge commit message * `rebase-merge` to rebase and merge, using the PR as merge commit message * `rebase-ff` to rebase and fast-forward -"""), +""".format_map(users)), ] def test_pr_method_no_review(self, repo, env, users, config): @@ -1453,11 +1453,11 @@ class TestMergeMethod: assert prx.comments == [ (users['reviewer'], 'hansen rebase-merge'), seen(env, prx, users), - (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"), + (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message."), (users['reviewer'], 'hansen merge'), - (users['user'], "Merge method set to merge directly, using the PR as merge commit message"), + (users['user'], "Merge method set to merge directly, using the PR as merge commit message."), (users['reviewer'], 'hansen rebase-ff'), - (users['user'], "Merge method set to rebase and fast-forward"), + (users['user'], "Merge method set to rebase and fast-forward."), ] def test_pr_rebase_merge(self, repo, env, users, config): @@ -2025,7 +2025,7 @@ Part-of: {pr_id.display_name}""" assert pr1.comments == [ seen(env, pr1, users), (users['reviewer'], 'hansen r+ squash'), - (users['user'], 'Merge method set to squash') + (users['user'], 'Merge method set to squash.') ] merged_head = repo.commit('master') assert merged_head.message == f"""first pr @@ -2049,13 +2049,13 @@ Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\ assert pr2.comments == [ seen(env, pr2, users), (users['reviewer'], 'hansen r+ squash'), - (users['user'], f"I'm sorry, @{users['reviewer']}. Squash can only be used with a single commit at this time."), - (users['user'], """Because this PR has multiple commits, I need to know how to merge it: + (users['user'], f"I'm sorry, @{users['reviewer']}: squash can only be used with a single commit at this time."), + (users['user'], """@{user} @{reviewer} because this PR has multiple commits, I need to know how to merge it: * `merge` to merge directly, using the PR as merge commit message * `rebase-merge` to rebase and merge, using the PR as merge commit message * `rebase-ff` to rebase and fast-forward -""") +""".format_map(users)) ] @pytest.mark.xfail(reason="removed support for squash- command") @@ -2859,7 +2859,7 @@ class TestReviewing(object): (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'], "I'm sorry, @{}. This PR is already reviewed, reviewing it again is useless.".format( + (users['user'], "I'm sorry, @{}: this PR is already reviewed, reviewing it again is useless.".format( users['reviewer'])), ] @@ -2888,7 +2888,7 @@ class TestReviewing(object): assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], "I'm sorry, @{}. You can't review+.".format(users['reviewer'])), + (users['user'], "I'm sorry, @{}: you can't review+.".format(users['reviewer'])), ] def test_self_review_success(self, env, repo, users, config): @@ -3044,7 +3044,7 @@ class TestReviewing(object): seen(env, pr, users), (users['reviewer'], 'hansen delegate+'), (users['user'], 'hansen r+'), - (users['user'], f"I'm sorry, @{users['user']}. I must know your email before you can review PRs. Please contact an administrator."), + (users['user'], f"I'm sorry, @{users['user']}: I must know your email before you can review PRs. Please contact an administrator."), ] user_partner.fetch_github_email() assert user_partner.email @@ -3108,9 +3108,9 @@ class TestUnknownPR: seen(env, prx, users), (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen r+'), - (users['user'], "Sorry, I didn't know about this PR and had to " + (users['user'], "I didn't know about this PR and had to " "retrieve its information, you may have to " - "re-approve it."), + "re-approve it as I didn't see previous commands."), seen(env, prx, users), ] @@ -3161,9 +3161,9 @@ class TestUnknownPR: assert pr.comments == [ seen(env, pr, users), (users['reviewer'], 'hansen r+'), - (users['user'], "Sorry, I didn't know about this PR and had to " - "retrieve its information, you may have to " - "re-approve it."), + (users['user'], "I didn't know about this PR and had to retrieve " + "its information, you may have to re-approve it " + "as I didn't see previous commands."), seen(env, pr, users), ] @@ -3189,7 +3189,7 @@ class TestUnknownPR: assert prx.comments == [ (users['reviewer'], 'hansen r+'), (users['user'], "This PR targets the un-managed branch %s:branch, it can not be merged." % repo.name), - (users['user'], "I'm sorry. Branch `branch` is not within my remit."), + (users['user'], "Branch `branch` is not within my remit, imma just ignore it."), ] def test_rplus_review_unmanaged(self, env, repo, users, config): @@ -3586,7 +3586,7 @@ class TestFeedback: assert prx.comments == [ (users['reviewer'], 'hansen r+'), seen(env, prx, users), - (users['user'], "'ci/runbot' failed on this reviewed PR.") + (users['user'], "@%(user)s @%(reviewer)s 'ci/runbot' failed on this reviewed PR." % users) ] def test_review_failed(self, repo, env, users, config): @@ -3616,8 +3616,9 @@ class TestFeedback: assert prx.comments == [ seen(env, prx, users), (users['reviewer'], 'hansen r+'), - (users['user'], "@%s, you may want to rebuild or fix this PR as it has failed CI." % users['reviewer']) + (users['user'], "@%s you may want to rebuild or fix this PR as it has failed CI." % users['reviewer']) ] + class TestInfrastructure: def test_protection(self, repo): """ force-pushing on a protected ref should fail diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index a51d99b7..96c61423 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -429,7 +429,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config): assert pr1b.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1b, users), - (users['user'], re_matches('^Unable to stage PR')), + (users['user'], '@%(user)s @%(reviewer)s unable to stage: merge conflict' % users), ] other = to_pr(env, pr1a) reviewer = get_partner(env, users["reviewer"]).formatted_email @@ -527,14 +527,22 @@ class TestCompanionsNotReady: assert p_a.comments == [ (users['reviewer'], 'hansen r+'), seen(env, p_a, users), - (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), + (users['user'], "@%s @%s linked pull request(s) %s not ready. Linked PRs are not staged until all of them are ready." % ( + users['user'], + users['reviewer'], + pr_b.display_name, + )), ] # ensure the message is only sent once per PR env.run_crons('runbot_merge.check_linked_prs_status') assert p_a.comments == [ (users['reviewer'], 'hansen r+'), seen(env, p_a, users), - (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), + (users['user'], "@%s @%s linked pull request(s) %s not ready. Linked PRs are not staged until all of them are ready." % ( + users['user'], + users['reviewer'], + pr_b.display_name, + )), ] assert p_b.comments == [seen(env, p_b, users)] @@ -570,7 +578,8 @@ class TestCompanionsNotReady: assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_b, users), - (users['user'], "Linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + (users['user'], "@%s @%s linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + users['user'], users['reviewer'], repo_a.name, pr_a.number, repo_c.name, pr_c.number )) @@ -609,7 +618,8 @@ class TestCompanionsNotReady: assert pr_b.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_b, users), - (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + (users['user'], "@%s @%s linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + users['user'], users['reviewer'], repo_a.name, pr_a.number )) ] @@ -617,7 +627,8 @@ class TestCompanionsNotReady: (users['reviewer'], 'hansen r+'), seen(env, pr_c, users), (users['user'], - "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + "@%s @%s linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + users['user'], users['reviewer'], repo_a.name, pr_a.number )) ] @@ -655,7 +666,10 @@ def test_other_failed(env, project, repo_a, repo_b, users, config): assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr_a, users), - (users['user'], 'Staging failed: ci/runbot on %s (view more at http://example.org/b)' % sth) + (users['user'], '@%s @%s staging failed: ci/runbot on %s (view more at http://example.org/b)' % ( + users['user'], users['reviewer'], + sth + )) ] class TestMultiBatches: diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py index 0350a239..9ffe7604 100644 --- a/runbot_merge/tests/test_status_overrides.py +++ b/runbot_merge/tests/test_status_overrides.py @@ -89,7 +89,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config): (users['reviewer'], 'hansen r+'), seen(env, pr, users), (users['reviewer'], 'hansen override=l/int'), - (users['user'], "I'm sorry, @{}. You are not allowed to override this status.".format(users['reviewer'])), + (users['user'], "I'm sorry, @{}: you are not allowed to override this status.".format(users['reviewer'])), (users['other'], "hansen override=l/int"), ] assert pr_id.statuses == '{}'