[IMP] *: add draft support to mergebot, kinda

* Remove the forwardport creating PRs in draft, that was mostly to
  avoid codeowners triggering but we've removed the github one and
  hand-rolled it, so not a concern anymore.
* Prevent merging `draft` PRs, the mergebot rejects approval on draft
  PRs and insults people.

TBD (maybe): try to create *conflicting* forward-port PRs in draft so
it's clearer they need to be *fixed*? Issue of not being able to do
that on all private repositories remains so~~

Fixes #500

conftest.py

@@ -688,7 +688,7 @@ class Repo:
         )).raise_for_status()
         return PR(self, number)
 
-    def make_pr(self, *, title=None, body=None, target, head, token=None):
+    def make_pr(self, *, title=None, body=None, target, head, draft=False, token=None):
         assert self.hook
         self.hook = 2
 
@@ -717,6 +717,7 @@ class Repo:
                 'body': body,
                 'head': head,
                 'base': target,
+                'draft': draft,
             },
             headers=headers,
         )
@@ -774,6 +775,22 @@ class Comment(tuple):
     def __getitem__(self, item):
         return self._c[item]
 
+
+PR_SET_READY = '''
+mutation setReady($pid: ID!) {
+    markPullRequestReadyForReview(input: { pullRequestId: $pid}) {
+        clientMutationId
+    }
+}
+'''
+
+PR_SET_DRAFT = '''
+mutation setDraft($pid: ID!) {
+    convertPullRequestToDraft(input: { pullRequestId: $pid }) {
+        clientMutationId
+    }
+}
+'''
 class PR:
     def __init__(self, repo, number):
         self.repo = repo
@@ -796,6 +813,22 @@ class PR:
         raise NotImplementedError()
     base = base.setter(lambda self, v: self._set_prop('base', v))
 
+    @property
+    def draft(self):
+        return self._pr['draft']
+    @draft.setter
+    def draft(self, v):
+        assert self.repo.hook
+        # apparently it's not possible to update the draft flag via the v3 API,
+        # only the V4...
+        r = self.repo._session.post('https://api.github.com/graphql', json={
+            'query': PR_SET_DRAFT if v else PR_SET_READY,
+            'variables': {'pid': self._pr['node_id']}
+        })
+        assert r.ok, r.text
+        out = r.json()
+        assert 'errors' not in out, out['errors']
+
     @property
     def head(self):
         return self._pr['head']['sha']
@@ -863,7 +896,7 @@ class PR:
         r = self.repo._session.patch('https://api.github.com/repos/{}/pulls/{}'.format(self.repo.name, self.number), json={
             prop: value
         }, headers=headers)
-        assert 200 <= r.status_code < 300, r.json()
+        assert r.ok, r.text
 
     def open(self, token=None):
         self._set_prop('state', 'open', token=token)

forwardport/models/forwardport.py

@@ -123,7 +123,7 @@ class UpdateQueue(models.Model, Queue):
                                    )
                     })
                     return
-                # QUESTION: update PR to draft if there are conflicts?
+
                 conflicts, working_copy = previous._create_fp_branch(
                     child.target, child.refname, s)
                 if conflicts:

forwardport/models/project.py

@@ -662,16 +662,9 @@ class PullRequests(models.Model):
             url = 'https://api.github.com/repos/{}/pulls'.format(pr.repository.name)
             pr_data = {
                 'base': target.name, 'head': '%s:%s' % (owner, new_branch),
-                'title': title, 'body': body, 'draft': True
+                'title': title, 'body': body
             }
             r = gh.post(url, json=pr_data)
-            if r.status_code == 422:
-                # assume this is a private repo which doesn't support draft PRs
-                # (github error response doesn't provide any machine
-                # information, only a human-readable message) so retry without
-                del pr_data['draft']
-                r = gh.post(url, json=pr_data)
-
             if not r.ok:
                 _logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name)
                 # delete all the branches this should automatically close the

forwardport/tests/test_conflicts.py

@@ -92,7 +92,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     time.sleep(5)
     env.run_crons()
     assert pra_id | prb_id | prc_id == env['runbot_merge.pull_requests'].search([], order='number'),\
-        "CI passing should not have resumed the FP process on a conflicting / draft PR"
+        "CI passing should not have resumed the FP process on a conflicting PR"
 
     # fix the PR, should behave as if this were a normal PR
     prc = prod.get_pr(prc_id.number)
@@ -199,7 +199,7 @@ def test_conflict_deleted(env, config, make_repo):
     time.sleep(5)
     env.run_crons()
     assert pr0 | pr1 == env['runbot_merge.pull_requests'].search([], order='number'),\
-        "CI passing should not have resumed the FP process on a conflicting / draft PR"
+        "CI passing should not have resumed the FP process on a conflicting PR"
 
     # fix the PR, should behave as if this were a normal PR
     get_pr = prod.get_pr(pr1.number)

forwardport/tests/test_simple.py

@@ -234,7 +234,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
 
 def test_empty(env, config, make_repo, users):
     """ Cherrypick of an already cherrypicked (or separately implemented)
-    commit -> create draft PR.
+    commit -> conflicting pr.
     """
     prod, other = make_basic(env, config, make_repo)
     # merge change to b

forwardport/tests/test_weird.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 import pytest
 
-from utils import seen, Commit
+from utils import seen, Commit, to_pr
 
 
 def make_basic(env, config, make_repo, *, fp_token, fp_remote):
@@ -658,3 +658,28 @@ def test_skip_ci_next(env, config, make_repo):
     _, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number')
     assert pr1_id.state == 'opened'
     assert pr2_id.state == 'opened'
+
+def test_approve_draft(env, config, make_repo, users):
+    _, prod, _ = make_basic(env, config, make_repo, fp_token=True, fp_remote=True)
+
+    with prod:
+        prod.make_commits('a', Commit('x', tree={'x': '0'}), ref='heads/change')
+        pr = prod.make_pr(target='a', head='change', draft=True)
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    pr_id = to_pr(env, pr)
+    assert pr_id.state == 'opened'
+    assert pr.comments == [
+        (users['reviewer'], 'hansen r+'),
+        seen(env, pr, users),
+        (users['user'], f"I'm sorry, @{users['reviewer']}. Draft PRs can not be approved."),
+    ]
+
+    with prod:
+        pr.draft = False
+    assert pr.draft is False
+    with prod:
+        pr.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+    assert pr_id.state == 'approved'

runbot_merge/controllers/__init__.py

@@ -189,7 +189,14 @@ def handle_pr(env, event):
             'head': pr['head']['sha'],
             'squash': pr['commits'] == 1,
         })
-        return 'Updated {} to {}'.format(pr_obj.id, pr_obj.head)
+        return 'Updated {} to {}'.format(pr_obj.display_name, pr_obj.head)
+
+    if event['action'] == 'ready_for_review':
+        pr_obj.draft = False
+        return f'Updated {pr_obj.display_name} to ready'
+    if event['action'] == 'converted_to_draft':
+        pr_obj.draft = True
+        return f'Updated {pr_obj.display_name} to draft'
 
     # don't marked merged PRs as closed (!!!)
     if event['action'] == 'closed' and pr_obj.state != 'merged':
@@ -201,7 +208,7 @@ def handle_pr(env, event):
                 pr_obj.display_name,
                 oldstate,
             )
-            return 'Closed {}'.format(pr_obj.id)
+            return 'Closed {}'.format(pr_obj.display_name)
         else:
             _logger.warning(
                 '%s tried to close %s (state=%s)',
@@ -227,7 +234,7 @@ def handle_pr(env, event):
                 'squash': pr['commits'] == 1,
             })
 
-            return 'Reopened {}'.format(pr_obj.id)
+            return 'Reopened {}'.format(pr_obj.display_name)
 
     _logger.info("Ignoring event %s on PR %s", event['action'], pr['number'])
     return "Not handling {} yet".format(event['action'])

runbot_merge/models/pull_requests.py

@@ -688,6 +688,7 @@ class PullRequests(models.Model):
              "cross-repository branch-matching"
     )
     message = fields.Text(required=True)
+    draft = fields.Boolean(default=False, required=True)
     squash = fields.Boolean(default=False)
     merge_method = fields.Selection([
         ('merge', "merge directly, using the PR as merge commit message"),
@@ -999,7 +1000,9 @@ class PullRequests(models.Model):
                         'number': self.number,
                     })
             elif command == 'review':
-                if param and is_reviewer:
+                if self.draft:
+                    msg = "Draft PRs can not be approved."
+                elif param and is_reviewer:
                     oldstate = self.state
                     newstate = RPLUS.get(self.state)
                     if newstate:
@@ -1250,6 +1253,7 @@ class PullRequests(models.Model):
             'head': description['head']['sha'],
             'squash': description['commits'] == 1,
             'message': message,
+            'draft': description['draft'],
         })
 
     def write(self, vals):

runbot_merge/tests/test_multirepo.py

@@ -902,6 +902,7 @@ class TestSubstitutions:
                     },
                     'pull_request': {
                         'state': 'open',
+                        'draft': False,
                         'user': {'login': 'bob'},
                         'base': {
                             'repo': {'full_name': r.name},