From d9661064d608a1fc1e363be0f5a0488f9375b3e2 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 10 Feb 2020 09:48:03 +0100
Subject: [PATCH] [FIX runbot_merge: prevent reopening a merged PR

Fixes #305
---
 conftest.py                          | 25 +++++++++++++---
 runbot_merge/controllers/__init__.py | 25 ++++++++++------
 runbot_merge/tests/test_basic.py     | 43 ++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/conftest.py b/conftest.py
index 74833dea..370a521e 100644
--- a/conftest.py
+++ b/conftest.py
@@ -402,6 +402,20 @@ class Repo:
             'ignored': True,
         })
 
+    def add_collaborator(self, login, token):
+        # send invitation to user
+        r = self._session.put('https://api.github.com/repos/{}/collaborators/{}'.format(self.name, login))
+        assert r.ok, r.json()
+        # accept invitation on behalf of user
+        r = requests.patch('https://api.github.com/user/repository_invitations/{}'.format(r.json()['id']), headers={
+            'Authorization': 'token ' + token
+        })
+        assert r.ok, r.json()
+        # sanity check that user is part of collaborators
+        r = self._session.get('https://api.github.com/repos/{}/collaborators'.format(self.name))
+        assert r.ok, r.json()
+        assert any(login == c['login'] for c in r.json())
+
     def _get_session(self, token):
         s = self._session
         if token:
@@ -809,15 +823,18 @@ class PR:
         )
         assert r.status_code == 204, r.json()
 
-    def _set_prop(self, prop, value):
+    def _set_prop(self, prop, value, token=None):
         assert self.repo.hook
+        headers = {}
+        if token:
+            headers['Authorization'] = 'token ' + token
         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()
 
-    def open(self):
-        self._set_prop('state', 'open')
+    def open(self, token=None):
+        self._set_prop('state', 'open', token=token)
 
     def close(self):
         self._set_prop('state', 'closed')
diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index 8998fcf9..7972e194 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -192,15 +192,24 @@ def handle_pr(env, event):
         else:
             return 'Ignored: could not lock rows (probably being merged)'
 
-    if event['action'] == 'reopened' and pr_obj.state == 'closed':
-        _logger.info('%s reopening %s', event['sender']['login'], pr_obj.display_name)
-        pr_obj.write({
-            'state': 'opened',
-            # updating the head triggers a revalidation
-            'head': pr['head']['sha'],
-        })
+    if event['action'] == 'reopened' :
+        if pr_obj.state == 'merged':
+            env['runbot_merge.pull_requests.feedback'].create({
+                'repository': pr_obj.repository.id,
+                'pull_request': pr_obj.number,
+                'close': True,
+                'message': "@%s ya silly goose you can't reopen a PR that's been merged PR." % event['sender']['login']
+            })
 
-        return 'Reopened {}'.format(pr_obj.id)
+        if pr_obj.state == 'closed':
+            _logger.info('%s reopening %s', event['sender']['login'], pr_obj.display_name)
+            pr_obj.write({
+                'state': 'opened',
+                # updating the head triggers a revalidation
+                'head': pr['head']['sha'],
+            })
+
+            return 'Reopened {}'.format(pr_obj.id)
 
     _logger.info("Ignoring event %s on PR %s", event['action'], pr['number'])
     return "Not handling {} yet".format(event['action'])
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 78a872b8..d63894f4 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -962,6 +962,49 @@ def test_reopen_state(env, repo):
     assert pr.state == 'validated', \
         "if a PR is reopened and had a CI'd head, it should be validated immediately"
 
+def test_reopen_merged_pr(env, repo, config, users):
+    """ Reopening a *merged* PR should cause us to immediately close it again,
+    and insult whoever did it
+    """
+    with repo:
+        [m] = repo.make_commits(
+            None,
+            repo.Commit('initial', tree={'0': '0'}),
+            ref = 'heads/master'
+        )
+
+        [c] = repo.make_commits(
+            m, repo.Commit('second', tree={'0': '1'}),
+            ref='heads/abranch'
+        )
+        prx = repo.make_pr(target='master', head='abranch')
+        repo.post_status(c, 'success', 'legal/cla')
+        repo.post_status(c, 'success', 'ci/runbot')
+        prx.post_comment('hansen r+', config['role_reviewer']['token'])
+    env.run_crons()
+
+    with repo:
+        repo.post_status('staging.master', 'success', 'legal/cla')
+        repo.post_status('staging.master', 'success', 'ci/runbot')
+    env.run_crons()
+    pr = env['runbot_merge.pull_requests'].search([
+        ('repository.name', '=', repo.name),
+        ('number', '=', prx.number)
+    ])
+    assert prx.state == 'closed'
+    assert pr.state == 'merged'
+
+    repo.add_collaborator(users['other'], config['role_other']['token'])
+    with repo:
+        prx.open(config['role_other']['token'])
+    env.run_crons()
+    assert prx.state == 'closed'
+    assert pr.state == 'merged'
+    assert prx.comments == [
+        (users['reviewer'], 'hansen r+'),
+        (users['user'], "@%s ya silly goose you can't reopen a PR that's been merged PR." % users['other'])
+    ]
+
 class TestNoRequiredStatus:
     def test_basic(self, env, repo, config):
         """ check that mergebot can work on a repo with no CI at all