[FIX] runbot_merge: don't notify on closing unknown PRs

If an untracked PR is closed, especially on an inactive or untracked
branch, the closer (or author) almost certainly don't care to receive
3 different notifications on the subject.

The fix requires a schema change in order to track that we're fetching
the PR due to a `closed` event, as in other cases we may still want to
notify the user that we received the request (and it just happened to
resolve to a closed PR).

Fixes #857

runbot_merge/controllers/__init__.py

@@ -257,7 +257,7 @@ def handle_pr(env, event):
         pr_obj = env['runbot_merge.pull_requests']._from_gh(pr)
         return "Tracking PR as {}".format(pr_obj.id)
 
-    pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number'])
+    pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number'], closing=event['action'] == 'closed')
     if not pr_obj:
         _logger.info("webhook %s on unknown PR %s#%s, scheduled fetch", event['action'], repo.name, pr['number'])
         return "Unknown PR {}:{}, scheduling fetch".format(repo.name, pr['number'])

runbot_merge/models/pull_requests.py

@@ -96,28 +96,30 @@ All substitutions are tentatively applied sequentially to the input.
             self._cr, 'runbot_merge_unique_repo', self._table, ['name'])
         return res
 
-    def _load_pr(self, number):
+    def _load_pr(self, number, *, closing=False):
         gh = self.github()
 
         # fetch PR object and handle as *opened*
         issue, pr = gh.pr(number)
 
+        repo_name = pr['base']['repo']['full_name']
         if not self.project_id._has_branch(pr['base']['ref']):
-            _logger.info("Tasked with loading PR %d for un-managed branch %s:%s, ignoring",
-                         number, self.name, pr['base']['ref'])
-            self.env.ref('runbot_merge.pr.load.unmanaged')._send(
-                repository=self,
-                pull_request=number,
-                format_args = {
-                    'pr': pr,
-                    'repository': self,
-                },
-            )
+            _logger.info("Tasked with loading %s PR %s#%d for un-managed branch %s:%s, ignoring",
+                         pr['state'], repo_name, number, self.name, pr['base']['ref'])
+            if not closing:
+                self.env.ref('runbot_merge.pr.load.unmanaged')._send(
+                    repository=self,
+                    pull_request=number,
+                    format_args = {
+                        'pr': pr,
+                        'repository': self,
+                    },
+                )
             return
 
         # if the PR is already loaded, force sync a few attributes
         pr_id = self.env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', pr['base']['repo']['full_name']),
+            ('repository.name', '=', repo_name),
             ('number', '=', number),
         ])
         if pr_id:
@@ -143,6 +145,11 @@ All substitutions are tentatively applied sequentially to the input.
                     'pull_request': pr,
                     'sender': {'login': self.project_id.github_prefix}
                 }) + '. '
+            if pr_id.state != 'closed' and pr['state'] == 'closed':
+                # don't go through controller because try_closing does weird things
+                # for safety / race condition reasons which ends up committing
+                # and breaks everything
+                pr_id.state = 'closed'
             self.env['runbot_merge.pull_requests.feedback'].create({
                 'repository': pr_id.repository.id,
                 'pull_request': number,
@@ -150,6 +157,11 @@ All substitutions are tentatively applied sequentially to the input.
             })
             return
 
+        # special case for closed PRs, just ignore all events and skip feedback
+        if closing:
+            self.env['runbot_merge.pull_requests']._from_gh(pr)
+            return
+
         sender = {'login': self.project_id.github_prefix}
         # init the PR to the null commit so we can later synchronise it back
         # back to the "proper" head while resetting reviews
@@ -199,7 +211,7 @@ All substitutions are tentatively applied sequentially to the input.
             'sender': sender,
         })
         pr_id = self.env['runbot_merge.pull_requests'].search([
-            ('repository.name', '=', pr['base']['repo']['full_name']),
+            ('repository.name', '=', repo_name),
             ('number', '=', number),
         ])
         if pr['state'] == 'closed':
@@ -520,7 +532,7 @@ class PullRequests(models.Model):
         for r in self:
             r.batch_id = r.batch_ids.filtered(lambda b: b.active)[:1]
 
-    def _get_or_schedule(self, repo_name, number, target=None):
+    def _get_or_schedule(self, repo_name, number, *, target=None, closing=False):
         repo = self.env['runbot_merge.repository'].search([('name', '=', repo_name)])
         if not repo:
             return
@@ -546,6 +558,7 @@ class PullRequests(models.Model):
         Fetch.create({
             'repository': repo.id,
             'number': number,
+            'closing': closing,
         })
 
     def _parse_command(self, commandstring):
@@ -1823,6 +1836,7 @@ class FetchJob(models.Model):
     active = fields.Boolean(default=True)
     repository = fields.Many2one('runbot_merge.repository', required=True)
     number = fields.Integer(required=True, group_operator=None)
+    closing = fields.Boolean(default=False)
 
     def _check(self, commit=False):
         """
@@ -1835,7 +1849,7 @@ class FetchJob(models.Model):
 
             self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
             try:
-                f.repository._load_pr(f.number)
+                f.repository._load_pr(f.number, closing=f.closing)
             except Exception:
                 self.env.cr.execute("ROLLBACK TO SAVEPOINT runbot_merge_before_fetch")
                 _logger.exception("Failed to load pr %s, skipping it", f.number)

runbot_merge/tests/test_basic.py

@@ -3218,6 +3218,74 @@ class TestUnknownPR:
                             "as I didn't see previous commands."),
         ]
 
+    def test_close_unknown_unmanaged(self, env, repo, users, config):
+        """If an "unknown PR" is *closed*, it should be saved as closed but not
+        commented on, because that's unnecessary spam.
+        """
+        with repo:
+            m, _ = repo.make_commits(
+                None,
+                Commit('initial', tree={'m': 'm'}),
+                Commit('second', tree={'m2': 'm2'}),
+                ref='heads/master')
+
+            [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'}))
+            pr = repo.make_pr(title='title', body='body', target='master', head=c1)
+        env.run_crons()
+        assert pr.comments == [seen(env, pr, users)]
+
+        to_pr(env, pr).unlink()
+        env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink()
+
+        with repo:
+            pr.close()
+
+        Fetch = env['runbot_merge.fetch_job']
+        fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+        assert len(fetches) == 1, f"expected one fetch for {pr.number}, found {len(fetches)}"
+
+        env.run_crons('runbot_merge.fetch_prs_cron')
+        env.run_crons()
+        assert not Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+
+        assert to_pr(env, pr).state == 'closed'
+        assert pr.comments == [seen(env, pr, users)]
+
+
+    def test_close_unknown_disabled(self, env, repo, users, config):
+        """If an "unknown PR" on an disabled branch is *closed*, it should be
+        saved as closed but not commented on, because that's unnecessary spam.
+        """
+        with repo:
+            m, _ = repo.make_commits(
+                None,
+                Commit('initial', tree={'m': 'm'}),
+                Commit('second', tree={'m2': 'm2'}),
+                ref='heads/master')
+
+            [c1] = repo.make_commits(m, Commit('first', tree={'m': 'c1'}))
+            pr = repo.make_pr(title='title', body='body', target='master', head=c1)
+        env.run_crons()
+        assert pr.comments == [seen(env, pr, users)]
+
+        to_pr(env, pr).unlink()
+        env['runbot_merge.commit'].search([('sha', '=', pr.head)]).unlink()
+        env['runbot_merge.branch'].search([('name', '=', 'master')]).active = False
+
+        with repo:
+            pr.close()
+
+        Fetch = env['runbot_merge.fetch_job']
+        fetches = Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+        assert len(fetches) == 1, f"expected one fetch for {pr.number}, found {len(fetches)}"
+
+        env.run_crons('runbot_merge.fetch_prs_cron')
+        env.run_crons()
+        assert not Fetch.search([('repository', '=', repo.name), ('number', '=', pr.number)])
+
+        assert to_pr(env, pr).state == 'closed'
+        assert pr.comments == [seen(env, pr, users)]
+
     def test_rplus_unmanaged(self, env, repo, users, config):
         """ r+ on an unmanaged target should notify about
         """