diff --git a/bert_e/exceptions.py b/bert_e/exceptions.py index f9654dad..0855d64e 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 this message is a transient status that should edit the + # previous status comment instead of posting a new one + updatable = False def __init__(self, **kwargs): self.kwargs = kwargs @@ -93,78 +96,91 @@ class StatusReport(TemplateException): class IncompatibleSourceBranchPrefix(TemplateException): code = 106 + updatable = True template = 'incompatible_source_branch_prefix.md' status = "failure" class MissingJiraId(TemplateException): code = 107 + updatable = True template = 'missing_jira_id.md' status = "failure" class JiraIssueNotFound(TemplateException): code = 108 + updatable = True template = 'jira_issue_not_found.md' status = "failure" class IssueTypeNotSupported(TemplateException): code = 109 + updatable = True template = 'issue_type_not_supported.md' status = "failure" class IncorrectJiraProject(TemplateException): code = 110 + updatable = True template = 'incorrect_jira_project.md' status = "failure" class MismatchPrefixIssueType(TemplateException): code = 111 + updatable = True template = 'mismatch_prefix_issue_type.md' status = "failure" class IncorrectFixVersion(TemplateException): code = 112 + updatable = True template = 'incorrect_fix_version.md' status = "failure" class BranchHistoryMismatch(TemplateException): code = 113 + updatable = True template = 'history_mismatch.md' status = "failure" class Conflict(TemplateException): code = 114 + updatable = True template = 'conflict.md' status = "failure" class ApprovalRequired(TemplateException): code = 115 + updatable = True template = 'need_approval.md' status = "queued" class BuildFailed(TemplateException): code = 118 + updatable = True template = 'build_failed.md' status = "failure" class AfterPullRequest(TemplateException): code = 120 + updatable = True template = 'after_pull_request.md' status = "queued" class IntegrationDataCreated(InformationException): code = 121 + updatable = True template = 'integration_data_created.md' @@ -182,6 +198,7 @@ class NotEnoughCredentials(TemplateException): class QueueConflict(TemplateException): code = 124 + updatable = True template = "queue_conflict.md" status = "failure" @@ -225,11 +242,13 @@ class QueueOutOfOrder(TemplateException): class ResetComplete(TemplateException): code = 128 + updatable = True template = "reset_complete.md" class LossyResetWarning(TemplateException): code = 129 + updatable = True template = "lossy_reset.md" status = "failure" @@ -248,6 +267,7 @@ class IncorrectPullRequestNumber(TemplateException): class SourceBranchTooOld(TemplateException): code = 132 + updatable = True template = "source_branch_too_old.md" status = "failure" @@ -266,6 +286,7 @@ class NotAuthor(TemplateException): class RequestIntegrationBranches(TemplateException): code = 135 + updatable = True template = "request_integration_branches.md" # TODO: review if it should be failure. status = "queued" diff --git a/bert_e/git_host/base.py b/bert_e/git_host/base.py index a38ab926..8e24a855 100644 --- a/bert_e/git_host/base.py +++ b/bert_e/git_host/base.py @@ -257,6 +257,13 @@ class AbstractComment(metaclass=ABCMeta): def delete(self) -> None: """Delete the comment.""" + def update(self, msg: str) -> None: + """Replace the comment's contents with msg. + + Raises NotImplementedError if the git host cannot edit comments. + """ + raise NotImplementedError('Comment edition is not supported') + @property @abstractmethod def author(self) -> str: diff --git a/bert_e/git_host/github/__init__.py b/bert_e/git_host/github/__init__.py index 59515b1c..470549d5 100644 --- a/bert_e/git_host/github/__init__.py +++ b/bert_e/git_host/github/__init__.py @@ -1018,6 +1018,10 @@ def id(self) -> int: def delete(self) -> None: self.client.delete(self.data['url']) + def update(self, msg: str) -> None: + self.data = self.client.patch( + self.data['url'], data=json.dumps({'body': msg})) + 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..83b44817 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 update(self, msg): + self.controlled.content = {"raw": msg, "markup": "markdown", + "html": msg} + @property def author(self): return self['user']['username'].lower() diff --git a/bert_e/tests/test_bert_e.py b/bert_e/tests/test_bert_e.py index 8b6126c5..84f81bc4 100644 --- a/bert_e/tests/test_bert_e.py +++ b/bert_e/tests/test_bert_e.py @@ -1493,10 +1493,8 @@ def test_creation_integration_branch_by_approve(self): with self.assertRaises(exns.ApprovalRequired): self.handle(pr.id, options=options, backtrace=True) - self.assertEqual(len(list(pr.get_comments())), 3) - - self.assertIn( - 'Integration data created', list(pr.get_comments())[-2].text) + # the status comment is updated in place + self.assertEqual(len(list(pr.get_comments())), 2) self.assertIn( 'Waiting for approval', self.get_last_pr_comment(pr)) @@ -2250,7 +2248,9 @@ def get_last_comment(pr): returns the md5 digest of the last comment for easier comparison. """ - return md5(list(pr.get_comments())[-1].text.encode()).digest() + text = list(pr.get_comments())[-1].text + text = text.replace('\n', '') + return md5(text.encode()).digest() pr = self.create_pr('bugfix/TEST-01334', 'development/4.3', file_='toto.txt') 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..b01edefa --- /dev/null +++ b/bert_e/tests/unit/test_comment_update.py @@ -0,0 +1,115 @@ +"""Transient status messages edit the previous status comment.""" +from types import SimpleNamespace + +import pytest + +from bert_e import exceptions +from bert_e.workflow import pr_utils +from bert_e.workflow.pr_utils import STATUS_MARKER, notify_user + + +class FakeComment: + def __init__(self, author, text, editable=True): + self.author = author + self.text = text + self.editable = editable + + def update(self, msg): + if not self.editable: + raise NotImplementedError + self.text = msg + + +class FakePR: + def __init__(self, editable=True): + self.comments = [] + self.editable = editable + + def add_comment(self, msg): + self.comments.append(FakeComment('bert-e', msg, self.editable)) + + def set_bot_status(self, *args, **kwargs): + pass + + +@pytest.fixture +def settings(): + return SimpleNamespace(robot='bert-e', no_comment=False, + interactive=False, send_bot_status=False) + + +class FakeStatus(Exception): + status = None + title = 'title' + dont_repeat_if_in_history = 0 + updatable = True + + def __init__(self, text): + super().__init__(text) + self.text = text + + def __str__(self): + return self.text + + +def test_updatable_flag(): + assert exceptions.BranchHistoryMismatch.updatable + assert exceptions.Conflict.updatable + assert not exceptions.HelpMessage.updatable + assert not exceptions.SuccessMessage.updatable + + +def test_status_comment_is_edited_in_place(settings): + pr = FakePR() + pr_utils._send_comment(settings, pr, 'first', 0, updatable=True) + pr_utils._send_comment(settings, pr, 'second', 0, updatable=True) + assert len(pr.comments) == 1 + assert pr.comments[0].text == f'second\n{STATUS_MARKER}' + + +def test_same_status_is_not_republished(settings): + pr = FakePR() + pr_utils._send_comment(settings, pr, 'same', 0, updatable=True) + with pytest.raises(exceptions.CommentAlreadyExists): + pr_utils._send_comment(settings, pr, 'same', 0, updatable=True) + assert len(pr.comments) == 1 + + +def test_fallback_to_new_comment_when_edit_unsupported(settings): + pr = FakePR(editable=False) + pr_utils._send_comment(settings, pr, 'first', 0, updatable=True) + pr_utils._send_comment(settings, pr, 'second', 0, updatable=True) + assert len(pr.comments) == 2 + + +def test_non_updatable_messages_still_create_comments(settings): + pr = FakePR() + pr_utils._send_comment(settings, pr, 'one', 0) + pr_utils._send_comment(settings, pr, 'two', 0) + assert [c.text for c in pr.comments] == ['one', 'two'] + + +def test_other_authors_comments_are_not_edited(settings): + pr = FakePR() + pr.comments.append(FakeComment('someone', f'x\n{STATUS_MARKER}')) + pr_utils._send_comment(settings, pr, 'new', 0, updatable=True) + assert len(pr.comments) == 2 + assert pr.comments[0].text == f'x\n{STATUS_MARKER}' + + +def test_notify_user_uses_exception_flag(settings): + pr = FakePR() + notify_user(settings, pr, FakeStatus('history conflict')) + notify_user(settings, pr, FakeStatus('reset')) + assert len(pr.comments) == 1 + assert pr.comments[0].text.startswith('reset') + + +def test_status_comment_not_edited_after_user_comment(settings): + """Commands posted after the status must still be seen as new.""" + pr = FakePR() + pr_utils._send_comment(settings, pr, 'first', 0, updatable=True) + pr.comments.append(FakeComment('user', '@bert-e reset')) + pr_utils._send_comment(settings, pr, 'second', 0, updatable=True) + assert len(pr.comments) == 3 + assert pr.comments[0].text == f'first\n{STATUS_MARKER}' diff --git a/bert_e/workflow/pr_utils.py b/bert_e/workflow/pr_utils.py index a09f3991..d8bc199a 100644 --- a/bert_e/workflow/pr_utils.py +++ b/bert_e/workflow/pr_utils.py @@ -21,6 +21,9 @@ LOG = logging.getLogger(__name__) +# Hidden marker identifying the single, continuously edited status comment. +STATUS_MARKER = '' + def find_comment(pull_request: AbstractPullRequest, username=None, startswith=None, max_history=None) -> AbstractComment: @@ -51,7 +54,7 @@ def find_comment(pull_request: AbstractPullRequest, username=None, def _send_comment(settings, pull_request: AbstractPullRequest, msg: str, - dont_repeat_if_in_history=10) -> None: + dont_repeat_if_in_history=10, updatable=False) -> None: """Comment a pull request. Before posting: @@ -80,6 +83,30 @@ def _send_comment(settings, pull_request: AbstractPullRequest, msg: str, if not confirm('Do you want to send this comment?'): return + if updatable: + msg = f'{msg}\n{STATUS_MARKER}' + # Only edit the status comment if it is the last comment of the + # pull request: commands are looked up in the comments posted after + # Bert-E's last one, so editing an older comment would make already + # handled commands look new again. + comments = pull_request.comments + previous = comments[-1] if comments else None + if previous is not None: + is_status = (previous.author == settings.robot and + STATUS_MARKER in previous.text) + if not is_status: + previous = None + if previous is not None: + if previous.text == msg: + raise exceptions.CommentAlreadyExists( + "The status comment is already up to date.") + try: + LOG.debug('UPDATING MESSAGE %s', msg) + previous.update(msg) + return + except NotImplementedError: + pass + LOG.debug('SENDING MESSAGE %s', msg) pull_request.add_comment(msg) @@ -104,6 +131,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, + getattr(comment, 'updatable', False)) except exceptions.CommentAlreadyExists: LOG.info("Comment '%s' already posted", comment.__class__.__name__)