[FIX] forwardport: detached / closed PR clarifications

- trying to r+ a detached PR *via the forwardbot* should warn, same as
  a non-forwardport PR
- the following sibling of a closed PR should be detached from
  it (probably)
- when a closed forward-port PR is reopened, there should be a
  notification that it is detached and merged via mergebot

Fixes #617

forwardport/changelog/2022-06/closed.md

@@ -0,0 +1 @@
+IMP: notifications when reopening a closed forward-port (e.g. indicate that they're detached)

forwardport/models/project.py

@@ -240,6 +240,7 @@ class PullRequests(models.Model):
         # again 2 PRs with same head is weird so...
         newhead = vals.get('head')
         with_parents = self.filtered('parent_id')
+        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 any children, this is an FP PR being updated, enqueue
@@ -268,6 +269,17 @@ class PullRequests(models.Model):
                         ),
                         'token_field': 'fp_github_token',
                     })
+        for p in closed_fp.filtered(lambda p: p.state != 'closed'):
+            self.env['runbot_merge.pull_requests.feedback'].create({
+                'repository': p.repository.id,
+                'pull_request': p.number,
+                'message': "%sthis PR was closed then reopened. "
+                           "It should be merged the normal way (via @%s)" % (
+                    p.source_id.ping(),
+                    p.repository.project_id.github_prefix,
+                ),
+                'token_field': 'fp_github_token',
+            })
         if vals.get('state') == 'merged':
             for p in self:
                 self.env['forwardport.branch_remover'].create({
@@ -285,6 +297,7 @@ class PullRequests(models.Model):
         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
         return r
 
     def _parse_commands(self, author, comment, login):
@@ -330,6 +343,11 @@ class PullRequests(models.Model):
                     msg = "I can only do this on forward-port PRs and this is not one, see {}.".format(
                         self.repository.project_id.github_prefix
                     )
+                elif not self.parent_id:
+                    ping = True
+                    msg = "I can only do this on unmodified forward-port PRs, ask {}.".format(
+                        self.repository.project_id.github_prefix
+                    )
                 else:
                     merge_bot = self.repository.project_id.github_prefix
                     # don't update the root ever

forwardport/tests/test_simple.py

@@ -798,11 +798,12 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 """)
         ]
 
-    def test_closing_after_fp(self, env, config, make_repo):
+    def test_closing_after_fp(self, env, config, make_repo, users):
         """ Closing a PR which has been forward-ported should not touch the
         followups
         """
         prod, other = make_basic(env, config, make_repo)
+        project = env['runbot_merge.project'].search([])
         with prod:
             [p_1] = prod.make_commits(
                 'a',
@@ -822,23 +823,51 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
         # 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')
         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')
         # should create the second staging
         env.run_crons()
 
-        pr0_1, pr1_1, pr2_1 = env['runbot_merge.pull_requests'].search([], order='number')
+        pr0_id2, pr1_id2, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
-        assert pr0_1 == pr0
+        assert pr0_id2 == pr0_id
-        assert pr1_1 == pr1
+        assert pr1_id2 == pr1_id
+
+        pr1 = prod.get_pr(pr1_id.number)
+        with prod:
+            pr1.close()
+
+        assert pr1_id.state == 'closed'
+        assert not pr1_id.parent_id
+        assert pr2_id.state == 'opened'
+        assert not pr2_id.parent_id, \
+            "the descendant of a closed PR doesn't really make sense, maybe?"
 
         with prod:
-            prod.get_pr(pr1.number).close()
+            pr1.open()
+        assert pr1_id.state == 'validated'
+        env.run_crons()
+        assert pr1.comments[-1] == (
+            users['user'],
+            "@{} @{} this PR was closed then reopened. "
+            "It should be merged the normal way (via @{})".format(
+                users['user'],
+                users['reviewer'],
+                project.github_prefix,
+            )
+        )
 
-        assert pr1_1.state == 'closed'
+        with prod:
-        assert not pr1_1.parent_id
+            pr1.post_comment(f'{project.fp_github_name} r+', config['role_reviewer']['token'])
-        assert pr2_1.state == 'opened'
+        env.run_crons()
+        assert pr1.comments[-1] == (
+            users['user'],
+            "@{} I can only do this on unmodified forward-port PRs, ask {}.".format(
+                users['reviewer'],
+                project.github_prefix,
+            ),
+        )
 
 class TestBranchDeletion:
     def test_delete_normal(self, env, config, make_repo):