Skip to content

Commit 208d89b

Browse files
authored
More friendly closing of commitfests (#101)
When a commitfest is closed authors currently need to move their patches. This is an attempt to "age out" patches without interest automatically. This has a few downsides though. For patches that have activity going on, this is just busywork for the author. And currently it's not even very clear that authors are expected to do this (i.e. it's only shown in the dashboard). So this PR tries to improve the situation in three ways: 1. Automatically move "active" patches to the next commitfest 2. Send email notifications to authors for patches that were not moved automatically 3. Show a warning on the patch page
1 parent bc839b6 commit 208d89b

File tree

12 files changed

+822
-7
lines changed

12 files changed

+822
-7
lines changed

pgcommitfest/commitfest/fixtures/commitfest_data.json

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1351,5 +1351,63 @@
13511351
"created": "2025-01-26T22:11:40.000",
13521352
"modified": "2025-01-26T22:12:00.000"
13531353
}
1354+
},
1355+
{
1356+
"model": "commitfest.patch",
1357+
"pk": 9,
1358+
"fields": {
1359+
"name": "Abandoned patch in closed commitfest",
1360+
"topic": 3,
1361+
"wikilink": "",
1362+
"gitlink": "",
1363+
"targetversion": null,
1364+
"committer": null,
1365+
"created": "2024-10-15T10:00:00",
1366+
"modified": "2024-10-15T10:00:00",
1367+
"lastmail": "2024-10-01T10:00:00",
1368+
"tags": [],
1369+
"authors": [],
1370+
"reviewers": [],
1371+
"subscribers": [],
1372+
"mailthread_set": [
1373+
9
1374+
]
1375+
}
1376+
},
1377+
{
1378+
"model": "commitfest.mailthread",
1379+
"pk": 9,
1380+
"fields": {
1381+
"messageid": "abandoned@message-09",
1382+
"subject": "Abandoned patch in closed commitfest",
1383+
"firstmessage": "2024-10-01T10:00:00",
1384+
"firstauthor": "test@test.com",
1385+
"latestmessage": "2024-10-01T10:00:00",
1386+
"latestauthor": "test@test.com",
1387+
"latestsubject": "Abandoned patch in closed commitfest",
1388+
"latestmsgid": "abandoned@message-09"
1389+
}
1390+
},
1391+
{
1392+
"model": "commitfest.patchoncommitfest",
1393+
"pk": 10,
1394+
"fields": {
1395+
"patch": 9,
1396+
"commitfest": 1,
1397+
"enterdate": "2024-10-15T10:00:00",
1398+
"leavedate": null,
1399+
"status": 1
1400+
}
1401+
},
1402+
{
1403+
"model": "commitfest.patchhistory",
1404+
"pk": 23,
1405+
"fields": {
1406+
"patch": 9,
1407+
"date": "2024-10-15T10:00:00",
1408+
"by": 1,
1409+
"by_cfbot": false,
1410+
"what": "Created patch record"
1411+
}
13541412
}
13551413
]

pgcommitfest/commitfest/models.py

Lines changed: 129 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
from datetime import datetime, timedelta, timezone
88

9+
from pgcommitfest.mailqueue.util import send_template_mail
910
from pgcommitfest.userprofile.models import UserProfile
1011

1112
from .util import DiffableModel
@@ -109,6 +110,119 @@ def to_json(self):
109110
"enddate": self.enddate.isoformat(),
110111
}
111112

113+
def _should_auto_move_patch(self, patch, current_date):
114+
"""Determine if a patch should be automatically moved to the next commitfest.
115+
116+
A patch qualifies for auto-move if it both:
117+
1. Has had email activity within the configured number of days
118+
2. Hasn't been failing CI for longer than the configured threshold
119+
"""
120+
activity_cutoff = current_date - timedelta(
121+
days=settings.AUTO_MOVE_EMAIL_ACTIVITY_DAYS
122+
)
123+
failing_cutoff = current_date - timedelta(
124+
days=settings.AUTO_MOVE_MAX_FAILING_DAYS
125+
)
126+
127+
# Check for recent email activity
128+
if not patch.lastmail or patch.lastmail < activity_cutoff:
129+
return False
130+
131+
# Check if CI has been failing too long
132+
try:
133+
cfbot_branch = patch.cfbot_branch
134+
if (
135+
cfbot_branch.failing_since
136+
and cfbot_branch.failing_since < failing_cutoff
137+
):
138+
return False
139+
except CfbotBranch.DoesNotExist:
140+
# IF no CFBot data exists, the patch is probably very new (i.e. no
141+
# CI run has ever taken place for it yet). So we auto-move it in
142+
# that case.
143+
pass
144+
145+
return True
146+
147+
def auto_move_active_patches(self):
148+
"""Automatically move active patches to the next commitfest.
149+
150+
A patch is moved if it has recent email activity and hasn't been
151+
failing CI for too long.
152+
"""
153+
current_date = datetime.now()
154+
155+
# Get the next open commitfest (must exist, raises IndexError otherwise)
156+
# For draft CFs, find the next draft CF
157+
# For regular CFs, find the next regular CF by start date
158+
if self.draft:
159+
next_cf = CommitFest.objects.filter(
160+
status=CommitFest.STATUS_OPEN,
161+
draft=True,
162+
startdate__gt=self.enddate,
163+
).order_by("startdate")[0]
164+
else:
165+
next_cf = CommitFest.objects.filter(
166+
status=CommitFest.STATUS_OPEN,
167+
draft=False,
168+
startdate__gt=self.enddate,
169+
).order_by("startdate")[0]
170+
171+
# Get all patches with open status in this commitfest
172+
open_pocs = self.patchoncommitfest_set.filter(
173+
status__in=PatchOnCommitFest.OPEN_STATUSES
174+
).select_related("patch")
175+
176+
for poc in open_pocs:
177+
if self._should_auto_move_patch(poc.patch, current_date):
178+
poc.patch.move(self, next_cf, by_user=None, by_cfbot=True)
179+
180+
def send_closure_notifications(self):
181+
"""Send email notifications to authors of patches that are still open."""
182+
# Get patches that still need action (not moved, not closed)
183+
open_pocs = list(
184+
self.patchoncommitfest_set.filter(
185+
status__in=PatchOnCommitFest.OPEN_STATUSES
186+
)
187+
.select_related("patch")
188+
.prefetch_related("patch__authors")
189+
)
190+
191+
if not open_pocs:
192+
return
193+
194+
# Collect unique authors and their patches
195+
authors_patches = {}
196+
for poc in open_pocs:
197+
for author in poc.patch.authors.all():
198+
if author not in authors_patches:
199+
authors_patches[author] = []
200+
authors_patches[author].append(poc)
201+
202+
# Send email to each author who has notifications enabled
203+
for author, patches in authors_patches.items():
204+
try:
205+
if not author.userprofile.notify_all_author:
206+
continue
207+
notifyemail = author.userprofile.notifyemail
208+
except UserProfile.DoesNotExist:
209+
continue
210+
211+
email = notifyemail.email if notifyemail else author.email
212+
213+
send_template_mail(
214+
settings.NOTIFICATION_FROM,
215+
None,
216+
email,
217+
f"Commitfest {self.name} has closed and you have unmoved patches",
218+
"mail/commitfest_closure.txt",
219+
{
220+
"user": author,
221+
"commitfest": self,
222+
"patches": patches,
223+
},
224+
)
225+
112226
@staticmethod
113227
def _are_relevant_commitfests_up_to_date(cfs, current_date):
114228
inprogress_cf = cfs["in_progress"]
@@ -143,26 +257,33 @@ def _refresh_relevant_commitfests(cls, for_update):
143257
if inprogress_cf and inprogress_cf.enddate < current_date:
144258
inprogress_cf.status = CommitFest.STATUS_CLOSED
145259
inprogress_cf.save()
260+
inprogress_cf.auto_move_active_patches()
261+
inprogress_cf.send_closure_notifications()
146262

147263
open_cf = cfs["open"]
148264

149265
if open_cf.startdate <= current_date:
150266
if open_cf.enddate < current_date:
151267
open_cf.status = CommitFest.STATUS_CLOSED
268+
open_cf.save()
269+
cls.next_open_cf(current_date).save()
270+
open_cf.auto_move_active_patches()
271+
open_cf.send_closure_notifications()
152272
else:
153273
open_cf.status = CommitFest.STATUS_INPROGRESS
154-
open_cf.save()
155-
156-
cls.next_open_cf(current_date).save()
274+
open_cf.save()
275+
cls.next_open_cf(current_date).save()
157276

158277
draft_cf = cfs["draft"]
159278
if not draft_cf:
160279
cls.next_draft_cf(current_date).save()
161280
elif draft_cf.enddate < current_date:
162-
# If the draft commitfest has started, we need to update it
281+
# Create next CF first so auto_move has somewhere to move patches
163282
draft_cf.status = CommitFest.STATUS_CLOSED
164283
draft_cf.save()
165284
cls.next_draft_cf(current_date).save()
285+
draft_cf.auto_move_active_patches()
286+
draft_cf.send_closure_notifications()
166287

167288
return cls.relevant_commitfests(for_update=for_update)
168289

@@ -456,7 +577,9 @@ def update_lastmail(self):
456577
else:
457578
self.lastmail = max(threads, key=lambda t: t.latestmessage).latestmessage
458579

459-
def move(self, from_cf, to_cf, by_user, allow_move_to_in_progress=False):
580+
def move(
581+
self, from_cf, to_cf, by_user, allow_move_to_in_progress=False, by_cfbot=False
582+
):
460583
"""Returns the new PatchOnCommitFest object, or raises UserInputError"""
461584

462585
current_poc = self.current_patch_on_commitfest()
@@ -501,6 +624,7 @@ def move(self, from_cf, to_cf, by_user, allow_move_to_in_progress=False):
501624
PatchHistory(
502625
patch=self,
503626
by=by_user,
627+
by_cfbot=by_cfbot,
504628
what=f"Moved from CF {from_cf} to CF {to_cf}",
505629
).save_and_notify()
506630

pgcommitfest/commitfest/templates/commitfest.html

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22
{%load commitfest %}
33
{%block contents%}
44

5+
{%if cf.is_closed%}
6+
<div class="alert alert-warning" role="alert">
7+
<strong>This commitfest is closed.</strong> Open patches in this commitfest are no longer being picked up by CI. If you want CI to run on your patch, move it to an open commitfest.
8+
</div>
9+
{%endif%}
10+
511
<button type="button" class="btn btn-secondary active" id="filterButton" onClick="togglePatchFilterButton('filterButton', 'collapseFilters')">Search/filter</button>
612
<a class="btn btn-secondary{% if request.GET.reviewer == '-2' %} active{% endif %}" href="?reviewer=-2">No reviewers</a>
713
<a class="btn btn-secondary{% if request.GET.author == '-3' %} active{% endif %}" href="?author=-3">My patches</a>

pgcommitfest/commitfest/templates/help.html

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ <h2>Commitfest</h2>
1515
</p>
1616
<p>There are 5 Commitfests per year. The first one is "In Progress" in <em>July</em> and starts the nine months feature development cycle of PostgreSQL. The next three are "In Progress" in <em>September</em>, <em>November</em> and <em>January</em>. The last Commitfest of the feature development cycle is "In Progress" in <em>March</em>, and ends a when the feature freeze starts. The exact date of the feature freeze depends on the year, but it's usually in early April.</p>
1717

18+
<h3>Commitfest closure</h3>
19+
<p>
20+
When a Commitfest closes, patches that have been active recently are automatically moved to the next Commitfest. A patch is considered "active" if it has had email activity in the past {{auto_move_email_activity_days}} days and has not been failing CI for more than {{auto_move_max_failing_days}} days. Patches that are not automatically moved will stay in the closed Commitfest, where they will no longer be picked up by CI. Authors of such patches that have enabled "Notify on all where author" in their profile settings will receive an email notification asking them to either move the patch to the next Commitfest or close it with an appropriate status.
21+
</p>
22+
1823
<h2>Patches</h2>
1924
<p>
2025
A "patch" is a bit of an overloaded term in the PostgreSQL community. Email threads on the mailing list often contain "patch files" as attachments, such a file is often referred to as a "patch". A single email can even contain multiple related "patch files", which are called a "patchset". However, in the context of a Commitfest app a "patch" usually means a "patch entry" in the Commitfest app. Such a "patch entry" is a reference to a mailinglist thread on which change to PostgreSQL has been proposed, by someone sending an email that contain one or more "patch files". The Commitfest app will automatically detect new versions of the patch files and update the "patch entry" accordingly.
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
Commitfest {{commitfest.name}} has now closed.
2+
3+
You have {{patches|length}} open patch{{patches|length|pluralize:"es"}} that need{{patches|length|pluralize:"s,"}} attention:
4+
5+
{% for poc in patches %}
6+
- {{poc.patch.name}}
7+
https://commitfest.postgresql.org/patch/{{poc.patch.id}}/
8+
{% endfor %}
9+
10+
Please take action on {{patches|length|pluralize:"these patches,this patch"}} by doing either of the following:
11+
12+
1. If you want to continue working on {{patches|length|pluralize:"them,it"}}, move {{patches|length|pluralize:"them,it"}} to the next commitfest.
13+
14+
2. If you no longer wish to pursue {{patches|length|pluralize:"these patches,this patch"}}, please close {{patches|length|pluralize:"them,it"}} with an appropriate status (Withdrawn, Returned with feedback, etc.)

pgcommitfest/commitfest/templates/patch.html

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,13 @@
11
{%extends "base.html"%}
22
{%load commitfest%}
33
{%block contents%}
4+
{%with current_poc=patch_commitfests.0%}
5+
{%if cf.is_closed and not current_poc.is_closed%}
6+
<div class="alert alert-warning" role="alert">
7+
<strong>This patch is part of a closed commitfest.</strong> It is no longer being picked up by CI. If you want CI to run on this patch, move it to an open commitfest.
8+
</div>
9+
{%endif%}
10+
{%endwith%}
411
{%include "patch_commands.inc"%}
512
<table class="table table-bordered">
613
<tbody>

pgcommitfest/commitfest/tests/conftest.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,20 @@
77
import pytest
88

99
from pgcommitfest.commitfest.models import CommitFest
10+
from pgcommitfest.userprofile.models import UserProfile
1011

1112

1213
@pytest.fixture
1314
def alice():
14-
"""Create test user Alice."""
15-
return User.objects.create_user(
15+
"""Create test user Alice with notify_all_author enabled."""
16+
user = User.objects.create_user(
1617
username="alice",
1718
first_name="Alice",
1819
last_name="Anderson",
1920
email="alice@example.com",
2021
)
22+
UserProfile.objects.create(user=user, notify_all_author=True)
23+
return user
2124

2225

2326
@pytest.fixture

0 commit comments

Comments
 (0)