[IMP] *: allow disabling staging on branch, remove fp target flag

- currently disabling staging only works globally, allow disabling on
  a single branch

  - use a toggle
  - remove a pair of tests which work specifically with `fp_target`,
    can't work with `active` (probably)
  - cleanup search of possible and active stagings, add relevant
    indexes and use direct search of relevant branches instead of
    looking up from the project

- also use toggle button for `active` on branches
- shitty workaround for upgrading DB: apparently mail really wants to
  have a `user_id` to do some weird thing, so need to re-add it after
  resetting everything

Fixes #727

forwardport/data/views.xml

@@ -152,12 +152,6 @@
                        help="Repository where forward port branches will be created"
                 />
             </xpath>
-
-            <xpath expr="//field[@name='branch_ids']/tree" position="inside">
-                <field name="fp_target" string="FP to"
-                       help="This branch will be forward-ported to (from lower ones)"
-                />
-            </xpath>
         </field>
     </record>
 

forwardport/models/project.py

@@ -210,17 +210,6 @@ class Repository(models.Model):
     _inherit = 'runbot_merge.repository'
     fp_remote_target = fields.Char(help="where FP branches get pushed")
 
-class Branch(models.Model):
-    _inherit = 'runbot_merge.branch'
-
-    fp_target = fields.Boolean(default=True)
-    fp_enabled = fields.Boolean(compute='_compute_fp_enabled')
-
-    @api.depends('active', 'fp_target')
-    def _compute_fp_enabled(self):
-        for b in self:
-            b.fp_enabled = b.active and b.fp_target
-
 class PullRequests(models.Model):
     _inherit = 'runbot_merge.pull_requests'
 
@@ -438,7 +427,7 @@ class PullRequests(models.Model):
                         ping = False
                         msg = "Forward-port disabled."
                         self.limit_id = limit_id
-                    elif not limit_id.fp_enabled:
+                    elif not limit_id.active:
                         msg = "branch %r is disabled, it can't be used as a forward port target." % limit_id.name
                     else:
                         ping = False
@@ -550,7 +539,7 @@ class PullRequests(models.Model):
         return next((
             branch
             for branch in branches[from_+1:to_+1]
-            if branch.fp_enabled
+            if branch.active
         ), None)
 
     def _commits_lazy(self):

forwardport/tests/test_conflicts.py

@@ -16,7 +16,7 @@ def test_conflict(env, config, make_repo, users):
     project = env['runbot_merge.project'].search([])
     project.write({
         'branch_ids': [
-            (0, 0, {'name': 'd', 'sequence': 40, 'fp_target': True})
+            (0, 0, {'name': 'd', 'sequence': 40})
         ]
     })
 

forwardport/tests/test_limit.py

@@ -50,31 +50,6 @@ def test_configure(env, config, make_repo):
     assert prs[2].number == originals[2]
 
 
-def test_self_disabled(env, config, make_repo):
-    """ Allow setting target as limit even if it's disabled
-    """
-    prod, other = make_basic(env, config, make_repo)
-    bot_name = env['runbot_merge.project'].search([]).fp_github_name
-    branch_a = env['runbot_merge.branch'].search([('name', '=', 'a')])
-    branch_a.fp_target = False
-    with prod:
-        [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch')
-        pr = prod.make_pr(target='a', head='mybranch')
-        prod.post_status(c, 'success', 'legal/cla')
-        prod.post_status(c, 'success', 'ci/runbot')
-        pr.post_comment('hansen r+\n%s up to a' % bot_name, config['role_reviewer']['token'])
-    env.run_crons()
-    pr_id = env['runbot_merge.pull_requests'].search([('number', '=', pr.number)])
-    assert pr_id.limit_id == branch_a
-
-    with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-
-    assert env['runbot_merge.pull_requests'].search([]) == pr_id,\
-        "should not have created a forward port"
-
-
 def test_ignore(env, config, make_repo):
     """ Provide an "ignore" command which is equivalent to setting the limit
     to target
@@ -100,17 +75,12 @@ def test_ignore(env, config, make_repo):
         "should not have created a forward port"
 
 
-@pytest.mark.parametrize('enabled', ['active', 'fp_target'])
+def test_disable(env, config, make_repo, users):
-def test_disable(env, config, make_repo, users, enabled):
     """ Checks behaviour if the limit target is disabled:
 
     * disable target while FP is ongoing -> skip over (and stop there so no FP)
     * forward-port over a disabled branch
     * request a disabled target as limit
-
-    Disabling (with respect to forward ports) can be performed by marking the
-    branch as !active (which also affects mergebot operations), or as
-    !fp_target (won't be forward-ported to).
     """
     prod, other = make_basic(env, config, make_repo)
     project = env['runbot_merge.project'].search([])
@@ -133,7 +103,7 @@ def test_disable(env, config, make_repo, users, enabled):
         prod.post_status('staging.a', 'success', 'legal/cla')
         prod.post_status('staging.a', 'success', 'ci/runbot')
     # disable branch b
-    env['runbot_merge.branch'].search([('name', '=', 'b')]).write({enabled: False})
+    env['runbot_merge.branch'].search([('name', '=', 'b')]).active = False
     env.run_crons()
 
     # should have created a single PR (to branch c, for pr 1)
@@ -168,49 +138,6 @@ def test_disable(env, config, make_repo, users, enabled):
         seen(env, pr, users),
     }
 
-
-def test_default_disabled(env, config, make_repo, users):
-    """ If the default limit is disabled, it should still be the default
-    limit but the ping message should be set on the actual last FP (to the
-    last non-deactivated target)
-    """
-    prod, other = make_basic(env, config, make_repo)
-    branch_c = env['runbot_merge.branch'].search([('name', '=', 'c')])
-    branch_c.fp_target = False
-
-    with prod:
-        [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/branch0')
-        pr = prod.make_pr(target='a', head='branch0')
-        prod.post_status(c, 'success', 'legal/cla')
-        prod.post_status(c, 'success', 'ci/runbot')
-        pr.post_comment('hansen r+', config['role_reviewer']['token'])
-    env.run_crons()
-
-    assert env['runbot_merge.pull_requests'].search([]).limit_id == branch_c
-
-    with prod:
-        prod.post_status('staging.a', 'success', 'legal/cla')
-        prod.post_status('staging.a', 'success', 'ci/runbot')
-    env.run_crons()
-
-    p1, p2 = env['runbot_merge.pull_requests'].search([], order='number')
-    assert p1.number == pr.number
-    pr2 = prod.get_pr(p2.number)
-
-    cs = pr2.comments
-    assert len(cs) == 2
-    assert pr2.comments == [
-        seen(env, pr2, users),
-        (users['user'], """\
-@%(user)s @%(reviewer)s this PR targets b and is the last of the forward-port chain.
-
-To merge the full chain, say
-> @%(user)s r+
-
-More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
-""" % users)
-    ]
-
 def test_limit_after_merge(env, config, make_repo, users):
     """ If attempting to set a limit (<up to>) on a PR which is merged
     (already forward-ported or not), or is a forward-port PR, fwbot should

forwardport/tests/test_updates.py

@@ -151,9 +151,7 @@ def test_update_merged(env, make_repo, config, users):
     with prod:
         prod.make_ref('heads/d', prod.commit('c').id)
     env['runbot_merge.project'].search([]).write({
-        'branch_ids': [(0, 0, {
+        'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})]
-            'name': 'd', 'sequence': 40, 'fp_target': True,
-        })]
     })
 
     with prod:
@@ -251,10 +249,10 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users):
         'github_prefix': 'hansen',
         'fp_github_token': config['github']['token'],
         'branch_ids': [
-            (0, 0, {'name': 'master', 'sequence': 0, 'fp_target': True}),
+            (0, 0, {'name': 'master', 'sequence': 0}),
-            (0, 0, {'name': 'v3', 'sequence': 1, 'fp_target': True}),
+            (0, 0, {'name': 'v3', 'sequence': 1}),
-            (0, 0, {'name': 'v2', 'sequence': 2, 'fp_target': True}),
+            (0, 0, {'name': 'v2', 'sequence': 2}),
-            (0, 0, {'name': 'v1', 'sequence': 3, 'fp_target': True}),
+            (0, 0, {'name': 'v1', 'sequence': 3}),
         ],
         'repo_ids': [
             (0, 0, {

forwardport/tests/test_weird.py

@@ -26,9 +26,9 @@ def make_basic(env, config, make_repo, *, fp_token, fp_remote):
             'github_prefix': 'hansen',
             'fp_github_token': fp_token and config['github']['token'],
             'branch_ids': [
-                (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}),
+                (0, 0, {'name': 'a', 'sequence': 2}),
-                (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}),
+                (0, 0, {'name': 'b', 'sequence': 1}),
-                (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}),
+                (0, 0, {'name': 'c', 'sequence': 0}),
             ],
         })
 
@@ -263,9 +263,9 @@ class TestNotAllBranches:
             'github_prefix': 'hansen',
             'fp_github_token': config['github']['token'],
             'branch_ids': [
-                (0, 0, {'name': 'a', 'sequence': 2, 'fp_target': True}),
+                (0, 0, {'name': 'a', 'sequence': 2}),
-                (0, 0, {'name': 'b', 'sequence': 1, 'fp_target': True}),
+                (0, 0, {'name': 'b', 'sequence': 1}),
-                (0, 0, {'name': 'c', 'sequence': 0, 'fp_target': True}),
+                (0, 0, {'name': 'c', 'sequence': 0}),
             ]
         })
         repo_a = env['runbot_merge.repository'].create({
@@ -514,7 +514,7 @@ def test_new_intermediate_branch(env, config, make_repo):
     env.run_crons()
     project.write({
         'branch_ids': [
-            (0, False, {'name': 'new', 'sequence': 1, 'fp_target': True}),
+            (0, False, {'name': 'new', 'sequence': 1}),
         ]
     })
     env.run_crons()
@@ -748,7 +748,7 @@ def test_retarget_after_freeze(env, config, make_repo, users):
     project.write({
         'branch_ids': [
             (1, branch_c.id, {'sequence': 1}),
-            (0, 0, {'name': 'bprime', 'sequence': 2, 'fp_target': True}),
+            (0, 0, {'name': 'bprime', 'sequence': 2}),
             (1, branch_b.id, {'sequence': 3}),
             (1, branch_a.id, {'sequence': 4}),
         ]

mergebot_test_utils/utils.py

@@ -74,9 +74,9 @@ def make_basic(env, config, make_repo, *, reponame='proj', project_name='myproje
             'github_prefix': 'hansen',
             'fp_github_token': config['github']['token'],
             'branch_ids': [
-                (0, 0, {'name': 'a', 'sequence': 100, 'fp_target': True}),
+                (0, 0, {'name': 'a', 'sequence': 100}),
-                (0, 0, {'name': 'b', 'sequence': 80, 'fp_target': True}),
+                (0, 0, {'name': 'b', 'sequence': 80}),
-                (0, 0, {'name': 'c', 'sequence': 60, 'fp_target': True}),
+                (0, 0, {'name': 'c', 'sequence': 60}),
             ],
         })
 

runbot_merge/models/project.py

@@ -46,10 +46,11 @@ class Project(models.Model):
     freeze_reminder = fields.Text()
 
     def _check_stagings(self, commit=False):
-        for branch in self.search([]).mapped('branch_ids').filtered('active'):
+        # check branches with an active staging
+        for branch in self.env['runbot_merge.branch']\
+                .with_context(active_test=False)\
+                .search([('active_staging_id', '!=', False)]):
             staging = branch.active_staging_id
-            if not staging:
-                continue
             try:
                 with self.env.cr.savepoint():
                     staging.check_status()
@@ -61,16 +62,20 @@ class Project(models.Model):
                     self.env.cr.commit()
 
     def _create_stagings(self, commit=False):
-        for branch in self.search([]).mapped('branch_ids').filtered('active'):
+        # look up branches which can be staged on and have no active staging
-            if not branch.active_staging_id:
+        for branch in self.env['runbot_merge.branch'].search([
-                try:
+            ('active_staging_id', '=', False),
-                    with self.env.cr.savepoint():
+            ('active', '=', True),
-                        branch.try_staging()
+            ('staging_enabled', '=', True),
-                except Exception:
+        ]):
-                    _logger.exception("Failed to create staging for branch %r", branch.name)
+            try:
-                else:
+                with self.env.cr.savepoint():
-                    if commit:
+                    branch.try_staging()
-                        self.env.cr.commit()
+            except Exception:
+                _logger.exception("Failed to create staging for branch %r", branch.name)
+            else:
+                if commit:
+                    self.env.cr.commit()
 
     def _find_commands(self, comment):
         return re.findall(

runbot_merge/models/pull_requests.py

@@ -67,7 +67,7 @@ class Repository(models.Model):
 
     sequence = fields.Integer(default=50, group_operator=None)
     name = fields.Char(required=True)
-    project_id = fields.Many2one('runbot_merge.project', required=True)
+    project_id = fields.Many2one('runbot_merge.project', required=True, index=True)
     status_ids = fields.One2many('runbot_merge.repository.status', 'repo_id', string="Required Statuses")
 
     group_id = fields.Many2one('res.groups', default=lambda self: self.env.ref('base.group_user'))
@@ -235,10 +235,10 @@ class Branch(models.Model):
     _order = 'sequence, name'
 
     name = fields.Char(required=True)
-    project_id = fields.Many2one('runbot_merge.project', required=True)
+    project_id = fields.Many2one('runbot_merge.project', required=True, index=True)
 
     active_staging_id = fields.Many2one(
-        'runbot_merge.stagings', compute='_compute_active_staging', store=True,
+        'runbot_merge.stagings', compute='_compute_active_staging', store=True, index=True,
         help="Currently running staging for the branch."
     )
     staging_ids = fields.One2many('runbot_merge.stagings', 'target')
@@ -252,6 +252,8 @@ class Branch(models.Model):
     active = fields.Boolean(default=True)
     sequence = fields.Integer(group_operator=None)
 
+    staging_enabled = fields.Boolean(default=True)
+
     def _auto_init(self):
         res = super(Branch, self)._auto_init()
         tools.create_unique_index(

runbot_merge/tests/test_staging.py

@@ -0,0 +1,41 @@
+import pytest
+
+from utils import Commit, to_pr
+
+
+@pytest.fixture
+def repo(env, project, make_repo, users, setreviewers):
+    r = make_repo('repo')
+    project.write({'repo_ids': [(0, 0, {
+        'name': r.name,
+        'group_id': False,
+        'required_statuses': 'ci'
+    })]})
+    setreviewers(*project.repo_ids)
+    return r
+
+def test_staging_disabled_branch(env, project, repo, config):
+    """Check that it's possible to disable staging on a specific branch
+    """
+    project.branch_ids = [(0, 0, {
+        'name': 'other',
+        'staging_enabled': False,
+    })]
+    with repo:
+        [master_commit] = repo.make_commits(None, Commit("master", tree={'a': '1'}), ref="heads/master")
+        [c1] = repo.make_commits(master_commit, Commit("thing", tree={'a': '2'}), ref='heads/master-thing')
+        master_pr = repo.make_pr(title="whatever", target="master", head="master-thing")
+        master_pr.post_comment("hansen r+", config['role_reviewer']['token'])
+        repo.post_status(c1, 'success', 'ci')
+
+        [other_commit] = repo.make_commits(None, Commit("other", tree={'b': '1'}), ref='heads/other')
+        [c2] = repo.make_commits(other_commit, Commit("thing", tree={'b': '2'}), ref='heads/other-thing')
+        other_pr = repo.make_pr(title="whatever", target="other", head="other-thing")
+        other_pr.post_comment("hansen r+", config['role_reviewer']['token'])
+        repo.post_status(c2, 'success', 'ci')
+    env.run_crons()
+
+    assert to_pr(env, master_pr).staging_id, \
+        "master is allowed to stage, should be staged"
+    assert not to_pr(env, other_pr).staging_id, \
+        "other is *not* allowed to stage, should not be staged"

runbot_merge/views/res_partner.xml

@@ -25,6 +25,7 @@
                     <field name="display_name" string="Name"/>
                     <field name="github_login"/>
                     <field name="review_rights" widget="many2many_tags"/>
+                    <field name="user_id" invisible="1"/>
                 </tree>
             </xpath>
         </field>

runbot_merge/views/runbot_merge_project.xml

@@ -56,7 +56,8 @@
                         <tree editable="bottom" decoration-muted="not active">
                             <field name="sequence" widget="handle" />
                             <field name="name"/>
-                            <field name="active"/>
+                            <field name="active" widget="boolean_toggle"/>
+                            <field name="staging_enabled" widget="boolean_toggle"/>
                         </tree>
                     </field>
                 </sheet>