From 942570e60aefaad9b9914c199d33eff01a03e4fa Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 23 Jan 2025 15:48:14 +0100 Subject: [PATCH] [IMP] forwardport: outstanding FP reminders - only remind weekly initially (not daily) - root reminders on the forward port's creation, not the source's merge date - cap reminder interval at 4 weeks (instead of doubling every time) - track reminders on forward ports, don't share between siblings - remove `forwardport_updated_before` from the testing system, it's now possible to just update `reminder_next` to a past date and see if it gets triggered (it should to nothing on sources, or on forward port in a state which does not warrant concern) Fixes #801 --- conftest.py | 2 - forwardport/__manifest__.py | 2 +- .../changelog/2025-02/reminder-individual.md | 6 +++ .../changelog/2025-02/reminder-interval.md | 6 +++ .../migrations/17.0.1.5/pre-migrate.py | 27 ++++++++++ forwardport/models/project.py | 51 ++++++++----------- forwardport/tests/test_simple.py | 20 +++++--- forwardport/tests/test_weird.py | 6 ++- 8 files changed, 77 insertions(+), 43 deletions(-) create mode 100644 forwardport/changelog/2025-02/reminder-individual.md create mode 100644 forwardport/changelog/2025-02/reminder-interval.md create mode 100644 forwardport/migrations/17.0.1.5/pre-migrate.py diff --git a/conftest.py b/conftest.py index 1ce1ef06..11257f4c 100644 --- a/conftest.py +++ b/conftest.py @@ -391,9 +391,7 @@ class Base(models.AbstractModel): def run_crons(self): builtins.current_date = self.env.context.get('current_date') builtins.forwardport_merged_before = self.env.context.get('forwardport_merged_before') - builtins.forwardport_updated_before = self.env.context.get('forwardport_updated_before') self.env['ir.cron']._process_jobs(self.env.cr.dbname) - del builtins.forwardport_updated_before del builtins.forwardport_merged_before return True diff --git a/forwardport/__manifest__.py b/forwardport/__manifest__.py index cb24f840..66fa6fff 100644 --- a/forwardport/__manifest__.py +++ b/forwardport/__manifest__.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- { 'name': 'forward port bot', - 'version': '1.4', + 'version': '1.5', 'summary': "A port which forward ports successful PRs.", 'depends': ['runbot_merge'], 'data': [ diff --git a/forwardport/changelog/2025-02/reminder-individual.md b/forwardport/changelog/2025-02/reminder-individual.md new file mode 100644 index 00000000..cced3751 --- /dev/null +++ b/forwardport/changelog/2025-02/reminder-individual.md @@ -0,0 +1,6 @@ +IMP: reminder interval is now tracked by each forward port + +Previously it was tracked on the source, which made sense when the notification +was on the source, but not since the reminder is per-forward-port. However this +also means brand new forward ports start at 0 instead of wherever the reminder +source PR last got from their predecessors. diff --git a/forwardport/changelog/2025-02/reminder-interval.md b/forwardport/changelog/2025-02/reminder-interval.md new file mode 100644 index 00000000..38db8909 --- /dev/null +++ b/forwardport/changelog/2025-02/reminder-interval.md @@ -0,0 +1,6 @@ +IMP: the reminder interval has been capped at ~1 month, but increased in the initial phase + +Previously the reminder interval would start at 1 day for about a week, then +double every time, quickly increasing to less than yearly. + +The reminder interval is now weekly for the first month, then monthly. diff --git a/forwardport/migrations/17.0.1.5/pre-migrate.py b/forwardport/migrations/17.0.1.5/pre-migrate.py new file mode 100644 index 00000000..b47676d7 --- /dev/null +++ b/forwardport/migrations/17.0.1.5/pre-migrate.py @@ -0,0 +1,27 @@ +def migrate(cr, version): + """ + before: source PR has a `reminder_backoff_factor`, specifies the + power-of-two number of days between the source's merge date and the + next reminder(s) + after: forward ports have a `reminder_next` which is the date for sending + the next reminder, needs to be at least 7 days after the forward + port's creation, if more than that just send a reminder the next + time we can (?) + + We don't actually care about the source's anything (technically we could + e.g. if we just sent a reminder via the backoff factor then don't send a + new one but...) + """ + cr.execute(""" + ALTER TABLE runbot_merge_pull_requests + ADD COLUMN reminder_next varchar; + + UPDATE runbot_merge_pull_requests + SET reminder_next = greatest( + now(), + create_date::timestamp + interval '7 days' + ) + WHERE source_id IS NOT NULL + AND state IN ('opened', 'validated', 'approved', 'ready', 'error') + AND blocked IS NOT NULL; + """) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index fbc510d1..94f42426 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -36,7 +36,7 @@ from odoo.addons.runbot_merge.models.stagings_create import Message Conflict = tuple[str, str, str, list[str]] -DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3) +DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=7) _logger = logging.getLogger('odoo.addons.forwardport') @@ -183,7 +183,10 @@ class PullRequests(models.Model): merge_date: datetime.datetime parent_id: PullRequests - reminder_backoff_factor = fields.Integer(default=-4, group_operator=None) + reminder_next = fields.Datetime( + default=lambda self: self.env.cr.now() + datetime.timedelta(days=7), + index=True, + ) @api.model_create_multi def create(self, vals_list): @@ -518,41 +521,27 @@ stderr: # don't stringify so caller can still perform alterations return msg - 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 cutoff: a datetime (ISO-8601 formatted) - :returns: an iterator of (source, forward_ports) - """ - return groupby(self.env['runbot_merge.pull_requests'].search([ - # only FP PRs - ('source_id', '!=', False), - # active - ('state', 'not in', ['merged', 'closed']), - # not ready - ('blocked', '!=', False), - ('source_id.merge_date', '<', cutoff), - ], order='source_id, id'), lambda p: p.source_id) - def _reminder(self): - cutoff = getattr(builtins, 'forwardport_updated_before', None) \ - or fields.Datetime.to_string(datetime.datetime.now() - DEFAULT_DELTA) - cutoff_dt = fields.Datetime.from_string(cutoff) - - for source, prs in self._outstanding(cutoff): - backoff = dateutil.relativedelta.relativedelta(days=2**source.reminder_backoff_factor) - if source.merge_date > (cutoff_dt - backoff): - continue - source.reminder_backoff_factor += 1 - - # only keep the PRs which don't have an attached descendant + for _, prs in groupby(self.search([ + ('source_id', '!=', False), + ('blocked', '!=', False), + ('state', 'in', ['opened', 'validated', 'approved', 'ready', 'error']), + ('reminder_next', '<', self.env.cr.now()), + ], order='source_id, id'), lambda p: p.source_id): + # only remind on the "tip" of every chain of descendants as they + # will most likely lead to their parent being validated (?) for pr in set(prs).difference(p.parent_id for p in prs): + # reminder every 7 days for the first 4 weeks, then every 4 weeks + if (pr.reminder_next - pr.create_date) < datetime.timedelta(days=28): + pr.reminder_next += datetime.timedelta(days=7) + else: + pr.reminder_next += datetime.timedelta(days=28) + self.env.ref('runbot_merge.forwardport.reminder')._send( repository=pr.repository, pull_request=pr.number, token_field='fp_github_token', - format_args={'pr': pr, 'source': source}, + format_args={'pr': pr, 'source': pr.source_id}, ) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index b672526a..323faa76 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -120,9 +120,10 @@ def test_straightforward_flow(env, config, make_repo, users): prod.post_status(pr1.head, 'success', 'legal/cla') env.run_crons() - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) - pr0_, pr1_, pr2 = env['runbot_merge.pull_requests'].search([], order='number') + pr2.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') + assert pr.comments == [ (users['reviewer'], 'hansen r+ rebase-ff'), @@ -323,8 +324,10 @@ def test_empty(env, config, make_repo, users): assert project.fp_github_name == users['other'] # check reminder - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + fail_id.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') + fail_id.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') awaiting = ( users['other'], @@ -368,7 +371,8 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port # check that this stops if we close the PR with prod: fail_pr.close() - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + fail_id.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') assert pr1.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1, users), @@ -381,13 +385,15 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port with prod: prod.post_status(fail_id.head, 'success') assert fail_id.state == 'validated' - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + fail_id.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') assert fail_pr.comments[2:] == [awaiting]*3, "check that message triggers again" with prod: fail_pr.post_comment('hansen r+', config['role_reviewer']['token']) assert fail_id.state == 'ready' - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + fail_id.reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') assert fail_pr.comments[2:] == [ awaiting, awaiting, diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 66741500..17cb7bef 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -1209,7 +1209,8 @@ def test_reminder_detached(env, config, make_repo, users): pr_c = prod.get_pr(pr_c_id.number) # region sanity check - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + (pr_a_id | pr_b_id | pr_c_id).reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') assert pr_b.comments == [ seen(env, pr_b, users), @@ -1243,7 +1244,8 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port # region check detached pr_c_id.write({'parent_id': False, 'detach_reason': 'because'}) - env.run_crons('forwardport.reminder', context={'forwardport_updated_before': FAKE_PREV_WEEK}) + (pr_a_id | pr_b_id | pr_c_id).reminder_next = datetime.now() - timedelta(days=1) + env.run_crons('forwardport.reminder') 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." % (