From 3da18741963e80b34fd98055ecb6bc1483d09c35 Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Mon, 11 Jul 2022 14:00:35 +0200
Subject: [PATCH] [FIX] runbot_merge: correctly handle emptying PR body

The previous version of the code assumed `pr['body']` is always a
string, which is not correct, when the PR body is emptied the body
itself is removed (its value is `None`).

Add a case for this in the PR edition test, and avoid blowing up (or
adding empty newlines) when the PR body is empty. For PR creation this
issue was fixed in c2db5659d8d325939db66b07d0cb257e749bea82 but
apparently I missed that the exact same issue occurs just a few lines
above.

Also turns out github does *not* send change information when the body
is updated from (or to?) `None`, so don't even bother with that, just
check every time if the overall message has been updated.

Fixes #629
---
 conftest.py                                  | 22 ++++++++------------
 runbot_merge/changelog/2022-06/empty-body.md |  4 ++++
 runbot_merge/controllers/__init__.py         | 20 +++++++++++++-----
 runbot_merge/tests/test_basic.py             |  2 ++
 4 files changed, 30 insertions(+), 18 deletions(-)
 create mode 100644 runbot_merge/changelog/2022-06/empty-body.md

diff --git a/conftest.py b/conftest.py
index e800258c..e7cdeda2 100644
--- a/conftest.py
+++ b/conftest.py
@@ -826,6 +826,12 @@ mutation setDraft($pid: ID!) {
     }
 }
 '''
+def state_prop(name: str) -> property:
+    @property
+    def _prop(self):
+        return self._pr[name]
+    return _prop.setter(lambda self, v: self._set_prop(name, v))
+
 class PR:
     def __init__(self, repo, number):
         self.repo = repo
@@ -850,15 +856,9 @@ class PR:
             caching['If-Modified-Since']= r.headers['Last-Modified']
         return contents
 
-    @property
-    def title(self):
-        return self._pr['title']
-    title = title.setter(lambda self, v: self._set_prop('title', v))
-
-    @property
-    def base(self):
-        return self._pr['base']
-    base = base.setter(lambda self, v: self._set_prop('base', v))
+    title = state_prop('title')
+    body = state_prop('body')
+    base = state_prop('base')
 
     @property
     def draft(self):
@@ -888,10 +888,6 @@ class PR:
     def state(self):
         return self._pr['state']
 
-    @property
-    def body(self):
-        return self._pr['body']
-
     @property
     def comments(self):
         r = self.repo._session.get('https://api.github.com/repos/{}/issues/{}/comments'.format(self.repo.name, self.number))
diff --git a/runbot_merge/changelog/2022-06/empty-body.md b/runbot_merge/changelog/2022-06/empty-body.md
new file mode 100644
index 00000000..093962f5
--- /dev/null
+++ b/runbot_merge/changelog/2022-06/empty-body.md
@@ -0,0 +1,4 @@
+FIX: correctly handle PR empty PR descriptions
+
+Github's webhook for this case are weird, and weren't handled correctly,
+updating a PR's description to *or from* empty might be mishandled.
diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py
index ecee99f3..3b0b02c0 100644
--- a/runbot_merge/controllers/__init__.py
+++ b/runbot_merge/controllers/__init__.py
@@ -120,15 +120,25 @@ def handle_pr(env, event):
         if not source_branch:
             return handle_pr(env, dict(event, action='opened'))
 
+        pr_obj = find(source_branch)
         updates = {}
         if source_branch != branch:
-            updates['target'] = branch.id
-            updates['squash'] = pr['commits'] == 1
-        if event['changes'].keys() & {'title', 'body'}:
-            updates['message'] = "{}\n\n{}".format(pr['title'].strip(), pr['body'].strip())
+            if branch != pr_obj.target:
+                updates['target'] = branch.id
+                updates['squash'] = pr['commits'] == 1
+
+        # turns out github doesn't bother sending a change key if the body is
+        # changing from empty (None), therefore ignore that entirely, just
+        # generate the message and check if it changed
+        message = pr['title'].strip()
+        body = (pr['body'] or '').strip()
+        if body:
+            message += f"\n\n{body}"
+        if message != pr_obj.message:
+            updates['message'] = message
+
         _logger.info("update: %s#%d = %s (by %s)", repo.name, pr['number'], updates, event['sender']['login'])
         if updates:
-            pr_obj = find(source_branch)
             pr_obj.write(updates)
             return 'Updated {}'.format(pr_obj.id)
         return "Nothing to update ({})".format(event['changes'].keys())
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 1f254960..5049fcda 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -728,6 +728,8 @@ class TestPREdition:
         assert pr.message == 'title\n\nbody'
         with repo: prx.title = "title 2"
         assert pr.message == 'title 2\n\nbody'
+        with repo: prx.body = None
+        assert pr.message == "title 2"
         assert pr.staging_id, \
             "message edition does not affect staging of rebased PRs"
         with repo: prx.base = '1.0'