From 64f9dcbc228e73f2d0acbf39c475db47672874ce Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 6 Sep 2024 13:04:13 +0200 Subject: [PATCH] [FIX] *: unnecessary warning on r- of forward port Reminding users that `r-` on a forward port only unreviews *that* forwardport is useful, as `r+;r-` is not a no-op (all preceding siblings are still reviewed). However it is useless if all siblings are not approved or already merged. So avoid sending the warning in that case. Fixes #934 --- forwardport/tests/test_simple.py | 8 ++++++++ runbot_merge/models/pull_requests.py | 8 +++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 0c4260c2..7ec2601c 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -629,6 +629,14 @@ def test_disapproval(env, config, make_repo, users): "sibling forward ports may have to be unapproved " "individually."), ] + with prod: + pr2.post_comment('hansen r-', token=config['role_other']['token']) + env.run_crons(None) + assert pr2_id.state == 'validated' + assert pr2.comments[2:] == [ + (users['other'], "hansen r+"), + (users['other'], "hansen r-"), + ], "shouldn't have a warning on r- because it's the only approved PR of the chain" def test_delegate_fw(env, config, make_repo, users): """If a user is delegated *on a forward port* they should be able to approve diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index cb7670e2..4bb2edd3 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -796,10 +796,12 @@ For your own safety I've ignored *everything in your entire comment*. pull_request=self.number, format_args={'user': login, 'pr': self}, ) - if self.source_id: + if self.source_id.forwardport_ids.filtered( + lambda p: p.reviewed_by and p.state not in ('merged', 'closed') + ): feedback("Note that only this forward-port has been" - " unapproved, sibling forward ports may " - "have to be unapproved individually.") + " unapproved, sibling forward ports may" + " have to be unapproved individually.") self.unstage("unreviewed (r-) by %s", login) else: msg = "r- makes no sense in the current PR state."