From 2fafd5419ad7d4924fc6e6194e861f086b134a52 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 1 Oct 2020 17:37:07 +0200
Subject: [PATCH] [FIX] runbot_merge: attempt to stage PRs on disabled branches

Normally opening a PR against a disabled branch is like opening a PR
against a branch which is not configured at all: the PR id ignored
entirely.

However if the PR already exists then the state of the branch isn't
normally checked when interacting with the branch, and it is possible
to trigger its staging, at which point the staging itself will crash:
on a project the branches are `active_test=False` so they're all
visible in the form, but when repos go search()-ing for the branch
they won't find it and will blow up.

Solution: only try staging on branches which are active. Fixes
odoo/runbot#408. Also do the same for checking stagings.

And while at it, fix #409 by wrapping each checking or staging into a
try/except and a savepoint. This way if a staging blows up it should
move on to the next branch instead of getting stuck.
---
 runbot_merge/models/pull_requests.py | 32 +++++++++++++++------
 runbot_merge/tests/test_oddities.py  | 43 ++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py
index da1f23c7..55de5ca3 100644
--- a/runbot_merge/models/pull_requests.py
+++ b/runbot_merge/models/pull_requests.py
@@ -66,18 +66,32 @@ class Project(models.Model):
     )
 
     def _check_stagings(self, commit=False):
-        for staging in self.search([]).mapped('branch_ids.active_staging_id'):
-            staging.check_status()
-            if commit:
-                self.env.cr.commit()
-
-    def _create_stagings(self, commit=False):
-        for branch in self.search([]).mapped('branch_ids'):
-            if not branch.active_staging_id:
-                branch.try_staging()
+        for branch in self.search([]).mapped('branch_ids').filtered('active'):
+            staging = branch.active_staging_id
+            if not staging:
+                continue
+            try:
+                with self.env.cr.savepoint():
+                    staging.check_status()
+            except Exception:
+                _logger.exception("Failed to check staging for branch %r (staging %s)",
+                                  branch.name, staging)
+            else:
                 if commit:
                     self.env.cr.commit()
 
+    def _create_stagings(self, commit=False):
+        for branch in self.search([]).mapped('branch_ids').filtered('active'):
+            if not branch.active_staging_id:
+                try:
+                    with self.env.cr.savepoint():
+                        branch.try_staging()
+                except Exception:
+                    _logger.exception("Failed to create staging for branch %r", branch.name)
+                else:
+                    if commit:
+                        self.env.cr.commit()
+
     def _send_feedback(self):
         Repos = self.env['runbot_merge.repository']
         ghs = {}
diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py
index 9f94225c..4a19065c 100644
--- a/runbot_merge/tests/test_oddities.py
+++ b/runbot_merge/tests/test_oddities.py
@@ -88,3 +88,46 @@ def test_override(env, project, make_repo, users, setreviewers, config):
         'target_url': comments[-1]['html_url'],
         'description': 'Overridden by @{}'.format(users['other']),
     }}
+
+def test_disabled_branch(env, project, make_repo, users, setreviewers, config):
+    """ PRs to disabled branches are ignored, but what if the PR exists *before*
+    the branch is disabled?
+    """
+    repo = make_repo('repo')
+    project.branch_ids.sequence = 0
+    project.write({'branch_ids': [
+        (0, 0, {'name': 'other', 'sequence': 1}),
+        (0, 0, {'name': 'other2', 'sequence': 2}),
+    ]})
+    repo_id = env['runbot_merge.repository'].create({
+        'project_id': project.id,
+        'name': repo.name,
+        'status_ids': [(0, 0, {'context': 'status'})]
+    })
+    setreviewers(*project.repo_ids)
+
+    with repo:
+        [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master')
+        [ot] = repo.make_commits(m, Commit('other', tree={'b': '1'}), ref='heads/other')
+        repo.make_commits(m, Commit('other2', tree={'c': '1'}), ref='heads/other2')
+
+        [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'}))
+        pr = repo.make_pr(title="title", body='body', target='other', head=c)
+        repo.post_status(c, 'success', 'status')
+
+    pr_id = env['runbot_merge.pull_requests'].search([
+        ('repository', '=', repo_id.id),
+        ('number', '=', pr.number),
+    ])
+
+    # disable branch "other"
+    project.branch_ids.filtered(lambda b: b.name == 'other').active = False
+
+    # r+ the PR
+    with repo:
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    # nothing should happen, the PR should be unstaged forever, maybe?
+    assert pr_id.state == 'ready'
+    assert not pr_id.staging_id