From afe4d13eebb9ba2f33b74ae7bd0e0e08e79aa968 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Thu, 3 Nov 2022 11:46:31 +0100
Subject: [PATCH] [FIX] forwardport: fix pinging on forwardport PRs

- avoid pinging the author of the fw PR (which is the forward-bot
  itself)
- instead ping the author and reviewer of the source, and possibly the
  reviewer of the PR if any
- might also be a good idea to ping reviewers of intermediate PRs?
---
 forwardport/changelog/2022-10/notifications.md |  3 +++
 forwardport/models/project.py                  | 16 ++++++++++++++++
 forwardport/tests/test_simple.py               |  8 +++++++-
 forwardport/tests/test_updates.py              |  4 ++--
 4 files changed, 28 insertions(+), 3 deletions(-)
 create mode 100644 forwardport/changelog/2022-10/notifications.md

diff --git a/forwardport/changelog/2022-10/notifications.md b/forwardport/changelog/2022-10/notifications.md
new file mode 100644
index 00000000..87cc5cfe
--- /dev/null
+++ b/forwardport/changelog/2022-10/notifications.md
@@ -0,0 +1,3 @@
+FIX: stop pinging the forwardbot on forward-port PRs
+
+Also ping the reviewer of the original PR.
diff --git a/forwardport/models/project.py b/forwardport/models/project.py
index 7f9c95f9..7fe95a1f 100644
--- a/forwardport/models/project.py
+++ b/forwardport/models/project.py
@@ -1112,6 +1112,22 @@ stderr:
                 'token_field': 'fp_github_token',
             })
 
+    def ping(self, author=True, reviewer=True):
+        source = self.source_id
+        if not source:
+            return super().ping(author=author, reviewer=reviewer)
+
+        # use a dict literal to maintain ordering (3.6+)
+        pingline = ' '.join(
+            f'@{p.github_login}'
+            for p in filter(None, {
+                author and source.author: None,
+                reviewer and source.reviewed_by: None,
+                reviewer and self.reviewed_by: None,
+            })
+        )
+        return pingline and (pingline + ' ')
+
 class Stagings(models.Model):
     _inherit = 'runbot_merge.stagings'
 
diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py
index 172cee51..e0d07bfe 100644
--- a/forwardport/tests/test_simple.py
+++ b/forwardport/tests/test_simple.py
@@ -71,7 +71,7 @@ def test_straightforward_flow(env, config, make_repo, users):
     # should merge the staging then create the FP PR
     env.run_crons()
 
-    assert datetime.now() - datetime.strptime(pr_id.merge_date, FMT) <= timedelta(minutes=1),\
+    assert datetime.utcnow() - datetime.strptime(pr_id.merge_date, FMT) <= timedelta(minutes=1),\
         "check if merge date was set about now (within a minute as crons and " \
         "RPC calls yield various delays before we're back)"
 
@@ -108,6 +108,12 @@ def test_straightforward_flow(env, config, make_repo, users):
     # TODO: add original committer (if !author) as co-author in commit message?
     assert c.author['name'] == other_user['user'], "author should still be original's probably"
     assert c.committer['name'] == other_user['user'], "committer should also still be the original's, really"
+
+    assert pr1.ping() == "@%s @%s " % (
+        config['role_other']['user'],
+        config['role_reviewer']['user'],
+    ), "ping of forward-port PR should include author and reviewer of source"
+
     assert prod.read_tree(c) == {
         'f': 'c',
         'g': 'b',
diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py
index cc7de7c2..491ac240 100644
--- a/forwardport/tests/test_updates.py
+++ b/forwardport/tests/test_updates.py
@@ -386,7 +386,7 @@ conflict!
     # 2. "forward port chain" bit
     # 3. updated / modified & got detached
     assert pr2.comments[3:] == [
-        (users['user'], f"@{users['user']} WARNING: the latest change ({pr2_id.head}) triggered "
+        (users['user'], f"@{users['user']} @{users['reviewer']} WARNING: the latest change ({pr2_id.head}) triggered "
                         f"a conflict when updating the next forward-port "
                         f"({pr3_id.display_name}), and has been ignored.\n\n"
                         f"You will need to update this pull request "
@@ -398,7 +398,7 @@ conflict!
     # 2. forward-port chain thing
     assert repo.get_pr(pr3_id.number).comments[2:] == [
         (users['user'], re_matches(f'''\
-@{users['user']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
+@{users['user']} @{users['reviewer']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
 conflict in this pull request, data may have been lost.
 
 stdout: