[IMP] runbot_merge: send merge method warning faster, and on review

- Instead of warning about the merge method on ready PRs, also warn on
  *approved* (but exclude staged just cuz), as that's really when the
  user wants to know that they forgot to set the merge method
- The cron only triggers hourly, but *if* a user approves a PR *and*
  the merge method is not set yet, chances are good they'll need a
  reminder (if they `r+ rebase-merge` or w/e the cron will just ignore
  the PR and it's no skin off our back), so `_trigger` the cron for
  validation.
- Also do the same when skipchecks is set as it's very similar.

In reality we might want to hook off of the state transitioning to
reviewed but I'm not sure there's good ways to do that (triggering a
cron inside a compute doesn't seem like a good idea).

Update a pair of tests which would approve a multi-commit PR without
setting a merge method, just because the helper they use to build the
PR happens to create multiple commits.

Fix #891

runbot_merge/models/pull_requests.py

@@ -777,6 +777,9 @@ class PullRequests(models.Model):
                 case commands.SkipChecks() if is_admin:
                     self.batch_id.skipchecks = True
                     self.reviewed_by = author
+                    if not (self.squash or self.merge_method):
+                        self.env.ref('runbot_merge.check_linked_prs_status')._trigger()
+
                     for p in self.batch_id.prs - self:
                         if not p.reviewed_by:
                             p.reviewed_by = author
@@ -999,13 +1002,12 @@ class PullRequests(models.Model):
     def _approve(self, author, login):
         oldstate = self.state
         newstate = RPLUS.get(self.state)
-        msg = None
         if not author.email:
-            msg = "I must know your email before you can review PRs. Please contact an administrator."
+            return "I must know your email before you can review PRs. Please contact an administrator."
         elif not newstate:
-            msg = "this PR is already reviewed, reviewing it again is useless."
+            return "this PR is already reviewed, reviewing it again is useless."
-        else:
+
-            self.reviewed_by = author
+        self.reviewed_by = author
         _logger.debug(
             "r+ on %s by %s (%s->%s) status=%s message? %s",
             self.display_name, author.github_login,
@@ -1020,7 +1022,9 @@ class PullRequests(models.Model):
                 pull_request=self.number,
                 format_args={'user': login, 'pr': self},
             )
-        return msg
+        if not (self.squash or self.merge_method):
+            self.env.ref('runbot_merge.check_linked_prs_status')._trigger()
+        return None
 
     def message_post(self, **kw):
         if author := self.env.cr.precommit.data.get('change-author'):
@@ -1362,7 +1366,8 @@ class PullRequests(models.Model):
             if pair[0] != 'squash'
         )
         for r in self.search([
-            ('state', '=', 'ready'),
+            ('state', 'in', ("approved", "ready")),
+            ('staging_id', '=', False),
             ('squash', '=', False),
             ('merge_method', '=', False),
             ('method_warned', '=', False),

runbot_merge/tests/test_basic.py

@@ -1050,7 +1050,7 @@ def test_ci_failure_after_review(env, repo, users, config):
     """
     with repo:
         prx = _simple_init(repo)
-        prx.post_comment('hansen r+', config['role_reviewer']['token'])
+        prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
     env.run_crons()
 
     for ctx, url in [
@@ -1066,8 +1066,9 @@ def test_ci_failure_after_review(env, repo, users, config):
         env.run_crons()
 
     assert prx.comments == [
-        (users['reviewer'], 'hansen r+'),
+        (users['reviewer'], 'hansen r+ rebase-ff'),
         seen(env, prx, users),
+        (users['user'], "Merge method set to rebase and fast-forward."),
         (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
         (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
         (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
@@ -1302,14 +1303,15 @@ class TestRetry:
         """
         with repo:
             prx = _simple_init(repo)
-            prx.post_comment('hansen r+', config['role_reviewer']['token'])
+            prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token'])
             prx.post_comment('hansen retry', config['role_reviewer']['token'])
         env.run_crons()
 
         assert prx.comments == [
-            (users['reviewer'], 'hansen r+'),
+            (users['reviewer'], 'hansen r+ rebase-ff'),
             (users['reviewer'], 'hansen retry'),
             seen(env, prx, users),
+            (users['user'], "Merge method set to rebase and fast-forward."),
             (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)),
         ]