From e6a743bdc2ff329a79280bb678db72262fe0009f Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 15 Jul 2024 13:49:39 +0200
Subject: [PATCH] [FIX] runbot_merge: the batch target should prioritise open
 PRs

From the previous version of `_compute_target` this was clearly
intended otherwise the fallback makes no sense, but just as clearly I
completely missed / forgot about it halfway through (and the lack of
test didn't help).

The compute is also way overcomplicated, it's not clear why (the only
explanation I can think of is that an intermediate version had a
string target but if that ever happened it was squashed away).
---
 runbot_merge/models/batch.py                 | 14 ++----
 runbot_merge/tests/test_batch_consistency.py | 52 ++++++++++++++++++--
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py
index 1d0ec945..2c6e85c2 100644
--- a/runbot_merge/models/batch.py
+++ b/runbot_merge/models/batch.py
@@ -187,19 +187,11 @@ class Batch(models.Model):
     def _search_name(self, operator, value):
         return [('all_prs.label', operator, value)]
 
-    @api.depends("all_prs.target")
+    @api.depends("all_prs.target", "all_prs.closed")
     def _compute_target(self):
         for batch in self:
-            if len(batch.prs) == 1:
-                batch.target = batch.all_prs.target
-            else:
-                targets = set(batch.all_prs.mapped('target'))
-                if not targets:
-                    targets = set(batch.all_prs.mapped('target'))
-                if len(targets) == 1:
-                    batch.target = targets.pop()
-                else:
-                    batch.target = False
+            targets = batch.prs.mapped('target') or batch.all_prs.mapped('target')
+            batch.target = targets if len(targets) == 1 else False
 
     @api.depends(
         "merge_date",
diff --git a/runbot_merge/tests/test_batch_consistency.py b/runbot_merge/tests/test_batch_consistency.py
index 4a35389b..8855bfbb 100644
--- a/runbot_merge/tests/test_batch_consistency.py
+++ b/runbot_merge/tests/test_batch_consistency.py
@@ -33,9 +33,6 @@ def test_close_single(env, repo):
     assert Batches.search_count([]) == 0
 
 def test_close_multiple(env, make_repo2):
-    """If a batch has a single PR and that PR gets closed, the batch should be
-    inactive *and* blocked.
-    """
     Batches = env['runbot_merge.batch']
     repo1 = make_repo2('wheee')
     repo2 = make_repo2('wheeee')
@@ -81,3 +78,52 @@ def test_close_multiple(env, make_repo2):
     assert batch_id.prs == env['runbot_merge.pull_requests'].browse(())
     assert not batch_id.active
     assert Batches.search_count([]) == 0
+
+def test_inconsistent_target(env, project, make_repo2):
+    """If a batch's PRs have inconsistent targets,
+
+    - only open PRs should count
+    - it should be clearly notified on the dash
+    - the dash should not get hopelessly lost
+    - there should be a wizard to split the batch / move a PR to a separate batch
+    """
+    Batches = env['runbot_merge.batch']
+    repo1 = make_repo2('whe')
+    repo2 = make_repo2('whee')
+    repo3 = make_repo2('wheee')
+    project.write({'branch_ids': [(0, 0, {'name': 'other'})]})
+
+    with repo1:
+        repo1.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master')
+        repo1.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/other')
+        repo1.make_commits('master', Commit('b', tree={"b": "b"}), ref='heads/a_pr')
+        pr1 = repo1.make_pr(head='a_pr', target='master')
+
+    with repo2:
+        repo2.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master')
+        repo2.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/other')
+        repo2.make_commits('master', Commit('b', tree={"b": "b"}), ref='heads/a_pr')
+        pr2 = repo2.make_pr(head='a_pr', target='master')
+
+    with repo3:
+        repo3.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/master')
+        repo3.make_commits(None, Commit("a", tree={"a": "a"}), ref='heads/other')
+        repo3.make_commits('master', Commit('b', tree={"b": "b"}), ref='heads/a_pr')
+        pr3 = repo3.make_pr(head='a_pr', target='master')
+
+    [b] = Batches.search([])
+    assert b.target.name == 'master'
+    assert len(b.prs) == 3
+    assert len(b.all_prs) == 3
+
+    with repo3:
+        pr3.base = 'other'
+    assert b.target.name == False
+    assert len(b.prs) == 3
+    assert len(b.all_prs) == 3
+
+    with repo3:
+        pr3.close()
+    assert b.target.name == 'master'
+    assert len(b.prs) == 2
+    assert len(b.all_prs) == 3