[IMP] forwardport: unmodified fw automatically inherit overrides

Before this change, a CI override would have to be replicated on most
/ all forward-ports of the base PR. This was intentional to see how it
would shake out, the answer being that it's rather annoying.

Also add a `statuses_full` computed field on PRs for the aggregate
status: the existing `statuses` field is just a copy of the commit
statuses which I didn't remember I kept free of the overrides so the
commit statuses could be displayed "as-is" in the backend (the
overrides are displayed separately). And while at it fix the PR
dashboard to use that new field: that was basically the intention but
then I went on to use the "wrong" field hence #433.

Mebbe the UI part should be displayed using a computed M2M (?)
as a table or as tags instead? This m2m could indicate whether the
status is an override or an "intrinsic" status.

Also removed some dead code:

* leftover from the removed tagging feature (removed the tag
  manipulation but forgot some of the setup / computations)
* unused local variables
* an empty skipped test case

Fixes #439.

Fixes #433.

forwardport/models/project.py

@@ -513,6 +513,16 @@ class PullRequests(models.Model):
             else:
                 break
 
+    @api.depends('parent_id.statuses')
+    def _compute_statuses(self):
+        super()._compute_statuses()
+
+    def _get_overrides(self):
+        # NB: assumes _get_overrides always returns an "owned" dict which we can modify
+        p = self.parent_id._get_overrides() if self.parent_id else {}
+        p.update(super()._get_overrides())
+        return p
+
     def _iter_ancestors(self):
         while self:
             yield self

forwardport/tests/test_overrides.py

@@ -0,0 +1,116 @@
+import json
+
+from utils import Commit, make_basic
+
+def statuses(pr):
+    return {
+        k: v['state']
+        for k, v in json.loads(pr.statuses_full).items()
+    }
+def test_override_inherited(env, config, make_repo, users):
+    """ A forwardport should inherit its parents' overrides, until it's edited.
+    """
+    repo, other = make_basic(env, config, make_repo)
+    project = env['runbot_merge.project'].search([])
+    env['res.partner'].search([('github_login', '=', users['reviewer'])])\
+        .write({'override_rights': [(0, 0, {
+            'repository_id': project.repo_ids.id,
+            'context': 'ci/runbot',
+        })]})
+
+    with repo:
+        repo.make_commits('a', Commit('C', tree={'a': '0'}), ref='heads/change')
+        pr = repo.make_pr(target='a', head='change')
+        repo.post_status('change', 'success', 'legal/cla')
+        pr.post_comment('hansen r+ override=ci/runbot', config['role_reviewer']['token'])
+    env.run_crons()
+
+    original = env['runbot_merge.pull_requests'].search([('repository.name', '=', repo.name), ('number', '=', pr.number)])
+    assert original.state == 'ready'
+
+    with repo:
+        repo.post_status('staging.a', 'success', 'legal/cla')
+        repo.post_status('staging.a', 'success', 'ci/runbot')
+    env.run_crons()
+
+    pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number')
+    assert pr0_id == original
+    assert pr1_id.parent_id, pr0_id
+
+    with repo:
+        repo.post_status(pr1_id.head, 'success', 'legal/cla')
+    env.run_crons()
+    assert pr1_id.state == 'validated'
+    assert statuses(pr1_id) == {'ci/runbot': 'success', 'legal/cla': 'success'}
+
+    # now we edit the child PR
+    pr_repo, pr_ref = repo.get_pr(pr1_id.number).branch
+    with pr_repo:
+        pr_repo.make_commits(
+            pr1_id.target.name,
+            Commit('wop wop', tree={'a': '1'}),
+            ref=f'heads/{pr_ref}',
+            make=False
+        )
+    env.run_crons()
+    assert pr1_id.state == 'opened'
+    assert not pr1_id.parent_id
+    assert statuses(pr1_id) == {}, "should not have any status left"
+
+def test_override_combination(env, config, make_repo, users):
+    """ A forwardport should inherit its parents' overrides, until it's edited.
+    """
+    repo, other = make_basic(env, config, make_repo)
+    project = env['runbot_merge.project'].search([])
+    env['res.partner'].search([('github_login', '=', users['reviewer'])]) \
+        .write({'override_rights': [
+        (0, 0, {
+            'repository_id': project.repo_ids.id,
+            'context': 'ci/runbot',
+        }),
+        (0, 0, {
+            'repository_id': project.repo_ids.id,
+            'context': 'legal/cla',
+        })
+    ]})
+
+    with repo:
+        repo.make_commits('a', Commit('C', tree={'a': '0'}), ref='heads/change')
+        pr = repo.make_pr(target='a', head='change')
+        repo.post_status('change', 'success', 'legal/cla')
+        pr.post_comment('hansen r+ override=ci/runbot', config['role_reviewer']['token'])
+    env.run_crons()
+
+    pr0_id = env['runbot_merge.pull_requests'].search([('repository.name', '=', repo.name), ('number', '=', pr.number)])
+    assert pr0_id.state == 'ready'
+    assert statuses(pr0_id) == {'ci/runbot': 'success', 'legal/cla': 'success'}
+
+    with repo:
+        repo.post_status('staging.a', 'success', 'legal/cla')
+        repo.post_status('staging.a', 'success', 'ci/runbot')
+    env.run_crons()
+
+    # check for combination: ci/runbot is overridden through parent, if we
+    # override legal/cla then the PR should be validated
+    pr1_id =  env['runbot_merge.pull_requests'].search([('parent_id', '=', pr0_id.id)])
+    assert pr1_id.state == 'opened'
+    assert statuses(pr1_id) == {'ci/runbot': 'success'}
+    with repo:
+        repo.get_pr(pr1_id.number).post_comment('hansen override=legal/cla', config['role_reviewer']['token'])
+    env.run_crons()
+    assert pr1_id.state == 'validated'
+
+    # editing the child should devalidate
+    pr_repo, pr_ref = repo.get_pr(pr1_id.number).branch
+    with pr_repo:
+        pr_repo.make_commits(
+            pr1_id.target.name,
+            Commit('wop wop', tree={'a': '1'}),
+            ref=f'heads/{pr_ref}',
+            make=False
+        )
+    env.run_crons()
+    assert pr1_id.state == 'opened'
+    assert not pr1_id.parent_id
+    assert statuses(pr1_id) == {'legal/cla': 'success'}, \
+        "should only have its own status left"

forwardport/tests/test_simple.py

@@ -1220,7 +1220,7 @@ class TestBranchDeletion:
             prod.post_status(c, 'success', 'ci/runbot')
             pr1.post_comment('hansen r+', config['role_reviewer']['token'])
 
-            [c] = other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch')
+            other.make_commits('a', Commit('c2', tree={'2': '0'}), ref='heads/bbranch')
             pr2 = prod.make_pr(target='a', head='%s:bbranch' % other.owner, title='b')
             pr2.close()
 

forwardport/tests/test_weird.py

@@ -173,7 +173,6 @@ def test_failed_staging(env, config, make_repo):
     env.run_crons()
 
     pr1_id, pr2_id, pr3_id = env['runbot_merge.pull_requests'].search([], order='number')
-    pr2 = prod.get_pr(pr2_id.number)
     pr3 = prod.get_pr(pr3_id.number)
     with prod:
         prod.post_status(pr3_id.head, 'success', 'legal/cla')

runbot_merge/controllers/dashboard.py

@@ -1,5 +1,4 @@
 # -*- coding: utf-8 -*-
-import ast
 import json
 
 import werkzeug.exceptions
@@ -43,7 +42,7 @@ class MergebotDashboard(Controller):
             # normalise `statuses` to map to a dict
             st = {
                 k: {'state': v} if isinstance(v, str) else v
-                for k, v in ast.literal_eval(pr_id.statuses).items()
+                for k, v in json.loads(pr_id.statuses_full).items()
             }
         return request.render('runbot_merge.view_pull_request', {
             'pr': pr_id,

runbot_merge/models/pull_requests.py

@@ -673,7 +673,14 @@ class PullRequests(models.Model):
     priority = fields.Integer(default=2, index=True)
 
     overrides = fields.Char(required=True, default='{}')
-    statuses = fields.Text(compute='_compute_statuses')
+    statuses = fields.Text(
+        compute='_compute_statuses',
+        help="Copy of the statuses from the HEAD commit, as a Python literal"
+    )
+    statuses_full = fields.Text(
+        compute='_compute_statuses',
+        help="Compilation of the full status of the PR (commit statuses + overrides), as JSON"
+    )
     status = fields.Char(compute='_compute_statuses')
     previous_failure = fields.Char(default='{}')
 
@@ -773,13 +780,19 @@ class PullRequests(models.Model):
                 pr.blocked = 'linked pr %s is not ready' % unready.display_name
                 continue
 
-    @api.depends('head', 'repository.status_ids')
+    def _get_overrides(self):
+        if self:
+            return json.loads(self.overrides)
+        return {}
+
+    @api.depends('head', 'repository.status_ids', 'overrides')
     def _compute_statuses(self):
         Commits = self.env['runbot_merge.commit']
         for pr in self:
             c = Commits.search([('sha', '=', pr.head)])
             st = json.loads(c.statuses or '{}')
-            statuses = {**st, **json.loads(pr.overrides)}
+            statuses = {**st, **pr._get_overrides()}
+            pr.statuses_full = json.dumps(statuses)
             if not statuses:
                 pr.status = pr.statuses = False
                 continue
@@ -1079,19 +1092,14 @@ class PullRequests(models.Model):
         failed = self.browse(())
         for pr in self:
             required = pr.repository.status_ids._for_pr(pr).mapped('context')
-            sts = {**statuses, **json.loads(pr.overrides)}
+            sts = {**statuses, **pr._get_overrides()}
 
-            a, r = [], []
             success = True
             for ci in required:
                 st = state_(sts, ci) or 'pending'
                 if st == 'success':
-                    r.append(f'\N{Ballot Box} {ci}')
-                    a.append(f'\N{Ballot Box with Check} {ci}')
                     continue
 
-                a.append(f'\N{Ballot Box} {ci}')
-                r.append(f'\N{Ballot Box with Check} {ci}')
                 success = False
                 if st in ('error', 'failure'):
                     failed |= pr
@@ -1192,8 +1200,6 @@ class PullRequests(models.Model):
         })
 
     def write(self, vals):
-        oldstate = { pr: pr._tagstate for pr in self }
-
         if vals.get('squash'):
             vals['merge_method'] = False
 
@@ -1472,7 +1478,6 @@ class Tagging(models.Model):
     tags_add = fields.Char(required=True, default='[]')
 
     def create(self, values):
-        before = str(values)
         if values.pop('state_from', None):
             values['tags_remove'] = ALL_TAGS
         if 'state_to' in values:

runbot_merge/tests/test_basic.py

@@ -659,7 +659,7 @@ def test_ff_failure_batch(env, repo, users, config):
 
     # block FF
     with repo:
-        m2 = repo.make_commit('heads/master', 'NO!', None, tree={'m': 'm2'})
+        repo.make_commit('heads/master', 'NO!', None, tree={'m': 'm2'})
 
     old_staging = repo.commit('heads/staging.master')
     # confirm staging
@@ -801,11 +801,6 @@ class TestPREdition:
         with repo:
             prx.base = 'master'
 
-@pytest.mark.skip(reason="What do?")
-def test_edit_staged(env, repo):
-    """
-    What should happen when editing the PR/metadata (not pushing) of a staged PR
-    """
 def test_close_staged(env, repo, config, page):
     """
     When closing a staged PR, cancel the staging
@@ -915,7 +910,7 @@ def test_rebase_failure(env, repo, users, config):
         return original(*args)
 
     env['runbot_merge.commit']._notify()
-    with mock.patch.object(GH, 'set_ref', autospec=True, side_effect=wrapper) as m:
+    with mock.patch.object(GH, 'set_ref', autospec=True, side_effect=wrapper):
         env['runbot_merge.project']._check_progress()
 
     env['runbot_merge.project']._send_feedback()