From c67325fdabb4ec4b090df43be0ab96108ba40180 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 10 Jun 2024 18:47:49 +0200 Subject: [PATCH] [IMP] forwardport: don't ping *every* forwardport 98aaa910 updated the forwardport notifications system to notify on the forward ports rather than the source, to try and mitigate or at least shift some of the spam: spam the followers of the original source (which might be many people) somewhat less, at the possible cost of spamming the author and reviewer more because they get a message per forgotten forward port. This change aims to alleviate part of the latter, by only notifying on PRs which actually need to be r+'d, and not notifying on those which will implicitly "inherit" the reviews. This should cut down on redundant notifications and let users focus on the important ones. --- forwardport/models/project.py | 17 +++-- forwardport/tests/test_simple.py | 8 +-- forwardport/tests/test_weird.py | 103 ++++++++++++++++++++++++++++++- 3 files changed, 116 insertions(+), 12 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 1ce6abb7..892c2d58 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -11,13 +11,14 @@ means PR creation is trickier (as mergebot assumes opened event will always lead to PR creation but fpbot wants to attach meaning to the PR when setting it up), ... """ +from __future__ import annotations + import datetime import itertools import json import logging import operator import subprocess -import sys import tempfile import typing from pathlib import Path @@ -181,6 +182,8 @@ class PullRequests(models.Model): reviewed_by: Partner head: str state: str + merge_date: datetime.datetime + parent_id: PullRequests reminder_backoff_factor = fields.Integer(default=-4, group_operator=None) @@ -529,11 +532,11 @@ stderr: # don't stringify so caller can still perform alterations return msg - def _outstanding(self, cutoff): + def _outstanding(self, cutoff: str) -> typing.ItemsView[PullRequests, list[PullRequests]]: """ Returns "outstanding" (unmerged and unclosed) forward-ports whose source was merged before ``cutoff`` (all of them if not provided). - :param str cutoff: a datetime (ISO-8601 formatted) + :param cutoff: a datetime (ISO-8601 formatted) :returns: an iterator of (source, forward_ports) """ return groupby(self.env['runbot_merge.pull_requests'].search([ @@ -551,11 +554,17 @@ stderr: for source, prs in self._outstanding(cutoff): backoff = dateutil.relativedelta.relativedelta(days=2**source.reminder_backoff_factor) - prs = list(prs) if source.merge_date > (cutoff_dt - backoff): continue source.reminder_backoff_factor += 1 + + # only keep the PRs which don't have an attached descendant) + pr_ids = {p.id for p in prs} for pr in prs: + pr_ids.discard(pr.parent_id.id) + print(source, prs, [p.parent_id for p in prs], + '\n\t->', pr_ids, flush=True) + for pr in (p for p in prs if p.id in pr_ids): self.env.ref('runbot_merge.forwardport.reminder')._send( repository=pr.repository, pull_request=pr.number, diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index eae53ce4..b17f83d8 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -139,13 +139,7 @@ def test_straightforward_flow(env, config, make_repo, users): This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -"""), - (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( - users['other'], - users['reviewer'], - pr0.display_name, - )) - ] +""")] assert pr0_ == pr0 assert pr1_ == pr1 diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index e0922ceb..8bc62aad 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -from datetime import datetime +from datetime import datetime, timedelta import pytest @@ -1164,3 +1164,104 @@ def test_maintain_batch_history(env, config, make_repo, users): assert pr1_b_id in b_batch.all_prs, "the PR is still in the batch" assert pr1_b_id not in b_batch.prs, "the PR is not in the open/active batch PRs" # endregion + +FMT = '%Y-%m-%d %H:%M:%S' +FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT) +def test_reminder_detached(env, config, make_repo, users): + """On detached forward ports, both sides of the detachment should be notified. + """ + # region setup + prod, _ = make_basic(env, config, make_repo, statuses='default') + with prod: + prod.make_commits('a', Commit('c', tree={'x': '0'}), ref="heads/abranch") + pr_a = prod.make_pr(target='a', head='abranch') + prod.post_status('abranch', 'success') + pr_a.post_comment('hansen r+ fw=skipci', config['role_reviewer']['token']) + env.run_crons() + + with prod: + prod.post_status('staging.a', 'success') + env.run_crons() + + pr_a_id = to_pr(env, pr_a) + pr_b_id = env['runbot_merge.pull_requests'].search([ + ('target.name', '=', 'b'), + ('parent_id', '=', pr_a_id.id), + ]) + assert pr_b_id + with prod: + prod.post_status(pr_b_id.head, 'success') + env.run_crons() + pr_c_id = env['runbot_merge.pull_requests'].search([ + ('target.name', '=', 'c'), + ('parent_id', '=', pr_b_id.id), + ]) + assert pr_c_id + # endregion + + pr_b = prod.get_pr(pr_b_id.number) + pr_c = prod.get_pr(pr_c_id.number) + + # region sanity check + env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + + assert pr_b.comments == [ + seen(env, pr_b, users), + (users['user'], """\ +This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""")], "the intermediate PR should not be reminded" + + assert pr_c.comments == [ + seen(env, pr_c, users), + (users['user'], """\ +@%s @%s this PR targets c and is the last of the forward-port chain containing: +* %s + +To merge the full chain, use +> @hansen r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""" % ( + users['user'], users['reviewer'], + pr_b_id.display_name, + )), + (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( + users['user'], + users['reviewer'], + pr_a_id.display_name, + )) + ], "the final PR should be reminded" + # endregion + + # region check detached + pr_c_id.write({'parent_id': False, 'detach_reason': 'because'}) + env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + + for p in env['runbot_merge.pull_requests'].search_read( + [], ['id', 'parent_id', 'source_id', 'display_name'] + ): + print(p, flush=True) + + assert pr_b.comments[2:] == [ + (users['user'], "@%s @%s child PR %s was modified / updated and has become a normal PR. This PR (and any of its parents) will need to be merged independently as approvals won't cross." % ( + users['user'], + users['reviewer'], + pr_c_id.display_name, + )), + (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( + users['user'], + users['reviewer'], + pr_a_id.display_name, + )) + ], "the detached-from intermediate PR should now be reminded" + assert pr_c.comments[3:] == [ + (users['user'], "@%(user)s @%(reviewer)s this PR was modified / updated and has become a normal PR. It must be merged directly." % users), + (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( + users['user'], + users['reviewer'], + pr_a_id.display_name, + )) + ], "the final forward port should be reminded as before" + # endregion