[IMP] runbot_merge: more reliable blocked attribute

Use the proper / actual "is there any stageable PR" query to check if
a PR is blocked as well, that way they shoudn't be diverging all the
time even if it might make PR.blocked a bit more expensive.

fixes #111

runbot_merge/models/pull_requests.py

@@ -258,24 +258,7 @@ class Branch(models.Model):
         for b in self:
             b.active_staging_id = b.staging_ids
 
-    def try_staging(self):
-        """ Tries to create a staging if the current branch does not already
-        have one. Returns None if the branch already has a staging or there
-        is nothing to stage, the newly created staging otherwise.
-        """
-        logger = _logger.getChild('cron')
-
-        logger.info(
-            "Checking %s (%s) for staging: %s, skip? %s",
-            self, self.name,
-            self.active_staging_id,
-            bool(self.active_staging_id)
-        )
-        if self.active_staging_id:
-            return
-
-        PRs = self.env['runbot_merge.pull_requests']
-
+    def _stageable(self):
         # noinspection SqlResolve
         self.env.cr.execute("""
         SELECT
@@ -283,7 +266,7 @@ class Branch(models.Model):
           array_agg(pr.id) AS match
         FROM runbot_merge_pull_requests pr
         LEFT JOIN runbot_merge_batch batch ON pr.batch_id = batch.id AND batch.active
-        WHERE pr.target = %s
+        WHERE pr.target = any(%s)
           -- exclude terminal states (so there's no issue when
           -- deleting branches & reusing labels)
           AND pr.state != 'merged'
@@ -302,9 +285,29 @@ class Branch(models.Model):
                 OR bool_and(pr.state = 'ready')
             )
         ORDER BY min(pr.priority), min(pr.id)
-        """, [self.id])
-        # result: [(priority, [(repo_id, pr_id) for repo in repos]
-        rows = self.env.cr.fetchall()
+        """, [self.ids])
+        # result: [(priority, [pr_id for repo in repos])]
+        return self.env.cr.fetchall()
+
+    def try_staging(self):
+        """ Tries to create a staging if the current branch does not already
+        have one. Returns None if the branch already has a staging or there
+        is nothing to stage, the newly created staging otherwise.
+        """
+        logger = _logger.getChild('cron')
+
+        logger.info(
+            "Checking %s (%s) for staging: %s, skip? %s",
+            self, self.name,
+            self.active_staging_id,
+            bool(self.active_staging_id)
+        )
+        if self.active_staging_id:
+            return
+
+        PRs = self.env['runbot_merge.pull_requests']
+
+        rows = self._stageable()
         priority = rows[0][0] if rows else -1
         if priority == 0:
             # p=0 take precedence over all else
@@ -440,23 +443,21 @@ class PullRequests(models.Model):
                             " PR is linked to an other non-ready PR"
     )
 
-    @property
-    def blocked(self):
-        if self.state not in ('ready', 'merged', 'closed'):
-            return True
-        if not (self.squash or self.merge_method):
-            return True
+    blocked = fields.Boolean(
+        compute='_compute_is_blocked',
+        help="PR is not currently stageable for some reason (mostly an issue if status is ready)"
+    )
 
-        # can't be blocked on a co-dependent PR if it's a patch-*
-        if re.search(r':patch-\d+$', self.label):
-            return False
-
-        return bool(self.search_count([
-            ('id', '!=', self.id),
-            ('label', '=', self.label),
-            ('state', '!=', 'ready'),
-            ('priority', '!=', 0),
-        ]))
+    # missing link to other PRs
+    @api.depends('priority', 'state', 'squash', 'merge_method', 'batch_id.active', 'label')
+    def _compute_is_blocked(self):
+        stageable = {
+            pr_id
+            for _, pr_ids in self.mapped('target')._stageable()
+            for pr_id in pr_ids
+        }
+        for pr in self:
+            pr.blocked = pr.id not in stageable
 
     @api.depends('head')
     def _compute_statuses(self):

runbot_merge/tests/local.py

@@ -9,10 +9,6 @@ import odoo
 
 import fake_github
 
-@pytest.fixture(autouse=True)
-def debuglog(caplog):
-    caplog.set_level(logging.DEBUG, logger='odoo')
-
 @pytest.fixture(scope='session')
 def remote_p():
     return False

runbot_merge/tests/remote.py

@@ -438,6 +438,9 @@ class Model:
 
         return Model(self._env, self._model, {*self._ids, *other._ids}, fields=self._fields)
 
+    def invalidate_cache(self, fnames=None, ids=None):
+        pass # not a concern when every access is an RPC call
+
 class Repo:
     __slots__ = ['name', '_session', '_tokens']
     def __init__(self, session, name, user_tokens):

runbot_merge/tests/test_multirepo.py

@@ -462,3 +462,81 @@ def test_urgent(env, repo_a, repo_b):
     assert p_a.batch_id and p_b.batch_id and p_a.batch_id == p_b.batch_id,\
         "a and b should have been recognised as co-dependent"
     assert not p_c.staging_id
+
+class TestBlocked:
+    def test_merge_method(self, env, repo_a):
+        make_branch(repo_a, 'master', 'initial', {'a0': 'a'})
+
+        pr = make_pr(repo_a, 'A', [{'a1': 'a'}, {'a2': 'a'}])
+
+        run_crons(env)
+
+        p = to_pr(env, pr)
+        assert p.state == 'ready'
+        print(p.id, p.squash, p.merge_method)
+        assert p.blocked
+
+        pr.post_comment('hansen rebase-merge', 'reviewer')
+        assert not p.blocked
+
+    def test_linked_closed(self, env, repo_a, repo_b):
+        make_branch(repo_a, 'master', 'initial', {'a0': 'a'})
+        make_branch(repo_b, 'master', 'initial', {'b0': 'b'})
+
+        pr = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx')
+        b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx', statuses=[])
+        run_crons(env)
+
+        p = to_pr(env, pr)
+        assert p.blocked
+        b.close()
+        # FIXME: find a way for PR.blocked to depend on linked PR somehow so this isn't needed
+        p.invalidate_cache(['blocked'], [p.id])
+        assert not p.blocked
+
+    def test_linked_merged(self, env, repo_a, repo_b):
+        make_branch(repo_a, 'master', 'initial', {'a0': 'a'})
+        make_branch(repo_b, 'master', 'initial', {'b0': 'b'})
+
+        b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx')
+
+        run_crons(env) # stage b and c
+
+        repo_a.post_status('heads/staging.master', 'success', 'legal/cla')
+        repo_a.post_status('heads/staging.master', 'success', 'ci/runbot')
+        repo_b.post_status('heads/staging.master', 'success', 'legal/cla')
+        repo_b.post_status('heads/staging.master', 'success', 'ci/runbot')
+
+        run_crons(env) # merge b and c
+        assert to_pr(env, b).state == 'merged'
+
+        pr = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx')
+        run_crons(env) # merge b and c
+
+        p = to_pr(env, pr)
+        assert not p.blocked
+
+    def test_linked_unready(self, env, repo_a, repo_b):
+        """ Create a PR A linked to a non-ready PR B,
+        * A is blocked by default
+        * A is not blocked if A.p=0
+        * A is not blocked if B.p=0
+        """
+        make_branch(repo_a, 'master', 'initial', {'a0': 'a'})
+        make_branch(repo_b, 'master', 'initial', {'b0': 'b'})
+
+        a = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx')
+        b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx', statuses=[])
+        run_crons(env)
+
+        pr_a = to_pr(env, a)
+        assert pr_a.blocked
+
+        a.post_comment('hansen p=0', 'reviewer')
+        assert not pr_a.blocked
+
+        a.post_comment('hansen p=2', 'reviewer')
+        assert pr_a.blocked
+
+        b.post_comment('hansen p=0', 'reviewer')
+        assert not pr_a.blocked