[IMP] forwardbot: show FP PRs in reminder message

When posting a reminder that there are open / waiting forward ports on
a source PR, also post *which* PRs those are.

While at it, move the cron code in a proper python file (so we can use
stuff from odoo.tools), and fix display_name so we can straight use
display_name as a github ref' ({owner}/{repo}#{number}). This impacts
log-grepping but it seems like an improvement nonetheless.

Closes odoo/runbot#228

conftest.py

@@ -80,6 +80,13 @@ def pytest_addoption(parser):
 def pytest_report_header(config):
     return 'Running against database ' + config.getoption('--db')
 
+@pytest.fixture(scope='session', autouse=True)
+def _set_socket_timeout():
+    """ Avoid unlimited wait on standard sockets during tests, this is mostly
+    an issue for non-trivial cron calls
+    """
+    socket.setdefaulttimeout(60.0)
+
 @pytest.fixture(scope="session")
 def config(pytestconfig):
     """ Flat version of the pytest config file (pytest.ini), parses to a

forwardport/data/crons.xml

@@ -23,27 +23,9 @@
 
     <record model="ir.cron" id="reminder">
         <field name="name">Remind open PR</field>
-        <field name="model_id" ref="model_forwardport_updates"/>
+        <field name="model_id" ref="model_runbot_merge_pull_requests"/>
         <field name="state">code</field>
-        <field name="code">
-default_delta = dateutil.relativedelta.relativedelta(days=3)
-cutoff = env.context.get('forwardport_updated_before') or (datetime.datetime.now() - default_delta).strftime('%Y-%m-%d %H:%M:%S')
-
-for pr in env['runbot_merge.pull_requests'].search([
-    # only FP PRs
-    ('source_id', '!=', False),
-    # active
-    ('state', 'not in', ['merged', 'closed']),
-    # last updated more than a week ago
-    ('write_date', '&lt;', cutoff),
-]).mapped('source_id'):
-    env['runbot_merge.pull_requests.feedback'].create({
-        'repository': pr.repository.id,
-        'pull_request': pr.number,
-        'message': "This pull request has forward-port PRs awaiting action",
-        'token_field': 'fp_github_token',
-    })
-        </field>
+        <field name="code">model._reminder()</field>
         <field name="interval_number">1</field>
         <field name="interval_type">days</field>
         <field name="numbercall">-1</field>

forwardport/models/project.py

@@ -13,6 +13,7 @@ it up), ...
 """
 import base64
 import contextlib
+import datetime
 import itertools
 import json
 import logging
@@ -22,15 +23,17 @@ import re
 import subprocess
 import tempfile
 
+import dateutil
 import requests
 
 from odoo import _, models, fields, api
 from odoo.exceptions import UserError
-from odoo.tools import topological_sort
+from odoo.tools import topological_sort, groupby
 from odoo.tools.appdirs import user_cache_dir
 from odoo.addons.runbot_merge import utils
 from odoo.addons.runbot_merge.models.pull_requests import RPLUS
 
+DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3)
 
 _logger = logging.getLogger('odoo.addons.forwardport')
 
@@ -228,8 +231,8 @@ class PullRequests(models.Model):
                     ])
                     if self.parent_id:
                         msg = "Sorry, forward-port limit can only be set on an origin PR" \
-                              " (#%d here) before it's merged and forward-ported." % (
-                            self._get_root().number
+                              " (%s here) before it's merged and forward-ported." % (
+                            self._get_root().display_name
                         )
                     elif self.state in ['merged', 'closed']:
                         msg = "Sorry, forward-port limit can only be set before the PR is merged."
@@ -417,16 +420,16 @@ class PullRequests(models.Model):
         target = base._find_next_target(ref)
         if target is None:
             _logger.info(
-                "Will not forward-port %s#%s: no next target",
-                ref.repository.name, ref.number,
+                "Will not forward-port %s: no next target",
+                ref.display_name,
             )
             return  # QUESTION: do the prs need to be updated?
 
         proj = self.mapped('target.project_id')
         if not proj.fp_github_token:
             _logger.warning(
-                "Can not forward-port %s#%s: no token on project %s",
-                ref.repository.name, ref.number,
+                "Can not forward-port %s: no token on project %s",
+                ref.display_name,
                 proj.name
             )
             return
@@ -471,7 +474,7 @@ class PullRequests(models.Model):
                 message = ''
             root = pr._get_root()
             message += '\n'.join(
-                "Forward-Port-Of: %s#%s" % (p.repository.name, p.number)
+                "Forward-Port-Of: %s" % p.display_name
                 for p in root | source
             )
 
@@ -539,7 +542,7 @@ In the former case, you may want to edit this PR message as well.
 """ % (h, source.number, sout, serr)
             elif base._find_next_target(new_pr) is None:
                 ancestors = "".join(
-                    "* %s#%d\n" % (p.repository.name, p.number)
+                    "* %s\n" % p.display_name
                     for p in pr._iter_ancestors()
                     if p.parent_id
                 )
@@ -767,6 +770,26 @@ stderr:
             repo.config('--add', 'remote.origin.fetch', '+refs/pull/*/head:refs/heads/pull/*')
             return repo
 
+    def _reminder(self):
+        cutoff = self.env.context.get('forwardport_updated_before') or fields.Datetime.to_string(datetime.datetime.now() - DEFAULT_DELTA)
+
+        for source, prs in groupby(self.env['runbot_merge.pull_requests'].search([
+            # only FP PRs
+            ('source_id', '!=', False),
+            # active
+            ('state', 'not in', ['merged', 'closed']),
+            # last updated more than <cutoff> ago
+            ('write_date', '<', cutoff),
+        ]), lambda p: p.source_id):
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': source.repository.id,
+                'pull_request': source.number,
+                'message': "This pull request has forward-port PRs awaiting action (not merged or closed): %s" % ', '.join(
+                    pr.display_name for pr in sorted(prs, key=lambda p: p.number)
+                ),
+                'token_field': 'fp_github_token',
+            })
+
 class Stagings(models.Model):
     _inherit = 'runbot_merge.stagings'
 

forwardport/tests/test_limit.py

@@ -255,6 +255,6 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 """),
         (users['reviewer'], bot_name + ' up to b'),
         (bot_name, "Sorry, forward-port limit can only be set on an origin PR"
-                   " (#%d here) before it's merged and forward-ported." % p1.number
+                   " (%s here) before it's merged and forward-ported." % p1.display_name
          ),
     ]

forwardport/tests/test_simple.py

@@ -109,14 +109,15 @@ def test_straightforward_flow(env, config, make_repo, users):
     env.run_crons()
     env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
 
+    pr0_, pr1_, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
+
     assert pr.comments == [
         (users['reviewer'], 'hansen r+ rebase-ff'),
         (users['user'], 'Merge method set to rebase and fast-forward'),
         (users['user'], re_matches(r'Merged at [0-9a-f]{40}, thanks!')),
-        (users['user'], 'This pull request has forward-port PRs awaiting action'),
+        (users['user'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + ', '.join((pr1 | pr2).mapped('display_name'))),
     ]
 
-    pr0_, pr1_, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
     assert pr0_ == pr0
     assert pr1_ == pr1
     assert pr1.parent_id == pr1.source_id == pr0
@@ -136,7 +137,7 @@ def test_straightforward_flow(env, config, make_repo, users):
         (users['user'], """\
 Ping @%s, @%s
 This PR targets c and is the last of the forward-port chain containing:
-* %s#%d
+* %s
 
 To merge the full chain, say
 > @%s r+
@@ -144,7 +145,7 @@ To merge the full chain, say
 More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 """ % (
             users['other'], users['reviewer'],
-            pr1.repository.name, pr1.number,
+            pr1.display_name,
             project.fp_github_name
     )),
     ]
@@ -594,8 +595,8 @@ def test_empty(env, config, make_repo, users):
     assert pr1.comments == [
         (users['reviewer'], 'hansen r+'),
         (users['user'], re_matches(r'Merged at [0-9a-f]{40}, thanks!')),
-        (users['other'], 'This pull request has forward-port PRs awaiting action'),
-        (users['other'], 'This pull request has forward-port PRs awaiting action'),
+        (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"
     # check that this stops if we close the PR
     with prod:
@@ -604,8 +605,8 @@ def test_empty(env, config, make_repo, users):
     assert pr1.comments == [
         (users['reviewer'], 'hansen r+'),
         (users['user'], re_matches(r'Merged at [0-9a-f]{40}, thanks!')),
-        (users['other'], 'This pull request has forward-port PRs awaiting action'),
-        (users['other'], 'This pull request has forward-port PRs awaiting action'),
+        (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),
     ]
 
 def test_partially_empty(env, config, make_repo):

runbot_merge/models/pull_requests.py

@@ -528,7 +528,7 @@ class PullRequests(models.Model):
 
     def name_get(self):
         return {
-            p.id: '%s:%s' % (p.repository.name, p.number)
+            p.id: '%s#%s' % (p.repository.name, p.number)
             for p in self
         }
 
@@ -540,7 +540,7 @@ class PullRequests(models.Model):
         else:
             separator = 's '
         return '<pull_request%s%s>' % (separator, ' '.join(
-            '{0.id} ({0.repository.name}:{0.number})'.format(p)
+            '{0.id} ({0.display_name})'.format(p)
             for p in self
         ))
 
@@ -988,7 +988,7 @@ class PullRequests(models.Model):
                     'pull_request': r.number,
                     'message': "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format(
                         ', '.join(map(
-                            '{0.repository.name}#{0.number}'.format,
+                            '{0.display_name}'.format,
                             unready
                         ))
                     )
@@ -1032,7 +1032,7 @@ class PullRequests(models.Model):
             repository=self.repository.name.replace('/', '\\/')
         )
         if not re.search(pattern, m.body):
-            m.body += '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=self)
+            m.body += '\n\ncloses {pr.display_name}'.format(pr=self)
 
         if self.reviewed_by:
             m.headers.add('signed-off-by', self.reviewed_by.formatted_email)