[CHG] runbot_merge, forwardport: remove labelling

Because github materialises every labels change in the
timeline (interspersed with comments), the increasing labels churn
contributes to PRs being difficult to read and review.

This change removes the update of labels on PRs, instead the mergebot
will automatically send a comment to created PRs serving as a
notification that the PR was noticed & providing a link to the
mergebot's dashboard for that PR where users should be able to see the
PR state in detail in case they wonder what's what.

Lots of tests had to be edited to:

- remove any check on the labels of the PR
- add checks on the PR dashboard (to ensure that they're at least on
  the correct "view")
- add a helper to handle the comment now added to every PR by the 'bot
- since that helper is needed by both mergebot and forwardbot, the
  utils modules were unified and moved out of the odoo modules

Probably relevant note: no test was added for the dashboard
ACL, though since I had to explicitly unset the group on the repo used
for tests for things to work it looks to me like it at least excludes
people just fine.

Fixes #419

conftest.py

@@ -55,7 +55,6 @@ import time
 import uuid
 import xmlrpc.client
 from contextlib import closing
-from datetime import datetime
 
 import psutil
 import pytest
@@ -80,6 +79,12 @@ def pytest_addoption(parser):
              "blow through the former); localtunnel has no rate-limiting but "
              "the servers are way less reliable")
 
+
+# noinspection PyUnusedLocal
+def pytest_configure(config):
+    sys.path.insert(0, os.path.join(os.path.dirname(__file__), 'mergebot_test_utils'))
+    print(sys.path)
+
 @pytest.fixture(scope='session', autouse=True)
 def _set_socket_timeout():
     """ Avoid unlimited wait on standard sockets during tests, this is mostly

forwardport/tests/test_batches.py

@@ -1,4 +1,4 @@
-from utils import *
+from utils import Commit, make_basic
 
 
 def test_single_updated(env, config, make_repo):
@@ -86,4 +86,4 @@ def test_single_updated(env, config, make_repo):
     assert pr12_id.parent_id == pr11_id
 
     assert pr22_id.source_id == pr2_id
-    assert pr22_id.parent_id == pr21_id
+    assert pr22_id.parent_id == pr21_id

forwardport/tests/test_limit.py

@@ -3,7 +3,7 @@ import collections
 
 import pytest
 
-from utils import *
+from utils import seen, Commit, make_basic
 
 Description = collections.namedtuple('Restriction', 'source limit')
 def test_configure(env, config, make_repo):
@@ -164,6 +164,7 @@ def test_disable(env, config, make_repo, users, enabled):
         (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),
     }
 
 
@@ -196,8 +197,9 @@ def test_default_disabled(env, config, make_repo, users):
     pr2 = prod.get_pr(p2.number)
 
     cs = pr2.comments
-    assert len(cs) == 1
+    assert len(cs) == 2
     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.
@@ -243,10 +245,12 @@ def test_limit_after_merge(env, config, make_repo, users):
         "check that limit was not updated"
     assert pr1.comments == [
         (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."),
     ]
     assert pr2.comments == [
+        seen(env, pr2, users),
         (users['user'], """\
 This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
 

forwardport/tests/test_simple.py

@@ -1,12 +1,13 @@
 # -*- coding: utf-8 -*-
 import collections
+import re
 import time
 from datetime import datetime, timedelta
 from operator import itemgetter
 
 import pytest
 
-from utils import *
+from utils import seen, re_matches, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all
 
 FMT = '%Y-%m-%d %H:%M:%S'
 FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT)
@@ -124,6 +125,7 @@ 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'))),
     ]
@@ -143,7 +145,9 @@ def test_straightforward_flow(env, config, make_repo, users):
         'h': 'a',
         'x': '1'
     }
-    assert prod.get_pr(pr2.number).comments == [
+    pr2_remote = prod.get_pr(pr2.number)
+    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:
@@ -163,7 +167,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
         prod.post_status(pr2.head, 'success', 'ci/runbot')
         prod.post_status(pr2.head, 'success', 'legal/cla')
 
-        prod.get_pr(pr2.number).post_comment('%s r+' % project.fp_github_name, config['role_reviewer']['token'])
+        pr2_remote.post_comment('%s r+' % project.fp_github_name, config['role_reviewer']['token'])
 
     env.run_crons()
 
@@ -225,7 +229,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     with pytest.raises(AssertionError, match='Not Found'):
         other.get_ref(pr1_ref)
 
-    pr2_ref = prod.get_pr(pr2.number).ref
+    pr2_ref = pr2_remote.ref
     with pytest.raises(AssertionError, match="Not Found"):
         other.get_ref(pr2_ref)
 
@@ -279,7 +283,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
         "forward port should not continue on CI failure"
     pr1_remote = prod.get_pr(pr1_id.number)
-    assert pr1_remote.comments == [fp_intermediate, ci_warning]
+    assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning]
 
     # it was a false positive, rebuild... it fails again!
     with prod:
@@ -288,7 +292,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     # check that FP did not resume & we have a ping on the PR
     assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
         "ensure it still hasn't restarted"
-    assert pr1_remote.comments == [fp_intermediate, ci_warning, ci_warning]
+    assert pr1_remote.comments == [seen(env, pr1_remote, users), fp_intermediate, ci_warning, ci_warning]
 
     # nb: updating the head would detach the PR and not put it in the warning
     # path anymore
@@ -320,6 +324,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     assert pr1_id.head == new_c != pr1_head, "the FP PR should be updated"
     assert not pr1_id.parent_id, "the FP PR should be detached from the original"
     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 should be warned that the PR has become non-FP"
@@ -373,6 +378,7 @@ a
     env.run_crons() # send feedback
 
     assert pr2.comments == [
+        seen(env, pr2, users),
         (users['user'], """Ping @{}, @{}
 This PR targets c and is the last of the forward-port chain containing:
 * {}
@@ -432,45 +438,47 @@ def test_update_merged(env, make_repo, config, users):
         prod.post_status('staging.a', 'success', 'ci/runbot')
     env.run_crons()
 
-    pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
+    pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
     with prod:
-        prod.post_status(pr1.head, 'success', 'legal/cla')
+        prod.post_status(pr1_id.head, 'success', 'legal/cla')
-        prod.post_status(pr1.head, 'success', 'ci/runbot')
+        prod.post_status(pr1_id.head, 'success', 'ci/runbot')
     env.run_crons()
 
-    pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+    pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
+    pr2 = prod.get_pr(pr2_id.number)
     with prod:
-        prod.get_pr(pr2.number).post_comment('hansen r+', config['role_reviewer']['token'])
+        pr2.post_comment('hansen r+', config['role_reviewer']['token'])
-        prod.post_status(pr2.head, 'success', 'legal/cla')
+        prod.post_status(pr2_id.head, 'success', 'legal/cla')
-        prod.post_status(pr2.head, 'success', 'ci/runbot')
+        prod.post_status(pr2_id.head, 'success', 'ci/runbot')
     env.run_crons()
 
-    assert pr2.staging_id
+    assert pr2_id.staging_id
     with prod:
         prod.post_status('staging.c', 'success', 'legal/cla')
         prod.post_status('staging.c', 'success', 'ci/runbot')
     env.run_crons()
-    assert pr2.state == 'merged'
+    assert pr2_id.state == 'merged'
-    assert prod.get_pr(pr2.number).state == 'closed'
+    assert pr2.state == 'closed'
 
     # now we can try updating pr1 and see what happens
-    repo, ref = prod.get_pr(pr1.number).branch
+    repo, ref = prod.get_pr(pr1_id.number).branch
     with repo:
         repo.make_commits(
-            pr1.target.name,
+            pr1_id.target.name,
             Commit('2', tree={'0': '0', '1': '1'}),
             ref='heads/%s' % ref,
             make=False
         )
     updates = env['forwardport.updates'].search([])
     assert updates
-    assert updates.original_root == pr0
+    assert updates.original_root == pr0_id
-    assert updates.new_root == pr1
+    assert updates.new_root == pr1_id
     env.run_crons()
-    assert not pr1.parent_id
+    assert not pr1_id.parent_id
     assert not env['forwardport.updates'].search([])
 
-    assert prod.get_pr(pr2.number).comments == [
+    assert pr2.comments == [
+        seen(env, pr2, users),
         (users['user'], '''This PR targets c and is part of the forward-port chain. Further PRs will be created up to d.
 
 More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
@@ -478,7 +486,7 @@ 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.
 
-You may want or need to manually update any followup PR.""" % pr1.display_name)
+You may want or need to manually update any followup PR.""" % pr1_id.display_name)
     ]
 
 def test_conflict(env, config, make_repo, users):
@@ -541,7 +549,9 @@ xxx
 >>>\x3e>>> [0-9a-f]{7,}.*
 '''),
     }
-    assert prod.get_pr(prb_id.number).comments == [
+    prb = prod.get_pr(prb_id.number)
+    assert prb.comments == [
+        seen(env, prb, users),
         (users['user'], '''\
 This PR targets b and is part of the forward-port chain. Further PRs will be created up to d.
 
@@ -777,6 +787,7 @@ 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),
     ], "each cron run should trigger a new message on the ancestor"
@@ -786,6 +797,7 @@ def test_empty(env, config, make_repo, users):
     env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
     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),
     ]
@@ -1015,17 +1027,15 @@ def test_batched(env, config, make_repo, users):
         ('repository.name', '=', main2.name),
     ])
     # check that relevant users were pinged
-    ping = [
+    ping = (users['user'], """\
-        (users['user'], """\
 This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
 
 More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
-"""),
+""")
-    ]
     pr_remote_1b = main1.get_pr(pr1b.number)
     pr_remote_2b = main2.get_pr(pr2b.number)
-    assert pr_remote_1b.comments == ping
+    assert pr_remote_1b.comments == [seen(env, pr_remote_1b, users), ping]
-    assert pr_remote_2b.comments == ping
+    assert pr_remote_2b.comments == [seen(env, pr_remote_2b, users), ping]
 
     with main1, main2:
         validate_all([main1], [pr1b.head])
@@ -1097,21 +1107,23 @@ class TestClosing:
         # should merge the staging then create the FP PR
         env.run_crons()
 
-        pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
+        pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
         # close the FP PR then have CI validate it
+        pr1 = prod.get_pr(pr1_id.number)
         with prod:
-            prod.get_pr(pr1.number).close()
+            pr1.close()
-        assert pr1.state == 'closed'
+        assert pr1_id.state == 'closed'
-        assert not pr1.parent_id, "closed PR should should be detached from its parent"
+        assert not pr1_id.parent_id, "closed PR should should be detached from its parent"
         with prod:
-            prod.post_status(pr1.head, 'success', 'legal/cla')
+            prod.post_status(pr1_id.head, 'success', 'legal/cla')
-            prod.post_status(pr1.head, 'success', 'ci/runbot')
+            prod.post_status(pr1_id.head, 'success', 'ci/runbot')
         env.run_crons()
         env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron')
 
-        assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\
+        assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\
             "closing the PR should suppress the FP sequence"
-        assert prod.get_pr(pr1.number).comments == [
+        assert pr1.comments == [
+            seen(env, pr1, users),
             (users['user'], """\
 This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
 

forwardport/tests/test_weird.py

@@ -1,10 +1,8 @@
 # -*- coding: utf-8 -*-
-import sys
-
 import pytest
-import re
 
-from utils import *
+from utils import seen, Commit
+
 
 def make_basic(env, config, make_repo, *, fp_token, fp_remote):
     """ Creates a basic repo with 3 forking branches
@@ -401,6 +399,7 @@ class TestNotAllBranches:
         # different next target
         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)
@@ -408,6 +407,7 @@ class TestNotAllBranches:
         ]
         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)

mergebot_test_utils/utils.py

@@ -25,6 +25,20 @@ def validate_all(repos, refs, contexts=('ci/runbot', 'legal/cla')):
     for repo, branch, context in itertools.product(repos, refs, contexts):
         repo.post_status(branch, 'success', context)
 
+def get_partner(env, gh_login):
+    return env['res.partner'].search([('github_login', '=', gh_login)])
+
+def _simple_init(repo):
+    """ Creates a very simple initialisation: a master branch with a commit,
+    and a PR by 'user' with two commits, targeted to the master branch
+    """
+    m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
+    repo.make_ref('heads/master', m)
+    c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
+    c2 = repo.make_commit(c1, 'second', None, tree={'m': 'c2'})
+    prx = repo.make_pr(title='title', body='body', target='master', head=c2)
+    return prx
+
 class re_matches:
     def __init__(self, pattern, flags=0):
         self._r = re.compile(pattern, flags)
@@ -35,6 +49,12 @@ class re_matches:
     def __repr__(self):
         return '~' + self._r.pattern + '~'
 
+def seen(env, pr, users):
+    pr_id = env['runbot_merge.pull_requests'].search([
+        ('repository.name', '=', pr.repo.name),
+        ('number', '=', pr.number)
+    ])
+    return users['user'], f'[Pull request status dashboard]({pr_id.url}).'
 
 def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproject'):
     """ Creates a basic repo with 3 forking branches

runbot_merge/controllers/dashboard.py

@@ -38,8 +38,15 @@ class MergebotDashboard(Controller):
         if not pr_id.repository.group_id <= request.env.user.groups_id:
             raise werkzeug.exceptions.NotFound()
 
+        st = {}
+        if pr_id.statuses:
+            # normalise `statuses` to map to a dict
+            st = {
+                k: {'state': v} if isinstance(v, str) else v
+                for k, v in ast.literal_eval(pr_id.statuses).items()
+            }
         return request.render('runbot_merge.view_pull_request', {
             'pr': pr_id,
             'merged_head': json.loads(pr_id.commits_map).get(''),
-            'statuses': ast.literal_eval(pr_id.statuses) if pr_id.statuses else {}
+            'statuses': st
         })

runbot_merge/models/pull_requests.py

@@ -16,6 +16,7 @@ import time
 from itertools import takewhile
 
 import requests
+import werkzeug
 from werkzeug.datastructures import Headers
 
 from odoo import api, fields, models, tools
@@ -607,18 +608,6 @@ For-Commit-Id: %s
                 )
                 raise TimeoutError("Staged head not updated after %d seconds" % sum(WAIT_FOR_VISIBILITY))
 
-
-        # creating the staging doesn't trigger a write on the prs
-        # and thus the ->staging taggings, so do that by hand
-        Tagging = self.env['runbot_merge.pull_requests.tagging']
-        for pr in st.mapped('batch_ids.prs'):
-            Tagging.create({
-                'pull_request': pr.number,
-                'repository': pr.repository.id,
-                'state_from': 'ready',
-                'state_to': 'staged',
-            })
-
         logger.info("Created staging %s (%s) to %s", st, ', '.join(
             '%s[%s]' % (batch, batch.prs)
             for batch in staged
@@ -703,6 +692,18 @@ class PullRequests(models.Model):
         help="PR is not currently stageable for some reason (mostly an issue if status is ready)"
     )
 
+    url = fields.Char(compute='_compute_url')
+    github_url = fields.Char(compute='_compute_url')
+
+    @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'))
+        gh_base = werkzeug.urls.url_parse('https://github.com')
+        for pr in self:
+            path = f'/{werkzeug.urls.url_quote(pr.repository.name)}/pull/{pr.number}'
+            pr.url = str(base.join(path))
+            pr.github_url = str(gh_base.join(path))
+
     @api.depends('repository.name', 'number')
     def _compute_display_name(self):
         return super(PullRequests, self)._compute_display_name()
@@ -1100,12 +1101,6 @@ class PullRequests(models.Model):
                     pr.state = 'validated'
                 elif oldstate == 'approved':
                     pr.state = 'ready'
-            self.env['runbot_merge.pull_requests.tagging'].create({
-                'repository': pr.repository.id,
-                'pull_request': pr.number,
-                'tags_remove': json.dumps(r),
-                'tags_add': json.dumps(a),
-            })
         return failed
 
     def _notify_ci_new_failure(self, ci, st):
@@ -1158,11 +1153,10 @@ class PullRequests(models.Model):
         pr._validate(json.loads(c.statuses or '{}'))
 
         if pr.state not in ('closed', 'merged'):
-            self.env['runbot_merge.pull_requests.tagging'].create({
+            self.env['runbot_merge.pull_requests.feedback'].create({
-                'pull_request': pr.number,
                 'repository': pr.repository.id,
-                'state_from': False,
+                'pull_request': pr.number,
-                'state_to': pr._tagstate,
+                'message': f"[Pull request status dashboard]({pr.url}).",
             })
         return pr
 
@@ -1208,28 +1202,8 @@ class PullRequests(models.Model):
         if newhead:
             c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)])
             self._validate(json.loads(c.statuses or '{}'))
-
-        for pr in self:
-            before, after = oldstate[pr], pr._tagstate
-            if after != before:
-                self.env['runbot_merge.pull_requests.tagging'].create({
-                    'pull_request': pr.number,
-                    'repository': pr.repository.id,
-                    'state_from': oldstate[pr],
-                    'state_to': pr._tagstate,
-                })
         return w
 
-    def unlink(self):
-        for pr in self:
-            self.env['runbot_merge.pull_requests.tagging'].create({
-                'pull_request': pr.number,
-                'repository': pr.repository.id,
-                'state_from': pr._tagstate,
-                'state_to': False,
-            })
-        return super().unlink()
-
     def _check_linked_prs_statuses(self, commit=False):
         """ Looks for linked PRs where at least one of the PRs is in a ready
         state and the others are not, notifies the other PRs.
@@ -1446,12 +1420,6 @@ class PullRequests(models.Model):
         self.env.cr.commit()
         self.modified(['state'])
         if self.env.cr.rowcount:
-            self.env['runbot_merge.pull_requests.tagging'].create({
-                'pull_request': self.number,
-                'repository': self.repository.id,
-                'state_from': res[1] if not self.staging_id else 'staged',
-                'state_to': 'closed',
-            })
             self.unstage(
                 "PR %s closed by %s",
                 self.display_name,

runbot_merge/tests/test_basic.py

@@ -2,23 +2,24 @@ import datetime
 import itertools
 import json
 import re
-import time
 from unittest import mock
 
 import pytest
 from lxml import html
 
 import odoo
+from utils import _simple_init, seen, re_matches, get_partner, Commit
 
-from test_utils import re_matches, get_partner, _simple_init
-from utils import Commit
 
+def pr_page(page, pr):
+    return html.fromstring(page(f'/{pr.repo.name}/pull/{pr.number}'))
 
 @pytest.fixture
 def repo(env, project, make_repo, users, setreviewers):
     r = make_repo('repo')
     project.write({'repo_ids': [(0, 0, {
         'name': r.name,
+        'group_id': False,
         'required_statuses': 'legal/cla,ci/runbot'
     })]})
     setreviewers(*project.repo_ids)
@@ -33,16 +34,18 @@ def test_trivial_flow(env, repo, page, users, config):
         # create PR with 2 commits
         c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'})
         c1 = repo.make_commit(c0, 'add file', None, tree={'a': 'some other content', 'b': 'a second file'})
-        pr1 = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,)
+        pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,)
 
-    [pr] = env['runbot_merge.pull_requests'].search([
+    [pr_id] = env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
-        ('number', '=', pr1.number),
+        ('number', '=', pr.number),
     ])
-    assert pr.state == 'opened'
+    assert pr_id.state == 'opened'
     env.run_crons()
-    assert pr1.labels == {'seen 🙂', '☐ legal/cla', '☐ ci/runbot'}
+    assert pr.comments == [seen(env, pr, users)]
-    # nothing happened
+    s = pr_page(page, pr).cssselect('.alert-info > ul > li')
+    assert [it.get('class') for it in s] == ['fail', 'fail', ''],\
+        "merge method unset, review missing, no CI"
 
     with repo:
         repo.post_status(c1, 'success', 'legal/cla')
@@ -54,19 +57,27 @@ def test_trivial_flow(env, repo, page, users, config):
         repo.post_status(c1, 'success', 'ci/runbot')
 
     env.run_crons()
-    assert pr.state == 'validated'
+    assert pr_id.state == 'validated'
+
+    s = pr_page(page, pr).cssselect('.alert-info > ul > li')
+    assert [it.get('class') for it in s] == ['fail', 'fail', 'ok'],\
+        "merge method unset, review missing, CI"
+    statuses = [
+        (l.find('a').text.split(':')[0], l.get('class').strip())
+        for l in s[2].cssselect('ul li')
+    ]
+    assert statuses == [('legal/cla', 'ok'), ('ci/runbot', 'ok')]
 
-    assert pr1.labels == {'seen 🙂', 'CI 🤖', '☑ ci/runbot', '☑ legal/cla'}
 
     with repo:
-        pr1.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
+        pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
-    assert pr.state == 'ready'
+    assert pr_id.state == 'ready'
 
     # can't check labels here as running the cron will stage it
 
     env.run_crons()
-    assert pr.staging_id
+    assert pr_id.staging_id
-    assert pr1.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ ci/runbot', '☑ legal/cla'}
+    assert pr_page(page, pr).cssselect('.alert-primary')
 
     with repo:
         # get head of staging branch
@@ -77,7 +88,7 @@ def test_trivial_flow(env, repo, page, users, config):
         repo.post_status(staging_head.id, 'failure', 'ci/lint', target_url='http://ignored.com/whocares')
     # need to store this because after the crons have run the staging will
     # have succeeded and been disabled
-    st = pr.staging_id
+    st = pr_id.staging_id
     env.run_crons()
 
     assert set(tuple(t) for t in st.statuses) == {
@@ -95,8 +106,8 @@ def test_trivial_flow(env, repo, page, users, config):
     assert re.match('^force rebuild', staging_head.message)
 
     assert st.state == 'success'
-    assert pr.state == 'merged'
+    assert pr_id.state == 'merged'
-    assert pr1.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merged 🎉', '☑ ci/runbot', '☑ legal/cla'}
+    assert pr_page(page, pr).cssselect('.alert-success')
 
     master = repo.commit('heads/master')
     # with default-rebase, only one parent is "known"
@@ -358,7 +369,6 @@ def test_staging_ongoing(env, repo, config):
         ('number', '=', pr2.number)
     ])
     assert p_2.state == 'ready', "PR2 should not have been staged since there is a pending staging for master"
-    assert pr2.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', '☑ ci/runbot', '☑ legal/cla'}
 
     staging_head = repo.commit('heads/staging.master')
     with repo:
@@ -413,7 +423,7 @@ def test_staging_concurrent(env, repo, config):
     ])
     assert pr2.staging_id
 
-def test_staging_confict_first(env, repo, users, config):
+def test_staging_confict_first(env, repo, users, config, page):
     """ If the first batch of a staging triggers a conflict, the PR should be
     marked as in error
     """
@@ -435,9 +445,10 @@ def test_staging_confict_first(env, repo, users, config):
         ('number', '=', prx.number)
     ])
     assert pr1.state == 'error'
-    assert prx.labels == {'seen 🙂', 'error 🙅', '☑ legal/cla', '☑ ci/runbot'}
+    assert pr_page(page, prx).cssselect('.alert-danger')
     assert prx.comments == [
         (users['reviewer'], 'hansen r+ rebase-merge'),
+        seen(env, prx, 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')),
     ]
@@ -566,6 +577,7 @@ def test_staging_ci_failure_single(env, repo, users, config):
 
     assert prx.comments == [
         (users['reviewer'], 'hansen r+ rebase-merge'),
+        seen(env, prx, users),
         (users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"),
         (users['user'], 'Staging failed: ci/runbot')
     ]
@@ -710,7 +722,6 @@ class TestPREdition:
         with repo: prx.base = '2.0'
         assert not pr.exists()
         env.run_crons()
-        assert prx.labels == {'☐ legal/cla', '☐ ci/runbot'}
 
         with repo: prx.base = '1.0'
         assert env['runbot_merge.pull_requests'].search([
@@ -794,7 +805,7 @@ def test_edit_staged(env, repo):
     """
     What should happen when editing the PR/metadata (not pushing) of a staged PR
     """
-def test_close_staged(env, repo, config):
+def test_close_staged(env, repo, config, page):
     """
     When closing a staged PR, cancel the staging
     """
@@ -822,7 +833,7 @@ def test_close_staged(env, repo, config):
     assert not pr.staging_id
     assert not env['runbot_merge.stagings'].search([])
     assert pr.state == 'closed'
-    assert prx.labels == {'seen 🙂', 'closed 💔', '☑ ci/runbot', '☑ legal/cla'}
+    assert pr_page(page, prx).cssselect('.alert-light')
 
 def test_forward_port(env, repo, config):
     with repo:
@@ -910,10 +921,12 @@ def test_rebase_failure(env, repo, users, config):
 
     assert pr_a.comments == [
         (users['reviewer'], 'hansen r+'),
+        seen(env, pr_a, users),
         (users['user'], re_matches(r'^Unable to stage PR')),
     ]
     assert pr_b.comments == [
         (users['reviewer'], 'hansen r+'),
+        seen(env, pr_b, users),
     ]
     assert repo.read_tree(repo.commit('heads/staging.master')) == {
         'm': 'm',
@@ -939,6 +952,7 @@ def test_ci_failure_after_review(env, repo, users, config):
 
     assert prx.comments == [
         (users['reviewer'], 'hansen r+'),
+        seen(env, prx, users),
         (users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
         (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)),
     ]
@@ -983,6 +997,7 @@ def test_reopen_merged_pr(env, repo, config, users):
     assert pr.state == 'merged'
     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'])
     ]
 
@@ -1142,6 +1157,7 @@ class TestRetry:
         assert prx.comments == [
             (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'])),
         ]
 
@@ -1315,6 +1331,7 @@ 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:
 
 * `merge` to merge directly, using the PR as merge commit message
@@ -1359,6 +1376,7 @@ 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['reviewer'], 'hansen merge'),
             (users['user'], "Merge method set to merge directly, using the PR as merge commit message"),
@@ -2551,6 +2569,7 @@ class TestReviewing(object):
         env.run_crons()
         assert prx.comments == [
             (users['other'], 'hansen r+'),
+            seen(env, prx, users),
             (users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])),
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
@@ -2582,6 +2601,7 @@ class TestReviewing(object):
         env.run_crons()
         assert prx.comments == [
             (users['reviewer'], 'hansen r+'),
+            seen(env, prx, users),
             (users['user'], "I'm sorry, @{}. You can't review+.".format(users['reviewer'])),
         ]
 
@@ -2768,8 +2788,10 @@ class TestUnknownPR:
             'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None}
         }
         assert prx.comments == [
+            seen(env, prx, users),
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
+            seen(env, prx, users),
             (users['user'], "Sorry, I didn't know about this PR and had to "
                             "retrieve its information, you may have to "
                             "re-approve it."),
@@ -2892,6 +2914,7 @@ class TestRecognizeCommands:
         assert pr.comments == [
             (users['reviewer'], "hansen do the thing"),
             (users['reviewer'], "hansen @bobby-b r+ :+1:"),
+            seen(env, pr, users),
         ]
 
 class TestRMinus:
@@ -3102,6 +3125,7 @@ class TestRMinus:
         assert pr.priority == 1, "priority should have been downgraded"
         assert prx.comments == [
             (users['reviewer'], 'hansen p=0'),
+            seen(env, prx, users),
             (users['reviewer'], 'hansen r-'),
             (users['user'], "PR priority reset to 1, as pull requests with priority 0 ignore review state."),
         ]
@@ -3201,6 +3225,7 @@ class TestFeedback:
 
         assert prx.comments == [
             (users['reviewer'], 'hansen r+'),
+            seen(env, prx, users),
             (users['user'], "'ci/runbot' failed on this reviewed PR.")
         ]
 
@@ -3229,6 +3254,7 @@ class TestFeedback:
         env.run_crons()
 
         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'])
         ]
@@ -3287,58 +3313,3 @@ class TestEmailFormatting:
             'github_login': 'Osmose99',
         })
         assert p1.formatted_email == 'Shultz <Osmose99@users.noreply.github.com>'
-
-class TestLabelling:
-    def test_desync(self, env, repo, config):
-        with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'})
-            pr = repo.make_pr(title='gibberish', body='blahblah', target='master', head=c)
-        env.run_crons('runbot_merge.feedback_cron')
-        assert pr.labels == {'seen 🙂', '☐ legal/cla', '☐ ci/runbot'},\
-            "required statuses should be labelled as pending"
-
-        with repo:
-            repo.post_status(c, 'success', 'legal/cla')
-            repo.post_status(c, 'success', 'ci/runbot')
-
-        env.run_crons()
-
-        assert pr.labels == {'seen 🙂', 'CI 🤖', '☑ legal/cla', '☑ ci/runbot'}
-        with repo:
-            # desync state and labels
-            pr.labels.remove('CI 🤖')
-
-            pr.post_comment('hansen r+', config['role_reviewer']['token'])
-        env.run_crons()
-
-        assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ legal/cla', '☑ ci/runbot'},\
-            "labels should be resynchronised"
-
-    def test_other_tags(self, env, repo, config):
-        with repo:
-            m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'})
-            repo.make_ref('heads/master', m)
-
-            c = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'})
-            pr = repo.make_pr(title='gibberish', body='blahblah', target='master', head=c)
-
-        with repo:
-            # "foreign" labels
-            pr.labels.update(('L1', 'L2'))
-
-        with repo:
-            repo.post_status(c, 'success', 'legal/cla')
-            repo.post_status(c, 'success', 'ci/runbot')
-        env.run_crons()
-
-        assert pr.labels == {'seen 🙂', 'CI 🤖', '☑ legal/cla', '☑ ci/runbot', 'L1', 'L2'}, "should not lose foreign labels"
-
-        with repo:
-            pr.post_comment('hansen r+', config['role_reviewer']['token'])
-        env.run_crons()
-
-        assert pr.labels == {'seen 🙂', 'CI 🤖', 'r+ 👌', 'merging 👷', '☑ legal/cla', '☑ ci/runbot', 'L1', 'L2'},\
-            "should not lose foreign labels"

runbot_merge/tests/test_disabled_branch.py

@@ -1,6 +1,4 @@
-import pytest
+from utils import seen, Commit
-
-from utils import Commit
 
 def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users):
     """ PRs to disabled branches are ignored, but what if the PR exists *before*
@@ -59,6 +57,7 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf
     env.run_crons()
     assert pr.comments == [
         (users['reviewer'], "hansen r+"),
+        seen(env, pr, users),
         (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name),
     ], "reopening a PR to an inactive branch should send feedback, but not closing it"
 
@@ -133,6 +132,7 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users):
     assert pr_id.state == 'opened'
     assert pr.comments == [
         (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name),
+        seen(env, pr, users),
     ]
 
 def test_retarget_from_disabled(env, make_repo, project, setreviewers):

runbot_merge/tests/test_multirepo.py

@@ -11,7 +11,8 @@ import time
 import pytest
 import requests
 
-from test_utils import re_matches, get_partner
+from utils import seen, re_matches, get_partner
+
 
 @pytest.fixture
 def repo_a(project, make_repo, setreviewers):
@@ -395,6 +396,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config):
     assert failed.state == 'error'
     assert pr1b.comments == [
         (users['reviewer'], 'hansen r+'),
+        seen(env, pr1b, users),
         (users['user'], re_matches('^Unable to stage PR')),
     ]
     other = to_pr(env, pr1a)
@@ -493,15 +495,17 @@ 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)),
         ]
         # 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)),
         ]
-        assert p_b.comments == []
+        assert p_b.comments == [seen(env, p_b, users)]
 
     def test_two_of_three_unready(self, env, project, repo_a, repo_b, repo_c, users, config):
         """ In a 3-batch, if two of the PRs are not ready both should be
@@ -531,15 +535,16 @@ class TestCompanionsNotReady:
             )
         env.run_crons()
 
-        assert pr_a.comments == []
+        assert pr_a.comments == [seen(env, pr_a, users)]
         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." % (
                 repo_a.name, pr_a.number,
                 repo_c.name, pr_c.number
             ))
         ]
-        assert pr_c.comments == []
+        assert pr_c.comments == [seen(env, pr_c, users)]
 
     def test_one_of_three_unready(self, env, project, repo_a, repo_b, repo_c, users, config):
         """ In a 3-batch, if one PR is not ready it should be linked on the
@@ -569,15 +574,17 @@ class TestCompanionsNotReady:
             )
         env.run_crons()
 
-        assert pr_a.comments == []
+        assert pr_a.comments == [seen(env, pr_a, users)]
         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." % (
                 repo_a.name, pr_a.number
             ))
         ]
         assert pr_c.comments == [
             (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." % (
                  repo_a.name, pr_a.number
@@ -616,6 +623,7 @@ def test_other_failed(env, project, repo_a, repo_b, users, config):
     assert pr.state == 'error'
     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)
     ]
 

runbot_merge/tests/test_oddities.py

@@ -1,8 +1,3 @@
-import json
-
-from utils import Commit
-
-
 def test_partner_merge(env):
     p_src = env['res.partner'].create({
         'name': 'kfhsf',

runbot_merge/tests/test_status_overrides.py

@@ -2,7 +2,7 @@ import json
 
 import pytest
 
-from utils import Commit
+from utils import seen, Commit
 
 
 def test_no_duplicates(env):
@@ -87,6 +87,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config):
     comments = pr.comments
     assert comments == [
         (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['other'], "hansen override=l/int"),
@@ -137,6 +138,7 @@ def test_no_repository(env, project, make_repo, users, setreviewers, config):
     comments = pr.comments
     assert comments == [
         (users['reviewer'], 'hansen r+'),
+        seen(env, pr, users),
         (users['other'], "hansen override=l/int"),
     ]
     assert pr_id.statuses == '{}'

runbot_merge/tests/test_utils.py

@@ -1,27 +0,0 @@
-# -*- coding: utf-8 -*-
-import re
-
-
-class re_matches:
-    def __init__(self, pattern, flags=0):
-        self._r = re.compile(pattern, flags)
-
-    def __eq__(self, text):
-        return self._r.match(text)
-
-    def __repr__(self):
-        return '~' + self._r.pattern + '~'
-
-def get_partner(env, gh_login):
-    return env['res.partner'].search([('github_login', '=', gh_login)])
-
-def _simple_init(repo):
-    """ Creates a very simple initialisation: a master branch with a commit,
-    and a PR by 'user' with two commits, targeted to the master branch
-    """
-    m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
-    repo.make_ref('heads/master', m)
-    c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'})
-    c2 = repo.make_commit(c1, 'second', None, tree={'m': 'c2'})
-    prx = repo.make_pr(title='title', body='body', target='master', head=c2)
-    return prx

runbot_merge/views/templates.xml

@@ -338,7 +338,6 @@
             </li>
             <li t-att-class="'ok' if pr.state not in ('opened', 'approved') else ''">
                 CI
-                <t t-set="statuses" t-value="statuses"/>
                 <ul class="todo">
                     <t t-foreach="pr.repository.status_ids._for_pr(pr)" t-as="ci">
                         <t t-set="st" t-value="statuses.get(ci.context.strip())"/>
@@ -359,7 +358,7 @@
                 <ul class="todo">
                     <t t-foreach="linked_prs" t-as="linked">
                         <li t-att-class="'ok' if linked._ready else 'fail'">
-                            <a t-attf-href="/{{linked.repository.name}}/pull/{{linked.number}}" t-field="linked.display_name"/>
+                            <a t-attf-href="linked.url" t-field="linked.display_name"/>
                         </li>
                     </t>
                 </ul>
@@ -371,8 +370,7 @@
     <template id="view_pull_request">
         <t t-call="website.layout">
             <div id="wrap"><div class="container-fluid">
-                <t t-set="pr_url" t-valuef="https://github.com/{{pr.repository.name}}/pull/{{pr.number}}"/>
+                <a t-att-href="pr.url">
-                <a t-att-href="pr_url">
                     <h1 t-field="pr.display_name"/>
                 </a>
                 <h6>Created by <span t-field="pr.author.display_name"/></h6>
@@ -386,7 +384,7 @@
                     <dt>label</dt>
                     <dd><span t-field="pr.label"/></dd>
                     <dt>head</dt>
-                    <dd><a t-attf-href="{{pr_url}}/commits/{{pr.head}}"><span t-field="pr.head"/></a></dd>
+                    <dd><a t-attf-href="{{pr.github_url}}/commits/{{pr.head}}"><span t-field="pr.head"/></a></dd>
                 </dl>
                 <p t-field="pr.message"/>
             </div></div>