[IMP] forwardport: store and display detachment reason

Currently, if a PR forward-port PR gets detached the reason for it is
not always obvious, and may have to be hunted in the logs or in
"sibling" PRs.

By writing a forward port reason (hopefully) ever time we detach a PR,
and displaying that reason in the form and dashboard, the
justification should be a lot more obvious.

Fixes #679

forwardport/__manifest__.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 {
     'name': 'forward port bot',
-    'version': '1.1',
+    'version': '1.2',
     'summary': "A port which forward ports successful PRs.",
     'depends': ['runbot_merge'],
     'data': [

forwardport/data/views.xml

@@ -112,7 +112,7 @@
                     <span t-if="not pr.parent_id"
                           class="badge badge-danger user-select-none"
                           title="A detached PR behaves like a non-forward-port, it has to be approved via the mergebot, this is usually caused by the forward-port having been in conflict or updated.">
-                        DETACHED
+                        DETACHED (<span t-out="pr.detach_reason" style="white-space: pre-wrap;"/>)
                     </span>
                 </dd>
             </t>
@@ -196,10 +196,7 @@
                         <field name="parent_id"/>
                     </group>
                     <group colspan="4" attrs="{'invisible': [('parent_id', '!=', False)]}">
-                        <span>
-                            Detached from forward porting (either conflicting
-                            or explicitly updated).
-                        </span>
+                        <field string="Detached because" name="detach_reason" readonly="1"/>
                     </group>
                     <group>
                         <field string="Forward ported up to" name="limit_id"/>

forwardport/migrations/15.0.1.2/pre-migration.py

@@ -0,0 +1,11 @@
+def migrate(cr, version):
+    """ Add a dummy detach reason to detached PRs.
+    """
+    cr.execute(
+        "ALTER TABLE runbot_merge_pull_requests"
+        " ADD COLUMN detach_reason varchar"
+    )
+    cr.execute(
+        "UPDATE runbot_merge_pull_requests"
+        " SET detach_reason = 'unknown'"
+        " WHERE source_id IS NOT NULL AND parent_id IS NULL")

forwardport/models/project.py

@@ -235,12 +235,20 @@ class PullRequests(models.Model):
     reminder_backoff_factor = fields.Integer(default=-4, group_operator=None)
     merge_date = fields.Datetime()
 
+    detach_reason = fields.Char()
+
     fw_policy = fields.Selection([
         ('ci', "Normal"),
         ('skipci', "Skip CI"),
         # ('skipmerge', "Skip merge"),
     ], required=True, default="ci")
 
+    _sql_constraints = [(
+        'fw_constraint',
+        'check(source_id is null or num_nonnulls(parent_id, detach_reason) = 1)',
+        "fw PRs must either be attached or have a reason for being detached",
+    )]
+
     refname = fields.Char(compute='_compute_refname')
     @api.depends('label')
     def _compute_refname(self):
@@ -280,6 +288,8 @@ class PullRequests(models.Model):
         closed_fp = self.filtered(lambda p: p.state == 'closed' and p.source_id)
         if newhead and not self.env.context.get('ignore_head_update') and newhead != self.head:
             vals.setdefault('parent_id', False)
+            if with_parents and vals['parent_id'] is False:
+                vals['detach_reason'] = f"Head updated from {self.head} to {newhead}"
             # if any children, this is an FP PR being updated, enqueue
             # updating children
             if self.search_count([('parent_id', '=', self.id)]):
@@ -333,8 +343,14 @@ class PullRequests(models.Model):
     def _try_closing(self, by):
         r = super()._try_closing(by)
         if r:
-            self.with_context(forwardport_detach_warn=False).parent_id = False
-            self.search([('parent_id', '=', self.id)]).parent_id = False
+            self.with_context(forwardport_detach_warn=False).write({
+                'parent_id': False,
+                'detach_reason': f"Closed by {by}",
+            })
+            self.search([('parent_id', '=', self.id)]).write({
+                'parent_id': False,
+                'detach_reason': f"{by} closed parent PR {self.display_name}",
+            })
         return r
 
     def _parse_commands(self, author, comment, login):
@@ -728,6 +744,9 @@ class PullRequests(models.Model):
                 'source_id': source.id,
                 # only link to previous PR of sequence if cherrypick passed
                 'parent_id': pr.id if not has_conflicts else False,
+                'detach_reason': "conflicts: {}".format(
+                    f'\n{conflicts[pr]}\n{conflicts[pr]}'.strip()
+                ) if has_conflicts else None,
                 # Copy author & delegates of source as well as delegates of
                 # previous so they can r+ the new forward ports.
                 'delegates': [(6, False, (source.delegates | pr.delegates).ids)]

forwardport/tests/test_simple.py

@@ -547,7 +547,10 @@ def test_delegate_fw(env, config, make_repo, users):
     # ensure pr1 has to be approved to be forward-ported
     _, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
     # detatch from source
-    pr1_id.parent_id = False
+    pr1_id.write({
+        'parent_id': False,
+        'detach_reason': "Detached for testing.",
+    })
     with prod:
         prod.post_status(pr1_id.head, 'success', 'legal/cla')
         prod.post_status(pr1_id.head, 'success', 'ci/runbot')