From f3a0a5c27c62426a69dd29f14af85fb9f1b75c1a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 21 Jun 2024 16:33:44 +0200 Subject: [PATCH] [FIX] runbot_merge: tracking message author on PullRequest events d4fa1fd35315d330566e37f515a937f722859ef7 added tracking to changes from *comments* (as well as a few hacks around authorship transfer), however it missed two things: First, it set the `change-author` during comments handling only, so changes from the `PullRequest` hook e.g. open, synchronise, close, edit, don't get attributed to their actual source, and instead just fall back to uid(1). This is easy enough to fix as the `sender` is always provided, that can be resolved to a partner which is then set as the author of whatever changes happen. Second, I actually missed one of the message hooks: there's both `_message_log` and `_message_log_batch` and they don't call one another, so both have to be overridden in order for tracking to be consistent. In this case, specifically, the *creation* of a tracked object goes through `_message_log_batch` (since that's a very generic message and so works on every tracked object created during the transaction... even though batch has a message per record anyway...) while *updates* go through `_message_log`. Fixes #895 --- conftest.py | 8 ++++++++ runbot_merge/controllers/__init__.py | 5 +++++ runbot_merge/models/pull_requests.py | 7 +++++++ runbot_merge/tests/test_basic.py | 19 ++++++++++++++----- 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/conftest.py b/conftest.py index 21af5274..28980399 100644 --- a/conftest.py +++ b/conftest.py @@ -160,6 +160,14 @@ def rolemap(request, config): @pytest.fixture def partners(env, config, rolemap): + """This specifically does not create partners for ``user`` and ``other`` + so they can be generated on-interaction, as "external" users. + + The two differ in that ``user`` has ownership of the org and can manage + repos there, ``other`` is completely unrelated to anything so useful to + check for interaction where the author only has read access to the reference + repositories. + """ m = {} for role, u in rolemap.items(): if role in ('user', 'other'): diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 86d65541..278b6609 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -254,6 +254,11 @@ def handle_pr(env, event): headers.get('X-Github-Delivery'), headers.get('User-Agent'), ) + sender = env['res.partner'].search([('github_login', '=', event['sender']['login'])], limit=1) + if not sender: + sender = env['res.partner'].create({'name': event['sender']['login'], 'github_login': event['sender']['login']}) + env.cr.precommit.data['change-author'] = sender.id + if event['action'] == 'opened': author_name = pr['user']['login'] author = env['res.partner'].search([('github_login', '=', author_name)], limit=1) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index d090eef4..9adb5ae0 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1049,6 +1049,13 @@ class PullRequests(models.Model): kw['body'] = html_escape(message) return super()._message_log(**kw) + def _message_log_batch(self, **kw): + if author := self.env.cr.precommit.data.get('change-author'): + kw['author_id'] = author + if message := self.env.cr.precommit.data.get('change-message'): + kw['bodies'] = {p.id: html_escape(message) for p in self} + return super()._message_log_batch(**kw) + def _pr_acl(self, user): if not self: return ACL(False, False, False) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 79ce42e2..d64596e6 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -65,6 +65,13 @@ def test_trivial_flow(env, repo, page, users, config): ) pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head='other') + [c2] = repo.make_commits( + 'other', + Commit('forgot a bit', tree={'whee': 'kjfdsh'}), + ref='heads/other', + make=False, + ) + pr_id = to_pr(env, pr) assert pr_id.state == 'opened' env.run_crons() @@ -79,12 +86,12 @@ def test_trivial_flow(env, repo, page, users, config): [e.text_content() for e in pr_dashboard.cssselect('dl.runbot-merge-fields dd')], )) == { 'label': f"{config['github']['owner']}:other", - 'head': c1, + 'head': c2, } with repo: - repo.post_status(c1, 'success', 'legal/cla') - repo.post_status(c1, 'success', 'ci/runbot') + repo.post_status(c2, 'success', 'legal/cla') + repo.post_status(c2, 'success', 'ci/runbot') env.run_crons() assert pr_id.state == 'validated' @@ -140,6 +147,7 @@ def test_trivial_flow(env, repo, page, users, config): assert repo.read_tree(master) == { 'a': 'some other content', 'b': 'a second file', + 'whee': 'kjfdsh', } assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1"\ "\n\nSigned-off-by: {reviewer.formatted_email}"\ @@ -156,8 +164,9 @@ def test_trivial_flow(env, repo, page, users, config): ]) assert list(messages) == [ - ('OdooBot', '

Pull Request created

', []), - ('OdooBot', f'

statuses changed on {c1}

', [('Opened', 'Validated')]), + (users['user'], '

Pull Request created

', []), + (users['user'], '', [(c1, c2)]), + ('OdooBot', f'

statuses changed on {c2}

', [('Opened', 'Validated')]), # reviewer approved changing the state and setting reviewer as reviewer # plus set merge method ('Reviewer', '', [