[FIX] runbot_merge: cancel approval (r+) when fetching PRs

When retrieving unknown PRs, the process would apply all comments,
thereby applying eventual r+ without taking in account their
relationship to a force push. This means it was possible for a
mergebot-unknown PR to be r+'d, updated, retargeted, and the mergetbot
would consider it good to go.

The possible damage would be somewhat limited but still, not great.

Sadly Github simply doesn't provide access to the entire event stream
of the PR, so there is no way to even know whether the PR was updated,
let alone when in relation to comments. Therefore just resync the PR
after having applied comments: we still want to apply the merge method
& al, we just want to reset back to un-approved.

An other minor fix (for something we never actually hit but could):
reviews are treated more or less as comments, but separate at github's
level. The job would apply all comments then all reviews, so the
relative order of comments and reviews would be wrong.

Combine and order comments and reviews so they are applied
in (hopefully) the correct order of their creation / submission.

Closes #416

runbot_merge/models/pull_requests.py

@@ -304,10 +304,11 @@ All substitutions are tentatively applied sequentially to the input.
         # fetch PR object and handle as *opened*
         issue, pr = gh.pr(number)
 
+        feedback = self.env['runbot_merge.pull_requests.feedback'].create
         if not self.project_id._has_branch(pr['base']['ref']):
             _logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring",
-                         pr['number'], self.name, pr['base']['ref'])
-            self.env['runbot_merge.pull_requests.feedback'].create({
+                         number, self.name, pr['base']['ref'])
+            feedback({
                 'repository': self.id,
                 'pull_request': number,
                 'message': "I'm sorry. Branch `{}` is not within my remit.".format(pr['base']['ref']),
@@ -317,7 +318,7 @@ All substitutions are tentatively applied sequentially to the input.
         # if the PR is already loaded, check... if the heads match?
         pr_id = self.env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', pr['base']['repo']['full_name']),
-            ('number', '=', pr['number']),
+            ('number', '=', number),
         ])
         if pr_id:
             # TODO: edited, maybe (requires crafting a 'changes' object)
@@ -325,36 +326,60 @@ All substitutions are tentatively applied sequentially to the input.
                 'action': 'synchronize',
                 'pull_request': pr,
             })
-            self.env['runbot_merge.pull_requests.feedback'].create({
+            feedback({
                 'repository': pr_id.repository.id,
                 'pull_request': number,
                 'message': r,
             })
             return
 
+        # init the PR to the null commit so we can later synchronise it back
+        # back to the "proper" head while resetting reviews
         controllers.handle_pr(self.env, {
             'action': 'opened',
-            'pull_request': pr,
+            'pull_request': {**pr, 'head': {**pr['head'], 'sha': '0'*40}},
         })
+        # fetch & set up actual head
         for st in gh.statuses(pr['head']['sha']):
             controllers.handle_status(self.env, st)
-        # get and handle all comments
-        for comment in gh.comments(number):
-            controllers.handle_comment(self.env, {
-                'action': 'created',
-                'issue': issue,
-                'sender': comment['user'],
-                'comment': comment,
-                'repository': {'full_name': self.name},
-            })
-        # get and handle all reviews
-        for review in gh.reviews(number):
-            controllers.handle_review(self.env, {
-                'action': 'submitted',
-                'review': review,
-                'pull_request': pr,
-                'repository': {'full_name': self.name},
-            })
+        # fetch and apply comments
+        counter = itertools.count()
+        items = [ # use counter so `comment` and `review` don't get hit during sort
+            (comment['created_at'], next(counter), False, comment)
+            for comment in gh.comments(number)
+        ] + [
+            (review['submitted_at'], next(counter), True, review)
+            for review in gh.reviews(number)
+        ]
+        items.sort()
+        for _, _, is_review, item in items:
+            if is_review:
+                controllers.handle_review(self.env, {
+                    'action': 'submitted',
+                    'review': item,
+                    'pull_request': pr,
+                    'repository': {'full_name': self.name},
+                })
+            else:
+                controllers.handle_comment(self.env, {
+                    'action': 'created',
+                    'issue': issue,
+                    'sender': item['user'],
+                    'comment': item,
+                    'repository': {'full_name': self.name},
+                })
+        # sync to real head
+        controllers.handle_pr(self.env, {
+            'action': 'synchronize',
+            'pull_request': pr,
+            'sender': {'login': self.project_id.github_prefix}
+        })
+        feedback({
+            'repository': self.id,
+            'pull_request': number,
+            'message': "Sorry, I didn't know about this PR and had to retrieve "
+                       "its information, you may have to re-approve it."
+        })
 
     def having_branch(self, branch):
         branches = self.env['runbot_merge.branch'].search

runbot_merge/tests/test_basic.py

@@ -2725,11 +2725,8 @@ class TestUnknownPR:
 
     => instead, create PRs on the fly when getting notifications related to
        valid but unknown PRs
-
-    * get statuses if head commit unknown (additional cron?)
-    * handle any comment & review (existing PRs may enter the system on a review/r+)
     """
-    def test_rplus_unknown(self, repo, env, config):
+    def test_rplus_unknown(self, repo, env, config, users):
         with repo:
             m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
             m2 = repo.make_commit(m, 'second', None, tree={'m': 'm', 'm2': 'm2'})
@@ -2751,27 +2748,42 @@ class TestUnknownPR:
         # reviewer reviewers
         with repo:
             prx.post_comment('hansen r+', config['role_reviewer']['token'])
+        with repo:
+            prx.post_review('REQUEST_CHANGES', 'hansen r-', config['role_reviewer']['token'])
+        with repo:
+            prx.post_comment('hansen r+', config['role_reviewer']['token'])
 
         Fetch = env['runbot_merge.fetch_job']
-        assert Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)])
+        fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)])
+        assert len(fetches) == 1, f"expected one fetch for {prx.number}, found {len(fetches)}"
+
         env.run_crons('runbot_merge.fetch_prs_cron')
         env.run_crons()
         assert not Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)])
 
         c = env['runbot_merge.commit'].search([('sha', '=', prx.head)])
+        print(prx.head, c, c.statuses, flush=True)
         assert json.loads(c.statuses) == {
             'legal/cla': {'state': 'success', 'target_url': None, 'description': None},
             'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None}
         }
+        assert prx.comments == [
+            (users['reviewer'], 'hansen r+'),
+            (users['reviewer'], 'hansen r+'),
+            (users['user'], "Sorry, I didn't know about this PR and had to "
+                            "retrieve its information, you may have to "
+                            "re-approve it."),
+        ]
 
         pr = env['runbot_merge.pull_requests'].search([
             ('repository.name', '=', repo.name),
             ('number', '=', prx.number)
         ])
-        assert pr.state == 'ready'
+        assert pr.state == 'validated'
 
-        env.run_crons('runbot_merge.merge_cron', 'runbot_merge.staging_cron')
-        assert pr.staging_id
+        with repo:
+            prx.post_comment('hansen r+', config['role_reviewer']['token'])
+        assert pr.state == 'ready'
 
     def test_rplus_unmanaged(self, env, repo, users, config):
         """ r+ on an unmanaged target should notify about