[IMP] runbot_merge: ping commenter when fetching PR due to comment

If a comment causes an unknown PR to be fetched, it's a bit odd to
ping the author (and possibly reviewer) anyway as they're not super
concerned (and technically we could be ignoring the purported /
attempted reviewer).

So if a fetch job was created because of a comment, remember the
comment author and ping *them* instead of using the default ping
policy.

Fixes #981

runbot_merge/controllers/__init__.py

@@ -431,7 +431,9 @@ def _handle_comment(env, repo, issue, comment, target=None):
     if not repository.project_id._find_commands(comment['body'] or ''):
         return "No commands, ignoring"
 
-    pr = env['runbot_merge.pull_requests']._get_or_schedule(repo, issue, target=target)
+    pr = env['runbot_merge.pull_requests']._get_or_schedule(
+        repo, issue, target=target, commenter=comment['user']['login'],
+    )
     if not pr:
         return "Unknown PR, scheduling fetch"
 

runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv

@@ -16,7 +16,7 @@ runbot_merge.pr.load.unmanaged,"Branch `{pr[base][ref]}` is not within my remit,
 
 pr: pull request (github object)
 Repository: repository object (???)"
-runbot_merge.pr.load.fetched,"{pr.ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction).
+runbot_merge.pr.load.fetched,"{ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction).
 
 Pr: pr object we just created"
 runbot_merge.pr.branch.disabled,"{pr.ping}the target branch {pr.target.name!r} has been disabled, you may want to close this PR.","Notifies that the target branch for this PR was deactivated.

runbot_merge/models/pull_requests.py

@@ -107,7 +107,14 @@ 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, *, closing=False, squash=False):
+    def _load_pr(
+        self,
+        number: int,
+        *,
+        closing: bool = False,
+        squash: bool = False,
+        ping: str | None = None,
+    ):
         gh = self.github()
 
         # fetch PR object and handle as *opened*
@@ -238,7 +245,10 @@ All substitutions are tentatively applied sequentially to the input.
         self.env.ref('runbot_merge.pr.load.fetched')._send(
             repository=self,
             pull_request=number,
-            format_args={'pr': pr_id},
+            format_args={
+                'pr': pr_id,
+                'ping': ping or pr_id.ping,
+            },
         )
 
     def having_branch(self, branch):
@@ -635,7 +645,15 @@ class PullRequests(models.Model):
             return json.loads(self.overrides)
         return {}
 
-    def _get_or_schedule(self, repo_name, number, *, target=None, closing=False) -> PullRequests | None:
+    def _get_or_schedule(
+            self,
+            repo_name: str,
+            number: int,
+            *,
+            target: str | None = None,
+            closing: bool = False,
+            commenter: str | None = None,
+    ) -> PullRequests | None:
         repo = self.env['runbot_merge.repository'].search([('name', '=', repo_name)])
         if not repo:
             source = self.env['runbot_merge.events_sources'].search([('repository', '=', repo_name)])
@@ -682,6 +700,7 @@ class PullRequests(models.Model):
             'repository': repo.id,
             'number': number,
             'closing': closing,
+            'commenter': commenter,
         })
 
     def _iter_ancestors(self) -> Iterator[PullRequests]:
@@ -2518,6 +2537,7 @@ class FetchJob(models.Model):
     number = fields.Integer(required=True, group_operator=None)
     closing = fields.Boolean(default=False)
     commits_at = fields.Datetime(index="btree_not_null")
+    commenter = fields.Char()
 
     @api.model_create_multi
     def create(self, vals_list):
@@ -2545,7 +2565,12 @@ class FetchJob(models.Model):
             f.active = False
             self.env.cr.execute("SAVEPOINT runbot_merge_before_fetch")
             try:
-                f.repository._load_pr(f.number, closing=f.closing, squash=bool(f.commits_at))
+                f.repository._load_pr(
+                    f.number,
+                    closing=f.closing,
+                    squash=bool(f.commits_at),
+                    ping=f.commenter and f'@{f.commenter} ',
+                )
             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

@@ -3414,7 +3414,7 @@ class TestUnknownPR:
             (users['reviewer'], 'hansen r+'),
             (users['reviewer'], 'hansen r+'),
             seen(env, prx, users),
-            (users['user'], f"@{users['user']} I didn't know about this PR and had to "
+            (users['user'], f"@{users['reviewer']} I didn't know about this PR and had to "
                             "retrieve its information, you may have to "
                             "re-approve it as I didn't see previous commands."),
         ]
@@ -3470,7 +3470,7 @@ class TestUnknownPR:
             # reviewer is set because fetch replays all the comments (thus
             # setting r+ and reviewer) but then syncs the head commit thus
             # unsetting r+ but leaving the reviewer
-            (users['user'], f"@{users['user']} I didn't know about this PR and had to retrieve "
+            (users['user'], f"@{users['reviewer']} I didn't know about this PR and had to retrieve "
                             "its information, you may have to re-approve it "
                             "as I didn't see previous commands."),
         ]