fix: resolve issue #293 - Reduce verbosity

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"

bert_e/git_host/base.py

@@ -257,6 +257,13 @@
     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:

bert_e/git_host/github/__init__.py

@@ -1018,6 +1018,10 @@
     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}'

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()

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<!-- bert-e-status -->', '')
+            return md5(text.encode()).digest()
 
         pr = self.create_pr('bugfix/TEST-01334', 'development/4.3',
                             file_='toto.txt')

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}'

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 = '<!-- bert-e-status -->'
+
 
 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__)