diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index f9654dad..af8388a4 100644 --- a/bert_e/exceptions.py +++ b/bert_e/exceptions.py @@ -31,6 +31,9 @@ class TemplateException(BertE_Exception): template = None # whether to re-publish if the message is already in the history dont_repeat_if_in_history = -1 + # whether to edit the previous status comment of the bot instead of + # posting a new one + update_status_comment = False def __init__(self, **kwargs): self.kwargs = kwargs @@ -130,18 +133,21 @@ class MismatchPrefixIssueType(TemplateException): class IncorrectFixVersion(TemplateException): code = 112 template = 'incorrect_fix_version.md' + update_status_comment = True status = "failure" class BranchHistoryMismatch(TemplateException): code = 113 template = 'history_mismatch.md' + update_status_comment = True status = "failure" class Conflict(TemplateException): code = 114 template = 'conflict.md' + update_status_comment = True status = "failure" @@ -166,6 +172,7 @@ class AfterPullRequest(TemplateException): class IntegrationDataCreated(InformationException): code = 121 template = 'integration_data_created.md' + update_status_comment = True class UnknownCommand(TemplateException): @@ -226,6 +233,7 @@ class QueueOutOfOrder(TemplateException): class ResetComplete(TemplateException): code = 128 template = "reset_complete.md" + update_status_comment = True class LossyResetWarning(TemplateException): diff --git a/bert_e/git_host/base.py b/bert_e/git_host/base.py index a38ab926..c043c755 100644 --- a/bert_e/git_host/base.py +++ b/bert_e/git_host/base.py @@ -277,6 +277,13 @@ def text(self) -> str: def id(self) -> int: """The comment's ID""" + def edit(self, text: str) -> None: + """Replace the comment's contents with the given text. + + Git hosts that cannot edit comments raise NotImplementedError. + """ + raise NotImplementedError('Comment edition is not supported') + class AbstractPullRequest(metaclass=ABCMeta): @abstractmethod diff --git a/bert_e/git_host/github/__init__.py b/bert_e/git_host/github/__init__.py index 59515b1c..f566e86e 100644 --- a/bert_e/git_host/github/__init__.py +++ b/bert_e/git_host/github/__init__.py @@ -269,7 +269,7 @@ def patch(self, url, data, **kwargs): """ url = self._patch_url(url) - response = self.session.post(url, data=data, **kwargs) + response = self.session.patch(url, data=data, **kwargs) response.raise_for_status() return json.loads(response.text) @@ -1018,6 +1018,11 @@ def id(self) -> int: def delete(self) -> None: self.client.delete(self.data['url']) + def edit(self, text: str) -> None: + updated = type(self).update( + self.client, {'body': text}, url=self.data['url']) + self.data = updated.data + class CheckRun(base.AbstractGitHostObject): GET_URL = '/repos/{owner}/{repo}/check-runs/{id}' diff --git a/bert_e/git_host/mock.py b/bert_e/git_host/mock.py index 02138b94..4e8a26be 100644 --- a/bert_e/git_host/mock.py +++ b/bert_e/git_host/mock.py @@ -409,6 +409,10 @@ class CommentController(Controller, base.AbstractComment): def delete(self): self.controlled.delete() + def edit(self, text): + self.controlled.content = {"raw": text, "markup": "markdown", + "html": text} + @property def author(self): return self['user']['username'].lower() diff --git a/bert_e/tests/unit/test_comment_update.py b/bert_e/tests/unit/test_comment_update.py new file mode 100644 index 00000000..00438034 --- /dev/null +++ b/bert_e/tests/unit/test_comment_update.py @@ -0,0 +1,85 @@ +from types import SimpleNamespace + +import pytest + +from bert_e import exceptions +from bert_e.workflow import pr_utils + + +class FakeComment: + def __init__(self, author, text, can_update=True): + self.author = author + self.text = text + self.can_update = can_update + + def edit(self, text): + if not self.can_update: + raise NotImplementedError + self.text = text + + +class FakePR: + def __init__(self, can_update=True): + self.comments = [] + self.can_update = can_update + + def add_comment(self, msg): + self.comments.append(FakeComment('bert-e', msg, self.can_update)) + + +@pytest.fixture +def settings(): + return SimpleNamespace(no_comment=False, interactive=False, + robot='bert-e') + + +def send(settings, pr, msg, update=True): + pr_utils._send_comment(settings, pr, msg, 0, update) + + +def test_status_comment_is_updated_in_place(settings): + pr = FakePR() + send(settings, pr, 'history conflict') + send(settings, pr, 'reset complete') + assert len(pr.comments) == 1 + assert pr.comments[0].text.endswith('reset complete') + assert pr.comments[0].text.startswith(pr_utils.STATUS_MARKER) + + +def test_status_comment_updated_even_after_other_comments(settings): + pr = FakePR() + send(settings, pr, 'status 1') + pr.comments.append(FakeComment('bert-e', 'approved')) + send(settings, pr, 'status 2') + assert len(pr.comments) == 2 + assert pr.comments[0].text.endswith('status 2') + + +def test_identical_status_raises(settings): + pr = FakePR() + send(settings, pr, 'same') + with pytest.raises(exceptions.CommentAlreadyExists): + send(settings, pr, 'same') + + +def test_other_authors_comments_are_not_edited(settings): + pr = FakePR() + pr.comments.append( + FakeComment('someone', pr_utils.STATUS_MARKER + '\nfoo')) + send(settings, pr, 'status') + assert len(pr.comments) == 2 + assert pr.comments[0].text.endswith('foo') + + +def test_regular_comments_are_not_updated(settings): + pr = FakePR() + send(settings, pr, 'a', update=False) + send(settings, pr, 'b', update=False) + assert [c.text for c in pr.comments] == ['a', 'b'] + + +def test_fallback_when_host_cannot_edit(settings): + pr = FakePR(can_update=False) + send(settings, pr, 'a') + send(settings, pr, 'b') + assert len(pr.comments) == 2 diff --git a/bert_e/tests/unit/test_github_comment_api.py b/bert_e/tests/unit/test_github_comment_api.py new file mode 100644 index 00000000..e34c2a91 --- /dev/null +++ b/bert_e/tests/unit/test_github_comment_api.py @@ -0,0 +1,28 @@ +from unittest import mock + +from bert_e.git_host.github import Client, Comment + + +def test_client_patch_uses_http_patch(): + client = Client.__new__(Client) + client.session = mock.Mock() + client.session.patch.return_value.text = '{}' + client._patch_url = lambda url: url + client.patch('https://api.github.com/c/1', data='{}') + client.session.patch.assert_called_once() + client.session.post.assert_not_called() + + +def test_comment_edit_sends_patch_and_refreshes_data(): + client = mock.Mock() + client.patch.return_value = { + 'id': 1, 'url': 'https://api.github.com/c/1', 'body': 'new', + 'user': {'login': 'bert-e', 'id': 1}, + 'created_at': '2020-01-01T00:00:00Z'} + comment = Comment(client, _validate=False, id=1, + url='https://api.github.com/c/1', body='old', + user={'login': 'bert-e', 'id': 1}, + created_at='2020-01-01T00:00:00Z') + comment.edit('new') + assert client.patch.call_args[0][0] == 'https://api.github.com/c/1' + assert comment.text == 'new' diff --git a/bert_e/workflow/pr_utils.py b/bert_e/workflow/pr_utils.py index a09f3991..814ea68f 100644 --- a/bert_e/workflow/pr_utils.py +++ b/bert_e/workflow/pr_utils.py @@ -50,8 +50,38 @@ def find_comment(pull_request: AbstractPullRequest, username=None, return comment +STATUS_MARKER = '' + + +def _update_status_comment(settings, pull_request: AbstractPullRequest, + msg: str) -> bool: + """Edit the bot's status comment in place, or create it. + + Returns True if the message was handled (edited or posted), False if + the git host cannot edit comments and a regular comment should be sent. + + Raises: + CommentAlreadyExists: if the status comment already has this text. + + """ + body = f'{STATUS_MARKER}\n{msg}' + previous = find_comment(pull_request, settings.robot, STATUS_MARKER) + if previous is None: + pull_request.add_comment(body) + return True + if previous.text == body: + raise exceptions.CommentAlreadyExists( + "The status comment is already up to date.") + try: + previous.edit(body) + except NotImplementedError: + return False + return True + + def _send_comment(settings, pull_request: AbstractPullRequest, msg: str, - dont_repeat_if_in_history=10) -> None: + dont_repeat_if_in_history=10, + update_status_comment=False) -> None: """Comment a pull request. Before posting: @@ -81,6 +111,9 @@ def _send_comment(settings, pull_request: AbstractPullRequest, msg: str, return LOG.debug('SENDING MESSAGE %s', msg) + if update_status_comment and _update_status_comment( + settings, pull_request, msg): + return pull_request.add_comment(msg) @@ -104,6 +137,7 @@ def notify_user(settings, pull_request: AbstractPullRequest, try: _send_bot_status(settings, pull_request, comment) _send_comment(settings, pull_request, str(comment), - comment.dont_repeat_if_in_history) + comment.dont_repeat_if_in_history, + comment.update_status_comment) except exceptions.CommentAlreadyExists: LOG.info("Comment '%s' already posted", comment.__class__.__name__)