fix: resolve issue #297

bert_e/git_host/base.py

@@ -253,6 +253,10 @@ def key(self) -> str:
 class AbstractComment(metaclass=ABCMeta):
     """Abstract class defining the interface of a pull requests's comment."""
 
+    @abstractmethod
+    def edit(self, text: str) -> None:
+        """Replace the text of the comment."""
+
     @abstractmethod
     def delete(self) -> None:
         """Delete the comment."""

bert_e/git_host/bitbucket/__init__.py

@@ -489,6 +489,16 @@
                               pull_request_id=self.data['pullrequest']['id'],
                               comment_id=self.id)
 
+    def edit(self, text):
+        # Bitbucket Cloud only supports PUT to update a comment
+        url = self.GET_URL.format(
+            full_name=self.full_name(),
+            pull_request_id=self.data['pullrequest']['id'],
+            comment_id=self.id)
+        response = self.client.put(
+            url, data=json.dumps({'content': {'raw': text}}))
+        self.data = self.load(response).data
+
     @classmethod
     def create(cls, client, data, **kwargs):
         return super().create(client, {'content': {'raw': data}}, **kwargs)

bert_e/git_host/github/__init__.py

@@ -1015,6 +1015,10 @@
     def id(self) -> int:
         return self.data['id']
 
+    def edit(self, text: str) -> None:
+        self.data = self.client.patch(
+            self.data['url'], data=json.dumps({'body': text}))
+
     def delete(self) -> None:
         self.client.delete(self.data['url'])
 

bert_e/git_host/mock.py

@@ -406,6 +406,11 @@ def comments(self):
 
 class CommentController(Controller, base.AbstractComment):
 
+    def edit(self, text):
+        self.controlled.content = {
+            "raw": text, "markup": "markdown", "html": text}
+        self.controlled.updated_on = datetime.now()
+
     def delete(self):
         self.controlled.delete()
 

bert_e/templates/pr_status_comment.md

@@ -0,0 +1,18 @@
+{{ marker }}
+## Bert-E status: {{ state }}
+{% if status %}
+Result: **{{ status }}**
+{% endif %}
+{% if integration_prs %}
+
+Integration pull requests:
+{% for pr in integration_prs -%}
+* #{{ pr.id }}: `{{ pr.src }}` → `{{ pr.dst }}`
+{% endfor %}
+{% endif %}
+{% if active_options %}
+
+*Options set:* **{{ active_options|join(', ') }}**
+{% endif %}
+
+*This comment is updated at each step; see the comments below for details.*

bert_e/tests/test_bert_e.py

@@ -1387,7 +1387,7 @@ def test_request_integration_branch_creation(self):
         with self.assertRaises(exns.RequestIntegrationBranches):
             self.handle(
                 pr.id, settings=settings, options=options, backtrace=True)
-        self.assertEqual(len(list(pr.get_comments())), 2)
+        self.assertEqual(len(list(pr.get_comments())), 3)
         self.assertIn(
             'Request integration branches', self.get_last_pr_comment(pr))
         self.assertIn(
@@ -1397,7 +1397,7 @@ def test_request_integration_branch_creation(self):
         with self.assertRaises(exns.BuildNotStarted):
             self.handle(
                 pr.id, settings=settings, options=options, backtrace=True)
-        self.assertEqual(len(list(pr.get_comments())), 4)
+        self.assertEqual(len(list(pr.get_comments())), 5)
         self.assertIn('Integration data created', self.get_last_pr_comment(pr))
         self.assertIn(
             'create_integration_branches', self.get_last_pr_comment(pr))
@@ -1444,7 +1444,7 @@ def test_request_integration_branch_by_creating_pull_requests(self):
         with self.assertRaises(exns.BuildNotStarted):
             self.handle(
                 pr.id, settings=settings, options=options, backtrace=True)
-        self.assertEqual(len(list(pr.get_comments())), 4)
+        self.assertEqual(len(list(pr.get_comments())), 5)
         self.assertIn('Integration data created', self.get_last_pr_comment(pr))
 
         options = self.bypass_all
@@ -1493,10 +1493,10 @@ 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.assertEqual(len(list(pr.get_comments())), 4)
 
             self.assertIn(
-                'Integration data created', list(pr.get_comments())[-2].text)
+                'Integration data created', list(pr.get_comments())[-3].text)
 
             self.assertIn(
                 'Waiting for approval', self.get_last_pr_comment(pr))
@@ -3533,7 +3533,7 @@ def test_bypass_author_comment_check(self):
 """ # noqa
         pr = self.create_pr('feature/TEST-0042', 'development/10')
         self.handle(pr.id, settings=settings)
-        self.assertIs(len(list(pr.get_comments())), 2)
+        self.assertIs(len(list(pr.get_comments())), 3)
         self.assertIn('bypass_jira_check', self.get_last_pr_comment(pr))
         settings = """
 repository_owner: {owner}
@@ -3555,7 +3555,7 @@ def test_bypass_author_comment_check(self):
 """ # noqa
         pr = self.create_pr('feature/TEST-0043', 'development/10')
         self.handle(pr.id, settings=settings)
-        self.assertIs(len(list(pr.get_comments())), 2)
+        self.assertIs(len(list(pr.get_comments())), 3)
         self.assertIn('bypass_author_approval', self.get_last_pr_comment(pr))
 
         settings = """
@@ -3578,7 +3578,7 @@ def test_bypass_author_comment_check(self):
 """ # noqa
         pr = self.create_pr('feature/TEST-0044', 'development/10')
         self.handle(pr.id, settings=settings)
-        self.assertIs(len(list(pr.get_comments())), 2)
+        self.assertIs(len(list(pr.get_comments())), 3)
         self.assertIn('bypass_peer_approval', self.get_last_pr_comment(pr))
 
         settings = """
@@ -3601,7 +3601,7 @@ def test_bypass_author_comment_check(self):
 """ # noqa
         pr = self.create_pr('feature/TEST-0045', 'development/10')
         self.handle(pr.id, settings=settings)
-        self.assertIs(len(list(pr.get_comments())), 2)
+        self.assertIs(len(list(pr.get_comments())), 3)
         self.assertIn('bypass_build_status', self.get_last_pr_comment(pr))
 
     def test_bypass_author_jira(self):

bert_e/tests/test_pr_status_comment.py

@@ -0,0 +1,93 @@
+"""Tests of the pinned status comment maintained by notify_user."""
+from types import SimpleNamespace
+
+from bert_e import exceptions
+from bert_e.workflow.pr_utils import (
+    STATUS_COMMENT_MARKER, find_comment, find_status_comment, notify_user,
+    update_status_comment,
+)
+
+
+class FakeComment:
+    def __init__(self, author, text):
+        self.author = author
+        self.text = text
+
+    def edit(self, text):
+        self.text = text
+
+
+class FakePR:
+    def __init__(self):
+        self.comments = []
+
+    def add_comment(self, msg):
+        self.comments.append(FakeComment('bert-e', msg))
+
+    def set_bot_status(self, *args, **kwargs):
+        pass
+
+
+SETTINGS = SimpleNamespace(robot='bert-e', no_comment=False,
+                           interactive=False, send_bot_status=False)
+
+
+def _conflict():
+    src = SimpleNamespace(name='x')
+    dst = SimpleNamespace(name='development/1.1', allow_prefixes=['feature'])
+    return exceptions.IncompatibleSourceBranchPrefix(
+        active_options=[], source=src, destination=dst)
+
+
+def test_status_comment_created_once_and_updated():
+    pr = FakePR()
+    err = _conflict()
+    notify_user(SETTINGS, pr, err)
+    assert len(pr.comments) == 2
+    # the pinned status comment comes first
+    assert pr.comments[0] is find_status_comment(pr, 'bert-e')
+    assert 'IncompatibleSourceBranchPrefix' in pr.comments[0].text
+
+    other = exceptions.MissingJiraId(
+        active_options=[], source_branch='x', dest_branch='b')
+    update_status_comment(SETTINGS, pr, other)
+    assert len(pr.comments) == 2
+    assert 'MissingJiraId' in pr.comments[0].text
+
+
+def test_informational_message_leaves_status_untouched():
+    pr = FakePR()
+    update_status_comment(SETTINGS, pr, _conflict())
+    before = pr.comments[0].text
+    update_status_comment(SETTINGS, pr, exceptions.HelpMessage(
+        options={}, commands={}, active_options=[]))
+    assert pr.comments[0].text == before
+
+
+def test_find_comment_skips_status_comment():
+    pr = FakePR()
+    pr.add_comment('hello')
+    pr.add_comment(STATUS_COMMENT_MARKER + ' status')
+    assert find_comment(pr, 'bert-e', 'hello', -1) is pr.comments[0]
+
+
+def test_integration_prs_kept_between_updates():
+    pr = FakePR()
+    update_status_comment(SETTINGS, pr, _conflict())
+    child = SimpleNamespace(id=7, src_branch='w/1.1/x', name='w/1.1/x',
+                            dst_branch='development/1.1')
+    info = exceptions.IntegrationDataCreated(
+        active_options=[], child_prs=[child, child], wbranches=[child, child],
+        ignored=[], githost='mock', owner='o', slug='r', bert_e='bert-e')
+    update_status_comment(SETTINGS, pr, info)
+    assert '#7' in pr.comments[0].text
+    assert 'IncompatibleSourceBranchPrefix' in pr.comments[0].text
+    update_status_comment(SETTINGS, pr, _conflict())
+    assert '#7' in pr.comments[0].text
+
+
+def test_no_comment_setting_skips():
+    pr = FakePR()
+    settings = SimpleNamespace(**{**vars(SETTINGS), 'no_comment': True})
+    update_status_comment(settings, pr, _conflict())
+    assert pr.comments == []

bert_e/tests/unit/test_status_template.py

@@ -0,0 +1,31 @@
+"""Unit tests for the pinned pull request status comment template."""
+from bert_e.lib.template_loader import render
+from bert_e.workflow.pr_utils import STATUS_COMMENT_MARKER, STATUS_PR_RE
+
+
+def _render(**kwargs):
+    params = dict(marker=STATUS_COMMENT_MARKER, state='Conflict', code=1,
+                  status=None, integration_prs=[], active_options=None)
+    params.update(kwargs)
+    return render('pr_status_comment.md', **params)
+
+
+def test_starts_with_marker_and_shows_state():
+    text = _render()
+    assert text.startswith(STATUS_COMMENT_MARKER)
+    assert 'Conflict' in text
+
+
+def test_lists_integration_prs_parseable():
+    text = _render(integration_prs=[
+        {'id': 12, 'src': 'w/1.1/feature/x', 'dst': 'development/1.1'},
+        {'id': 13, 'src': 'w/2.0/feature/x', 'dst': 'development/2.0'}])
+    found = [m.groupdict() for m in STATUS_PR_RE.finditer(text)]
+    assert [f['id'] for f in found] == ['12', '13']
+    assert found[1]['dst'] == 'development/2.0'
+
+
+def test_result_and_options():
+    text = _render(status='failure', active_options=['after_pull_request'])
+    assert 'failure' in text
+    assert 'after_pull_request' in text

bert_e/workflow/pr_utils.py

@@ -14,13 +14,27 @@
 """Pull Requests messaging utility functions."""
 import itertools
 import logging
+import re
+
+import requests
 
 from bert_e import exceptions
+from bert_e.git_host import base as git_host_base
 from bert_e.git_host.base import AbstractComment, AbstractPullRequest
 from bert_e.lib.cli import confirm
+from bert_e.lib.template_loader import render
 
 LOG = logging.getLogger(__name__)
 
+STATUS_COMMENT_MARKER = '<!-- bert-e-status -->'
+STATUS_STATE_RE = re.compile(r'^## Bert-E status: (?P<state>.*)$',
+                             re.MULTILINE)
+STATUS_RESULT_RE = re.compile(r'^Result: \*\*(?P<status>.*)\*\*$',
+                              re.MULTILINE)
+STATUS_PR_RE = re.compile(
+    r'^\* #(?P<id>\d+): `(?P<src>[^`]+)` \u2192 `(?P<dst>[^`]+)`',
+    re.MULTILINE)
+
 
 def find_comment(pull_request: AbstractPullRequest, username=None,
                  startswith=None, max_history=None) -> AbstractComment:
@@ -43,6 +57,8 @@
     for comment in comments:
         if comment.author != username:
             continue
+        if comment.text.startswith(STATUS_COMMENT_MARKER):
+            continue
         if startswith and not comment.text.startswith(startswith):
             if max_history == -1:
                 return
@@ -98,10 +114,81 @@
     )
 
 
+def find_status_comment(pull_request: AbstractPullRequest, username):
+    """Return the pinned status comment posted by the bot, if any."""
+    for comment in pull_request.comments:
+        if comment.author == username and \
+                comment.text.startswith(STATUS_COMMENT_MARKER):
+            return comment
+    return None
+
+
+def _integration_pull_requests(comment, previous_text=None):
+    """List the integration pull requests known for a status comment.
+
+    Use those carried by the message when available, otherwise keep the
+    ones listed in the previous status comment.
+    """
+    child_prs = comment.kwargs.get('child_prs')
+    if child_prs:
+        return [{'id': pr.id, 'dst': pr.dst_branch, 'src': pr.src_branch}
+                for pr in child_prs]
+    if previous_text:
+        return [
+            {'id': m.group('id'), 'dst': m.group('dst'),
+             'src': m.group('src')}
+            for m in STATUS_PR_RE.finditer(previous_text)
+        ]
+    return []
+
+
+def update_status_comment(settings, pull_request: AbstractPullRequest,
+                          comment: exceptions.TemplateException):
+    """Create or update the single comment showing the latest bot state.
+
+    The comment is edited in place so that the current status is always at
+    the same (early) position in the pull request conversation, and the
+    description of the pull request is left untouched.
+    """
+    if settings.no_comment or settings.interactive:
+        return
+    status = getattr(comment, 'status', None)
+    child_prs = comment.kwargs.get('child_prs')
+    if status is None and not child_prs:
+        # informational messages (greetings, help...) do not change the
+        # state of the pull request
+        return
+    existing = find_status_comment(pull_request, settings.robot)
+    state, code = comment.title, comment.code
+    if status is None:
+        # only the integration pull requests changed: keep the state
+        if existing is None:
+            return
+        m = STATUS_STATE_RE.search(existing.text)
+        state = m.group('state') if m else state
+        m = STATUS_RESULT_RE.search(existing.text)
+        status = m.group('status') if m else None
+        code = None
+    prs = _integration_pull_requests(
+        comment, existing.text if existing else None)
+    text = render('pr_status_comment.md', marker=STATUS_COMMENT_MARKER,
+                  state=state, code=code, status=status,
+                  integration_prs=prs,
+                  active_options=comment.kwargs.get('active_options'))
+    if existing is None:
+        pull_request.add_comment(text)
+    elif existing.text.strip() != text.strip():
+        existing.edit(text)
+
+
 def notify_user(settings, pull_request: AbstractPullRequest,
                 comment: exceptions.TemplateException):
     """Notify user by sending a comment or a build status in a pull request."""
     try:
+        try:
+            update_status_comment(settings, pull_request, comment)
+        except (requests.HTTPError, git_host_base.Error):
+            LOG.warning("Could not update the status comment", exc_info=True)
         _send_bot_status(settings, pull_request, comment)
         _send_comment(settings, pull_request, str(comment),
                       comment.dont_repeat_if_in_history)