diff --git a/pgcommitfest/commitfest/migrations/0018_commitfest_preclosure_warning_sent_at.py b/pgcommitfest/commitfest/migrations/0018_commitfest_preclosure_warning_sent_at.py new file mode 100644 index 00000000..4c6ae1a5 --- /dev/null +++ b/pgcommitfest/commitfest/migrations/0018_commitfest_preclosure_warning_sent_at.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.19 on 2026-03-31 00:00 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("commitfest", "0017_make_topic_optional"), + ] + + operations = [ + migrations.AddField( + model_name="commitfest", + name="preclosure_warning_sent_at", + field=models.DateTimeField(blank=True, null=True), + ), + ] diff --git a/pgcommitfest/commitfest/models.py b/pgcommitfest/commitfest/models.py index 6594e3c4..98fdac07 100644 --- a/pgcommitfest/commitfest/models.py +++ b/pgcommitfest/commitfest/models.py @@ -4,7 +4,8 @@ from django.db.models import Q from django.shortcuts import get_object_or_404 -from datetime import datetime, timedelta, timezone +from datetime import date, datetime, timedelta, timezone +from typing import Literal from pgcommitfest.mailqueue.util import send_template_mail from pgcommitfest.userprofile.models import UserProfile @@ -57,6 +58,7 @@ class CommitFest(models.Model): startdate = models.DateField(blank=False, null=False) enddate = models.DateField(blank=False, null=False) draft = models.BooleanField(blank=False, null=False, default=False) + preclosure_warning_sent_at = models.DateTimeField(blank=True, null=True) @property def statusstring(self): @@ -101,6 +103,46 @@ def is_open_draft(self): def is_in_progress(self): return self.status == self.STATUS_INPROGRESS + @property + def preclosure_warning_date(self) -> date: + return self.enddate - timedelta(days=settings.PRE_CLOSURE_NOTIFICATION_DAYS) + + @staticmethod + def _current_date() -> date: + return datetime.now(timezone.utc).date() + + def days_until_close(self, current_date: date | None = None) -> int: + if current_date is None: + current_date = self._current_date() + return (self.enddate - current_date).days + + def should_send_preclosure_warning(self, current_date: date | None = None) -> bool: + if current_date is None: + current_date = self._current_date() + return ( + self.is_in_progress + and not self.preclosure_warning_sent_at + and self.preclosure_warning_date <= current_date <= self.enddate + ) + + def save(self, *args, **kwargs): + update_fields = kwargs.get("update_fields") + update_fields_set = set(update_fields) if update_fields is not None else None + + enddate_is_updating = update_fields_set is None or "enddate" in update_fields_set + if self.pk and enddate_is_updating: + # I don't love adding a query here, but it seems like the best way to get the old enddate. + old_enddate = ( + type(self).objects.filter(pk=self.pk).values_list("enddate", flat=True).first() + ) + if old_enddate is not None and old_enddate != self.enddate: + self.preclosure_warning_sent_at = None + if update_fields_set is not None: + update_fields_set.add("preclosure_warning_sent_at") + kwargs["update_fields"] = update_fields_set + + super().save(*args, **kwargs) + def to_json(self): return { "id": self.id, @@ -225,12 +267,117 @@ def send_closure_notifications(self): }, ) + @staticmethod + def _notification_email_for_user(user: User) -> str: + try: + if user.userprofile and user.userprofile.notifyemail: + return user.userprofile.notifyemail.email + except UserProfile.DoesNotExist: + pass + return user.email + + @staticmethod + def _wants_role_notification( + user: User, role: Literal["author", "reviewer", "committer"] + ) -> bool: + try: + profile = user.userprofile + except UserProfile.DoesNotExist: + return role == "author" + + if role == "author": + return profile.notify_all_author + if role == "reviewer": + return profile.notify_all_reviewer + if role == "committer": + return profile.notify_all_committer + + return False + + def send_preclosure_notifications(self, days_remaining: int | None = None) -> None: + """Send pre-closure reminder notifications to involved users.""" + if days_remaining is None: + days_remaining = self.days_until_close() + + if self.preclosure_warning_sent_at: + return + + open_pocs = list( + self.patchoncommitfest_set.filter( + status__in=PatchOnCommitFest.OPEN_STATUSES + ) + .select_related("patch", "patch__committer__user") + .prefetch_related( + "patch__authors", + "patch__reviewers", + "patch__subscribers", + ) + ) + if not open_pocs: + return + + recipients_patches = {} + for poc in open_pocs: + patch = poc.patch + recipients = set() + + for author in patch.authors.all(): + if self._wants_role_notification(author, "author"): + recipients.add(author) + + for reviewer in patch.reviewers.all(): + if self._wants_role_notification(reviewer, "reviewer"): + recipients.add(reviewer) + + if patch.committer and self._wants_role_notification( + patch.committer.user, "committer" + ): + recipients.add(patch.committer.user) + + recipients.update(patch.subscribers.all()) + + for recipient in recipients: + if recipient.id not in recipients_patches: + recipients_patches[recipient.id] = { + "user": recipient, + "patches": {}, + } + recipients_patches[recipient.id]["patches"][patch.id] = poc + + if not recipients_patches: + return + + for payload in recipients_patches.values(): + user = payload["user"] + patches = list(payload["patches"].values()) + send_template_mail( + settings.NOTIFICATION_FROM, + None, + self._notification_email_for_user(user), + ( + f"Commitfest {self.name} closes in {days_remaining} days " + "and your patches need attention" + ), + "mail/commitfest_preclosure_warning.txt", + { + "user": user, + "commitfest": self, + "patches": patches, + "days_remaining": days_remaining, + }, + ) + + self.preclosure_warning_sent_at = datetime.now(timezone.utc) + self.save(update_fields=["preclosure_warning_sent_at"]) + @staticmethod def _are_relevant_commitfests_up_to_date(cfs, current_date): inprogress_cf = cfs["in_progress"] if inprogress_cf and inprogress_cf.enddate < current_date: return False + if inprogress_cf and inprogress_cf.should_send_preclosure_warning(): + return False if cfs["open"].startdate <= current_date: return False @@ -243,7 +390,7 @@ def _are_relevant_commitfests_up_to_date(cfs, current_date): @classmethod def _refresh_relevant_commitfests(cls, for_update): cfs = CommitFest.relevant_commitfests(for_update=for_update, refresh=False) - current_date = datetime.now(timezone.utc).date() + current_date = cls._current_date() if cls._are_relevant_commitfests_up_to_date(cfs, current_date): return cfs @@ -261,6 +408,10 @@ def _refresh_relevant_commitfests(cls, for_update): inprogress_cf.save() inprogress_cf.auto_move_active_patches() inprogress_cf.send_closure_notifications() + elif inprogress_cf and inprogress_cf.should_send_preclosure_warning(): + inprogress_cf.send_preclosure_notifications( + days_remaining=inprogress_cf.days_until_close() + ) open_cf = cfs["open"] diff --git a/pgcommitfest/commitfest/templates/mail/commitfest_preclosure_warning.txt b/pgcommitfest/commitfest/templates/mail/commitfest_preclosure_warning.txt new file mode 100644 index 00000000..e64a5325 --- /dev/null +++ b/pgcommitfest/commitfest/templates/mail/commitfest_preclosure_warning.txt @@ -0,0 +1,19 @@ +Commitfest {{commitfest.name}} will auto-close in {{days_remaining}} days. + +You are involved in {{patches|length}} open patch{{patches|length|pluralize:"es"}}: + +{% for poc in patches %} + - {{poc.patch.name}} + https://commitfest.postgresql.org/patch/{{poc.patch.id}}/ +{% endfor %} + +Please take action on {{patches|length|pluralize:"this patch,these patches"}} before close: + +1. Move {{patches|length|pluralize:"it,them"}} to the next commitfest if work should continue. + +2. Or close {{patches|length|pluralize:"it,them"}} with an appropriate status (Withdrawn, Returned with feedback, etc.) + +-- +This is an automated message from the PostgreSQL Commitfest app. +To manage your notification preferences, visit: +https://commitfest.postgresql.org/userprofile/ diff --git a/pgcommitfest/commitfest/tests/test_closure_notifications.py b/pgcommitfest/commitfest/tests/test_closure_notifications.py index eb7cfc7a..ef411164 100644 --- a/pgcommitfest/commitfest/tests/test_closure_notifications.py +++ b/pgcommitfest/commitfest/tests/test_closure_notifications.py @@ -8,6 +8,7 @@ from pgcommitfest.commitfest.models import ( CfbotBranch, + Committer, CommitFest, Patch, PatchHistory, @@ -441,3 +442,54 @@ def test_draft_cf_moves_active_patches_to_next_draft(alice, bob): # No closure email for moved patches assert QueuedMail.objects.count() == 0 + + +def test_send_preclosure_notifications_to_all_involved_roles( + alice, bob, charlie, dave, in_progress_cf +): + """Pre-closure reminder should notify involved users based on role preferences.""" + UserProfile.objects.create(user=bob, notify_all_reviewer=True) + UserProfile.objects.create(user=charlie, notify_all_committer=True) + committer = Committer.objects.create(user=charlie) + + patch = Patch.objects.create(name="Preclose Patch", committer=committer) + patch.authors.add(alice) + patch.reviewers.add(bob) + patch.subscribers.add(dave) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=in_progress_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + + in_progress_cf.send_preclosure_notifications( + days_remaining=settings.PRE_CLOSURE_NOTIFICATION_DAYS + ) + + assert QueuedMail.objects.count() == 4 + receivers = set(QueuedMail.objects.values_list("receiver", flat=True)) + assert receivers == {alice.email, bob.email, charlie.email, dave.email} + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is not None + + +def test_preclosure_notifications_are_sent_only_once(alice, in_progress_cf): + """Pre-closure reminder should be idempotent for a commitfest.""" + patch = Patch.objects.create(name="Single Reminder Patch") + patch.authors.add(alice) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=in_progress_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + + in_progress_cf.send_preclosure_notifications( + days_remaining=settings.PRE_CLOSURE_NOTIFICATION_DAYS + ) + in_progress_cf.send_preclosure_notifications( + days_remaining=settings.PRE_CLOSURE_NOTIFICATION_DAYS + ) + + assert QueuedMail.objects.count() == 1 diff --git a/pgcommitfest/commitfest/tests/test_refresh_commitfests.py b/pgcommitfest/commitfest/tests/test_refresh_commitfests.py index dda1fc9d..aae4b07d 100644 --- a/pgcommitfest/commitfest/tests/test_refresh_commitfests.py +++ b/pgcommitfest/commitfest/tests/test_refresh_commitfests.py @@ -8,6 +8,7 @@ Patch, PatchOnCommitFest, ) +from pgcommitfest.mailqueue.models import QueuedMail pytestmark = pytest.mark.django_db @@ -156,3 +157,89 @@ def test_no_changes_when_up_to_date(commitfests): assert open_cf.status == CommitFest.STATUS_OPEN assert draft_cf.status == CommitFest.STATUS_OPEN assert CommitFest.objects.count() == initial_cf_count + + +@freeze_time("2024-11-23") +def test_preclosure_notifications_trigger_7_days_before_close(commitfests, alice): + """In-progress CF should send pre-closure notifications exactly once.""" + in_progress_cf = commitfests["in_progress"] + patch = Patch.objects.create(name="Reminder Patch") + patch.authors.add(alice) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=in_progress_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + + CommitFest._refresh_relevant_commitfests(for_update=False) + CommitFest._refresh_relevant_commitfests(for_update=False) + + assert QueuedMail.objects.count() == 1 + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is not None + + +@freeze_time("2024-11-24") +def test_preclosure_notifications_trigger_on_first_check_inside_window( + commitfests, alice +): + """The reminder should still be sent if the site is first hit after the window opens.""" + in_progress_cf = commitfests["in_progress"] + patch = Patch.objects.create(name="Late Reminder Patch") + patch.authors.add(alice) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=in_progress_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + + CommitFest._refresh_relevant_commitfests(for_update=False) + + assert QueuedMail.objects.count() == 1 + mail = QueuedMail.objects.get() + assert "closes in 6 days" in mail.fullmsg + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is not None + + +def test_changing_enddate_clears_preclosure_warning_and_allows_resend( + commitfests, alice +): + """Moving the close date should clear the reminder timestamp and reopen the flow.""" + in_progress_cf = commitfests["in_progress"] + patch = Patch.objects.create(name="Rescheduled Reminder Patch") + patch.authors.add(alice) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=in_progress_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + + with freeze_time("2024-11-24"): + CommitFest._refresh_relevant_commitfests(for_update=False) + + assert QueuedMail.objects.count() == 1 + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is not None + + in_progress_cf.enddate = datetime(2024, 12, 7).date() + in_progress_cf.save(update_fields=["enddate"]) + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is None + + with freeze_time("2024-11-24"): + CommitFest._refresh_relevant_commitfests(for_update=False) + + assert QueuedMail.objects.count() == 1 + + with freeze_time("2024-11-30"): + CommitFest._refresh_relevant_commitfests(for_update=False) + + assert QueuedMail.objects.count() == 2 + mail = QueuedMail.objects.order_by("-id").first() + assert "closes in 7 days" in mail.fullmsg + in_progress_cf.refresh_from_db() + assert in_progress_cf.preclosure_warning_sent_at is not None diff --git a/pgcommitfest/settings.py b/pgcommitfest/settings.py index 07e4adaf..dffad9d8 100644 --- a/pgcommitfest/settings.py +++ b/pgcommitfest/settings.py @@ -173,6 +173,8 @@ AUTO_MOVE_EMAIL_ACTIVITY_DAYS = 30 # Patches failing CI for longer than this many days will NOT be auto-moved AUTO_MOVE_MAX_FAILING_DAYS = 21 +# Send pre-closure reminders this many days before an in-progress CF ends +PRE_CLOSURE_NOTIFICATION_DAYS = 7 # Load local settings overrides try: