[IMP] runbot_merge: send comments asynchronously via feedback table

Hopefully this finally fixes the double commenting
issue (e.g. odoo/odoo#28160). This seems (according to reading the
logs and also logic) like a failure (concurrency of some sort) leading
to a transaction rollback *after* the GH API call, so the cron's DB
actions are rollbacked (then performed again on the next run) *but*
the GH API interactions are not rollbacked, and are thus performed
twice.

Since the entire purpose of the feedback table is to send comments,
send both "staging failed" and "Linked pull requests" comments via
that.

runbot_merge/models/pull_requests.py

@@ -758,14 +758,16 @@ class PullRequests(models.Model):
             unready = prs - ready
 
             for r in ready:
-                r.repository.github().comment(
+                self.env['runbot_merge.pull_requests.feedback'].create({
-                    r.number, "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format(
+                    'repository': r.repository.id,
+                    'pull_request': r.number,
+                    'message': "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format(
                         ', '.join(map(
                             '{0.repository.name}#{0.number}'.format,
                             unready
                         ))
                     )
-                )
+                })
                 r.link_warned = True
                 if commit:
                     self.env.cr.commit()
@@ -992,8 +994,11 @@ class Stagings(models.Model):
         prs = prs or self.batch_ids.prs
         prs.write({'state': 'error'})
         for pr in prs:
-            pr.repository.github().comment(
+            self.env['runbot_merge.pull_requests.feedback'].create({
-                pr.number, "Staging failed: %s" % message)
+                'repository': pr.repository.id,
+                'pull_request': pr.number,
+                'message':"Staging failed: %s" % message
+            })
 
         self.batch_ids.write({'active': False})
         self.write({

runbot_merge/tests/test_basic.py

@@ -10,7 +10,7 @@ from lxml import html
 
 import odoo
 
-from test_utils import re_matches
+from test_utils import re_matches, run_crons
 
 @pytest.fixture
 def repo(make_repo):
@@ -387,7 +387,7 @@ def test_staging_ci_failure_single(env, repo, users):
     staging_head = repo.commit('heads/staging.master')
     repo.post_status(staging_head.id, 'success', 'legal/cla')
     repo.post_status(staging_head.id, 'failure', 'ci/runbot') # stable genius
-    env['runbot_merge.project']._check_progress()
+    run_crons(env)
     assert env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
         ('number', '=', prx.number)

runbot_merge/tests/test_multirepo.py

@@ -9,7 +9,7 @@ import json
 
 import pytest
 
-from test_utils import re_matches
+from test_utils import re_matches, run_crons
 
 @pytest.fixture
 def repo_a(make_repo):
@@ -263,13 +263,12 @@ class TestCompanionsNotReady:
         assert pr_b.state == 'validated'
         assert pr_a.label == pr_b.label == '{}:do-a-thing'.format(owner)
 
-        env['runbot_merge.project']._check_progress()
+        run_crons(env)
 
         assert not pr_b.staging_id
         assert not pr_a.staging_id, \
             "pr_a should not have been staged as companion is not ready"
 
-        env['runbot_merge.pull_requests']._check_linked_prs_statuses()
         assert p_a.comments == [
             (users['reviewer'], 'hansen r+'),
             (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)),
@@ -296,7 +295,7 @@ class TestCompanionsNotReady:
         make_branch(repo_c, 'master', 'initial', {'f': 'c0'})
         pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing', reviewer=None)
 
-        env['runbot_merge.pull_requests']._check_linked_prs_statuses()
+        run_crons(env)
         assert pr_a.comments == []
         assert pr_b.comments == [
             (users['reviewer'], 'hansen r+'),
@@ -321,7 +320,7 @@ class TestCompanionsNotReady:
         make_branch(repo_c, 'master', 'initial', {'f': 'c0'})
         pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing')
 
-        env['runbot_merge.pull_requests']._check_linked_prs_statuses()
+        run_crons(env)
         assert pr_a.comments == []
         assert pr_b.comments == [
             (users['reviewer'], 'hansen r+'),
@@ -356,7 +355,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users):
     repo_a.post_status('heads/staging.master', 'success', 'ci/runbot', target_url="http://example.org/a")
     repo_b.post_status('heads/staging.master', 'success', 'legal/cla')
     repo_b.post_status('heads/staging.master', 'failure', 'ci/runbot', target_url="http://example.org/b")
-    env['runbot_merge.project']._check_progress()
+    run_crons(env)
 
     sth = repo_b.commit('heads/staging.master').id
     assert not pr.staging_id

runbot_merge/tests/test_utils.py

@@ -11,3 +11,9 @@ class re_matches:
 
     def __repr__(self):
         return '~' + self._r.pattern + '~'
+
+def run_crons(env):
+    "Helper to run all crons (in a relevant order) except for the fetch PR one"
+    env['runbot_merge.project']._check_progress()
+    env['runbot_merge.pull_requests']._check_linked_prs_statuses()
+    env['runbot_merge.project']._send_feedback()