From cb3d34e6bab3912ca2f6893fb3ac51cefd16c663 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 18:03:00 +0100 Subject: [PATCH 1/5] Stop sending email on behalf of users Comments and reviews posted through the commitfest app were sent to the mailinglist with the user's own address as both the `From` header and the envelope sender. Our mail servers can't pass SPF or DKIM for the user's domain, so these emails bounced or ended up in spam for any domain with a strict DMARC policy, without the user ever finding out. This also caused a steady stream of delivery errors for pginfra. Instead of sending the email ourselves, the Comment and Review pages now link to the latest message of each thread in the archives, and explain how to use its "Resend email" button to get a copy of the message in your own inbox, so you can reply from your own email client. The review page also shows the usual review checklist, so people can still include it in their reply. The bulk emails that CF managers can send to authors and reviewers had the same problem. These are now sent from `NOTIFICATION_FROM`, with the manager's name in the display name and their address in `Reply-To`, so replies still go to them. Fixes #37 --- pgcommitfest/commitfest/forms.py | 87 +--------- .../commitfest/templates/base_form.html | 9 +- .../commitfest/templates/patch_reply.html | 36 +++++ .../commitfest/tests/test_patch_email.py | 77 +++++++++ pgcommitfest/commitfest/views.py | 151 +++--------------- pgcommitfest/mailqueue/util.py | 19 ++- 6 files changed, 153 insertions(+), 226 deletions(-) create mode 100644 pgcommitfest/commitfest/templates/patch_reply.html create mode 100644 pgcommitfest/commitfest/tests/test_patch_email.py diff --git a/pgcommitfest/commitfest/forms.py b/pgcommitfest/commitfest/forms.py index a01b437e..444905b1 100644 --- a/pgcommitfest/commitfest/forms.py +++ b/pgcommitfest/commitfest/forms.py @@ -5,7 +5,7 @@ from django.http import Http404 from .ajax import _archivesAPI -from .models import MailThread, Patch, PatchOnCommitFest, Tag, TargetVersion +from .models import Patch, PatchOnCommitFest, Tag, TargetVersion from .widgets import ThreadPickWidget @@ -214,91 +214,6 @@ def clean_threadmsgid(self): return self.cleaned_data["threadmsgid"] -def _fetch_thread_choices(patch): - for mt in patch.mailthread_set.order_by("-latestmessage"): - ti = sorted( - _archivesAPI("/message-id.json/%s" % mt.messageid), - key=lambda x: x["date"], - reverse=True, - ) - yield [ - mt.subject, - [ - ( - "%s,%s" % (mt.messageid, t["msgid"]), - "From %s at %s" % (t["from"], t["date"]), - ) - for t in ti - ], - ] - - -review_state_choices = ( - (0, "Tested"), - (1, "Passed"), -) - - -def reviewfield(label): - return forms.MultipleChoiceField( - choices=review_state_choices, - label=label, - widget=forms.CheckboxSelectMultiple, - required=False, - ) - - -class CommentForm(forms.Form): - responseto = forms.ChoiceField(choices=[], required=True, label="In response to") - - # Specific checkbox fields for reviews - review_installcheck = reviewfield("make installcheck-world") - review_implements = reviewfield("Implements feature") - review_spec = reviewfield("Spec compliant") - review_doc = reviewfield("Documentation") - - message = forms.CharField(required=True, widget=forms.Textarea) - newstatus = forms.ChoiceField( - choices=PatchOnCommitFest.OPEN_STATUS_CHOICES(), label="New status" - ) - - def __init__(self, patch, poc, is_review, *args, **kwargs): - super(CommentForm, self).__init__(*args, **kwargs) - self.is_review = is_review - - self.fields["responseto"].choices = _fetch_thread_choices(patch) - self.fields["newstatus"].initial = poc.status - if not is_review: - del self.fields["review_installcheck"] - del self.fields["review_implements"] - del self.fields["review_spec"] - del self.fields["review_doc"] - - def clean_responseto(self): - try: - (threadid, respid) = self.cleaned_data["responseto"].split(",") - self.thread = MailThread.objects.get(messageid=threadid) - self.respid = respid - except MailThread.DoesNotExist: - raise ValidationError("Selected thread appears to no longer exist") - except Exception: - raise ValidationError("Invalid message selected") - return self.cleaned_data["responseto"] - - def clean(self): - if self.is_review: - for fn, f in self.fields.items(): - if fn.startswith("review_") and fn in self.cleaned_data: - if ( - "1" in self.cleaned_data[fn] - and "0" not in self.cleaned_data[fn] - ): - self.errors[fn] = ( - ("Cannot pass a test without performing it!"), - ) - return self.cleaned_data - - class BulkEmailForm(forms.Form): reviewers = forms.CharField(required=False, widget=HiddenInput()) authors = forms.CharField(required=False, widget=HiddenInput()) diff --git a/pgcommitfest/commitfest/templates/base_form.html b/pgcommitfest/commitfest/templates/base_form.html index d4bb7805..01552912 100644 --- a/pgcommitfest/commitfest/templates/base_form.html +++ b/pgcommitfest/commitfest/templates/base_form.html @@ -51,14 +51,7 @@
{{e}}
{%endfor%} {%endif%} - {%if field.name|slice:":7" == 'review_' %} - {%for choice in field %} -
- - -
- {%endfor%} - {%elif not field.name in form.selectize_fields%}{{field|field_class:"form-control"}}{%else%}{{field}}{%endif%} + {%if not field.name in form.selectize_fields%}{{field|field_class:"form-control"}}{%else%}{{field}}{%endif%} {%if field.help_text%}
{{field.help_text|safe}}{%endif%} {%endif%} diff --git a/pgcommitfest/commitfest/templates/patch_reply.html b/pgcommitfest/commitfest/templates/patch_reply.html new file mode 100644 index 00000000..b95a7451 --- /dev/null +++ b/pgcommitfest/commitfest/templates/patch_reply.html @@ -0,0 +1,36 @@ +{%extends "base.html"%} +{%load commitfest%} +{%block contents%} +

+ Discussion about patches happens on the {{hackers_email}} mailinglist. To post a {%if is_review%}review{%else%}comment{%endif%}, reply to the email thread of this patch from your own email client: +

+
    +
  1. Open the message you want to reply to in the archives, using one of the links below.
    2. +
  2. Log in on the archives page and click Resend email. This sends a copy of the message to your own email address.
    3. +
  3. Reply to that email from your email client, using "Reply All" so that the mailinglist and everyone involved in the discussion are included.
    4. +
+ {%if threads%} +
+ {%for t in threads%} +
{{t.subject}}
+
+ Latest message: {{t.latestmessage}} by {{t.latestauthor|hidemail}} +
+ {%endfor%} +
+ {%else%} +
This patch has no email threads attached.
+ {%endif%} + {%if is_review%} +

+ Reviews traditionally start with the following summary of what you tested. Each line should say either not tested, tested, passed or tested, failed: +

+ 
make installcheck-world:  not tested
+Implements feature:       not tested
+Spec compliant:           not tested
+Documentation:            not tested
+ {%endif%} +

+ If your {%if is_review%}review{%else%}comment{%endif%} means that the status of the patch should change, then use the Change Status button on the patch page after sending your email. +

+{%endblock%} diff --git a/pgcommitfest/commitfest/tests/test_patch_email.py b/pgcommitfest/commitfest/tests/test_patch_email.py new file mode 100644 index 00000000..b884dcd9 --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_patch_email.py @@ -0,0 +1,77 @@ +from datetime import datetime +from email import message_from_string + +import pytest + +from pgcommitfest.commitfest.models import MailThread, Patch, PatchOnCommitFest +from pgcommitfest.mailqueue.models import QueuedMail + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def patch_with_thread(open_cf, alice, bob): + patch = Patch.objects.create(name="Test patch") + patch.authors.add(alice) + patch.reviewers.add(bob) + PatchOnCommitFest.objects.create( + patch=patch, + commitfest=open_cf, + enterdate=datetime.now(), + status=PatchOnCommitFest.STATUS_REVIEW, + ) + thread = MailThread.objects.create( + messageid="first@example.com", + subject="[PATCH] Test", + firstmessage=datetime(2025, 1, 1), + firstauthor="Alice ", + latestmessage=datetime(2025, 1, 2), + latestauthor="Bob ", + latestsubject="Re: [PATCH] Test", + latestmsgid="latest@example.com", + ) + patch.mailthread_set.add(thread) + return patch + + +@pytest.mark.parametrize("what", ["comment", "review"]) +def test_reply_page_links_to_archives( + client, patch_with_thread, in_progress_cf, recent_closed_cf, what +): + response = client.get(f"/patch/{patch_with_thread.id}/{what}/") + assert response.status_code == 200 + content = response.content.decode() + assert "https://www.postgresql.org/message-id/latest@example.com" in content + assert "Resend email" in content + assert ("make installcheck-world" in content) == (what == "review") + assert not QueuedMail.objects.exists() + + +def test_staff_email_not_sent_from_manager_address( + client, patch_with_thread, alice, bob, charlie, settings +): + charlie.is_staff = True + charlie.save() + client.force_login(charlie) + + response = client.post( + f"/patch/{patch_with_thread.id}/send_email/", + { + "authors": str(patch_with_thread.id), + "reviewers": str(patch_with_thread.id), + "subject": "Hello", + "body": "Please rebase", + "confirm": "on", + }, + ) + assert response.status_code == 302 + + mails = list(QueuedMail.objects.order_by("receiver")) + assert [m.receiver for m in mails] == ["alice@example.com", "bob@example.com"] + for m in mails: + assert m.sender == settings.NOTIFICATION_FROM + msg = message_from_string(m.fullmsg) + assert msg["From"] == ( + f"Charlie Chen via PostgreSQL Commitfest <{settings.NOTIFICATION_FROM}>" + ) + assert msg["Reply-To"] == "Charlie Chen " diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 0a697c9f..158521bd 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -20,10 +20,8 @@ import json import urllib from datetime import datetime, timedelta -from email.mime.text import MIMEText -from email.utils import formatdate, make_msgid -from pgcommitfest.mailqueue.util import send_mail, send_simple_mail +from pgcommitfest.mailqueue.util import send_simple_mail from pgcommitfest.userprofile.models import UserProfile from pgcommitfest.userprofile.util import UserWrapper @@ -31,7 +29,6 @@ from .feeds import ActivityFeed from .forms import ( BulkEmailForm, - CommentForm, CommitFestFilterForm, NewPatchForm, PatchForm, @@ -1109,143 +1106,29 @@ def newpatch(request, cfid): ) -def _review_status_string(reviewstatus): - if "0" in reviewstatus: - if "1" in reviewstatus: - return "tested, passed" - else: - return "tested, failed" - else: - return "not tested" - - -@login_required -@transaction.atomic def comment(request, patchid, what): + # We used to send comments and reviews to the mailinglist from here, with + # the user's address in the From header. That broke for any domain with a + # strict DMARC policy, since we can't DKIM sign on behalf of that domain. + # Instead we now point people to the archives, which can resend a message + # to them so they can reply to it from their own email client. patch = get_object_or_404(Patch, pk=patchid) cf = patch.current_commitfest() - poc = get_object_or_404(PatchOnCommitFest, patch=patch, commitfest=cf) - is_review = what == "review" - - if request.method == "POST": - try: - form = CommentForm(patch, poc, is_review, data=request.POST) - except Exception as e: - messages.add_message( - request, - messages.ERROR, - "Failed to build list of response options from the archives: %s" % e, - ) - return HttpResponseRedirect("/%s/%s/" % (cf.id, patch.id)) - - if form.is_valid(): - if is_review: - txt = ( - "The following review has been posted through the commitfest application:\n%s\n\n%s" - % ( - "\n".join( - [ - "%-25s %s" - % ( - f.label + ":", - _review_status_string(form.cleaned_data[fn]), - ) - for (fn, f) in form.fields.items() - if fn.startswith("review_") - ] - ), - form.cleaned_data["message"], - ) - ) - else: - txt = form.cleaned_data["message"] - - if int(form.cleaned_data["newstatus"]) != poc.status: - poc.status = int(form.cleaned_data["newstatus"]) - poc.save() - PatchHistory( - patch=poc.patch, - by=request.user, - what="New status: %s" % poc.statusstring, - ).save_and_notify() - txt += "\n\nThe new status of this patch is: %s\n" % poc.statusstring - - msg = MIMEText(txt, _charset="utf-8") - - if form.thread.subject.startswith("Re:"): - msg["Subject"] = form.thread.subject - else: - msg["Subject"] = "Re: %s" % form.thread.subject - - msg["To"] = settings.HACKERS_EMAIL - msg["From"] = UserWrapper(request.user).encoded_email_header - - # CC the authors of a patch, if there are any - authors = list(patch.authors.all()) - if len(authors): - msg["Cc"] = ", ".join( - [UserWrapper(a).encoded_email_header for a in authors] - ) - - msg["Date"] = formatdate(localtime=True) - msg["User-Agent"] = "pgcommitfest" - msg["X-cfsender"] = request.user.username - msg["In-Reply-To"] = "<%s>" % form.respid - # We just add the "top" messageid and the one we're responding to. - # This along with in-reply-to should indicate clearly enough where - # in the thread the message belongs. - msg["References"] = "<%s> <%s>" % (form.thread.messageid, form.respid) - msg["Message-ID"] = make_msgid("pgcf") - - uw = UserWrapper(request.user) - msgstring = msg.as_string() - send_mail(uw.email, settings.HACKERS_EMAIL, msgstring) - for a in authors: - # Actually send a copy directly to the author. Just setting the Cc field doesn't - # make it deliver the email... - send_mail(uw.email, UserWrapper(a).email, msgstring) - - PatchHistory( - patch=patch, - by=request.user, - what="Posted %s with messageid %s" % (what, msg["Message-ID"]), - ).save() - - messages.add_message( - request, - messages.INFO, - "Your email has been queued for %s, and will be sent within a few minutes." - % (settings.HACKERS_EMAIL), - ) - - return HttpResponseRedirect("/%s/%s/" % (cf.id, patch.id)) - else: - try: - form = CommentForm(patch, poc, is_review) - except Exception as e: - messages.add_message( - request, - messages.ERROR, - "Failed to build list of response options from the archives: %s" % e, - ) - return HttpResponseRedirect("/%s/%s/" % (cf.id, patch.id)) return render( request, - "base_form.html", + "patch_reply.html", { "cf": cf, - "form": form, "patch": patch, - "extraformclass": "patchcommentform", + "threads": patch.mailthread_set.order_by("-latestmessage"), + "is_review": what == "review", + "hackers_email": settings.HACKERS_EMAIL, "breadcrumbs": [ {"title": cf.title, "href": "/%s/" % cf.pk}, {"title": "View patch", "href": "/%s/%s/" % (cf.pk, patch.pk)}, ], "title": "Add %s" % what, - "note": 'Note! This form will generate an email to the public mailinglist %s, with sender set to %s!
Please ensure that the email settings for your domain (DKIM, SPF) allow emails from external sources.' - % (settings.HACKERS_EMAIL, UserWrapper(request.user).email), - "savebutton": "Send %s" % what, }, ) @@ -1524,13 +1407,22 @@ def send_email(request, cfid): recipients = User.objects.filter(q).distinct() + # We send from our own address instead of the manager's, because + # many domains have a DMARC policy that causes emails with their + # address in the From header to be rejected when they're not sent + # through their own mail servers. Reply-To makes replies still go + # to the manager. + uw = UserWrapper(request.user) for r in recipients: send_simple_mail( - UserWrapper(request.user).email, + settings.NOTIFICATION_FROM, r.email, form.cleaned_data["subject"], form.cleaned_data["body"], request.user.username, + sendername="%s %s via PostgreSQL Commitfest" + % (request.user.first_name, request.user.last_name), + replyto=uw.encoded_email_header, ) messages.add_message( request, messages.INFO, "Sent email to %s" % r.email @@ -1569,7 +1461,8 @@ def send_email(request, cfid): messages.add_message( request, messages.INFO, - "Email will be sent from: %s" % UserWrapper(request.user).email, + "Email will be sent from %s, with replies going to: %s" + % (settings.NOTIFICATION_FROM, UserWrapper(request.user).email), ) def _user_and_mail(u): diff --git a/pgcommitfest/mailqueue/util.py b/pgcommitfest/mailqueue/util.py index 441802b3..48fbbef0 100644 --- a/pgcommitfest/mailqueue/util.py +++ b/pgcommitfest/mailqueue/util.py @@ -1,23 +1,36 @@ from django.template.loader import get_template from email import encoders +from email.header import Header from email.mime.multipart import MIMEMultipart from email.mime.nonmultipart import MIMENonMultipart from email.mime.text import MIMEText -from email.utils import formatdate +from email.utils import formataddr, formatdate from .models import QueuedMail def send_simple_mail( - sender, receiver, subject, msgtxt, sending_username, attachments=None + sender, + receiver, + subject, + msgtxt, + sending_username, + attachments=None, + sendername=None, + replyto=None, ): # attachment format, each is a tuple of (name, mimetype,contents) # content should already be base64 encoded msg = MIMEMultipart() msg["Subject"] = subject msg["To"] = receiver - msg["From"] = sender + if sendername: + msg["From"] = formataddr((str(Header(sendername, "utf-8")), sender)) + else: + msg["From"] = sender + if replyto: + msg["Reply-To"] = replyto msg["Date"] = formatdate(localtime=True) msg["User-Agent"] = "pgcommitfest" if sending_username: From fddd2501d763d8114b6e423a2be2f96c45edbca3 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 18:07:27 +0100 Subject: [PATCH 2/5] Replace the reply instructions page with a reply link per thread The separate page explaining how to reply through the archives was more verbose than needed. Now the Comment/Review dropdown and its route are removed, and each thread on the patch page gets a "reply" link to its latest message in the archives, with a tooltip explaining to use "Resend email" there. --- pgcommitfest/commitfest/templates/patch.html | 2 +- .../commitfest/templates/patch_commands.inc | 8 --- .../commitfest/templates/patch_reply.html | 36 ---------- .../commitfest/tests/test_patch_email.py | 69 ++++--------------- pgcommitfest/commitfest/views.py | 27 -------- pgcommitfest/urls.py | 1 - 6 files changed, 16 insertions(+), 127 deletions(-) delete mode 100644 pgcommitfest/commitfest/templates/patch_reply.html diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 2121bcf5..c816126c 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -159,7 +159,7 @@
{{t.subject}}
First at {{t.firstmessage}} by {{t.firstauthor|hidemail}}
- Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}}
+ Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}} (reply)
{%for ta in t.mailthreadattachment_set.all%} {%if forloop.first%} Latest attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} diff --git a/pgcommitfest/commitfest/templates/patch_commands.inc b/pgcommitfest/commitfest/templates/patch_commands.inc index a1f45e7e..e96c9cfa 100644 --- a/pgcommitfest/commitfest/templates/patch_commands.inc +++ b/pgcommitfest/commitfest/templates/patch_commands.inc @@ -1,14 +1,6 @@
Edit -
- - -
-
    diff --git a/pgcommitfest/commitfest/templates/patch_reply.html b/pgcommitfest/commitfest/templates/patch_reply.html deleted file mode 100644 index b95a7451..00000000 --- a/pgcommitfest/commitfest/templates/patch_reply.html +++ /dev/null @@ -1,36 +0,0 @@ -{%extends "base.html"%} -{%load commitfest%} -{%block contents%} -

    - Discussion about patches happens on the {{hackers_email}} mailinglist. To post a {%if is_review%}review{%else%}comment{%endif%}, reply to the email thread of this patch from your own email client: -

    -
      -
    1. Open the message you want to reply to in the archives, using one of the links below.
      2. -
    2. Log in on the archives page and click Resend email. This sends a copy of the message to your own email address.
      3. -
    3. Reply to that email from your email client, using "Reply All" so that the mailinglist and everyone involved in the discussion are included.
      4. -
    - {%if threads%} -
    - {%for t in threads%} -
    {{t.subject}}
    -
    - Latest message: {{t.latestmessage}} by {{t.latestauthor|hidemail}} -
    - {%endfor%} -
    - {%else%} -
    This patch has no email threads attached.
    - {%endif%} - {%if is_review%} -

    - Reviews traditionally start with the following summary of what you tested. Each line should say either not tested, tested, passed or tested, failed: -

    - 
    make installcheck-world:  not tested
-Implements feature:       not tested
-Spec compliant:           not tested
-Documentation:            not tested
    - {%endif%} -

    - If your {%if is_review%}review{%else%}comment{%endif%} means that the status of the patch should change, then use the Change Status button on the patch page after sending your email. -

    -{%endblock%} diff --git a/pgcommitfest/commitfest/tests/test_patch_email.py b/pgcommitfest/commitfest/tests/test_patch_email.py index b884dcd9..5b3d3ad2 100644 --- a/pgcommitfest/commitfest/tests/test_patch_email.py +++ b/pgcommitfest/commitfest/tests/test_patch_email.py @@ -3,75 +3,36 @@ import pytest -from pgcommitfest.commitfest.models import MailThread, Patch, PatchOnCommitFest +from pgcommitfest.commitfest.models import Patch, PatchOnCommitFest from pgcommitfest.mailqueue.models import QueuedMail pytestmark = pytest.mark.django_db -@pytest.fixture -def patch_with_thread(open_cf, alice, bob): +def test_staff_email_not_sent_from_manager_address( + client, open_cf, alice, charlie, settings +): patch = Patch.objects.create(name="Test patch") patch.authors.add(alice) - patch.reviewers.add(bob) PatchOnCommitFest.objects.create( - patch=patch, - commitfest=open_cf, - enterdate=datetime.now(), - status=PatchOnCommitFest.STATUS_REVIEW, - ) - thread = MailThread.objects.create( - messageid="first@example.com", - subject="[PATCH] Test", - firstmessage=datetime(2025, 1, 1), - firstauthor="Alice ", - latestmessage=datetime(2025, 1, 2), - latestauthor="Bob ", - latestsubject="Re: [PATCH] Test", - latestmsgid="latest@example.com", + patch=patch, commitfest=open_cf, enterdate=datetime.now() ) - patch.mailthread_set.add(thread) - return patch - - -@pytest.mark.parametrize("what", ["comment", "review"]) -def test_reply_page_links_to_archives( - client, patch_with_thread, in_progress_cf, recent_closed_cf, what -): - response = client.get(f"/patch/{patch_with_thread.id}/{what}/") - assert response.status_code == 200 - content = response.content.decode() - assert "https://www.postgresql.org/message-id/latest@example.com" in content - assert "Resend email" in content - assert ("make installcheck-world" in content) == (what == "review") - assert not QueuedMail.objects.exists() - - -def test_staff_email_not_sent_from_manager_address( - client, patch_with_thread, alice, bob, charlie, settings -): charlie.is_staff = True charlie.save() client.force_login(charlie) - response = client.post( - f"/patch/{patch_with_thread.id}/send_email/", + client.post( + f"/patch/{patch.id}/send_email/", { - "authors": str(patch_with_thread.id), - "reviewers": str(patch_with_thread.id), - "subject": "Hello", - "body": "Please rebase", + "authors": patch.id, + "reviewers": "", + "subject": "Hi", + "body": "Rebase", "confirm": "on", }, ) - assert response.status_code == 302 - mails = list(QueuedMail.objects.order_by("receiver")) - assert [m.receiver for m in mails] == ["alice@example.com", "bob@example.com"] - for m in mails: - assert m.sender == settings.NOTIFICATION_FROM - msg = message_from_string(m.fullmsg) - assert msg["From"] == ( - f"Charlie Chen via PostgreSQL Commitfest <{settings.NOTIFICATION_FROM}>" - ) - assert msg["Reply-To"] == "Charlie Chen " + mail = QueuedMail.objects.get() + msg = message_from_string(mail.fullmsg) + assert mail.sender == settings.NOTIFICATION_FROM + assert msg["Reply-To"] == "Charlie Chen " diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 158521bd..ef0a3831 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -1106,33 +1106,6 @@ def newpatch(request, cfid): ) -def comment(request, patchid, what): - # We used to send comments and reviews to the mailinglist from here, with - # the user's address in the From header. That broke for any domain with a - # strict DMARC policy, since we can't DKIM sign on behalf of that domain. - # Instead we now point people to the archives, which can resend a message - # to them so they can reply to it from their own email client. - patch = get_object_or_404(Patch, pk=patchid) - cf = patch.current_commitfest() - - return render( - request, - "patch_reply.html", - { - "cf": cf, - "patch": patch, - "threads": patch.mailthread_set.order_by("-latestmessage"), - "is_review": what == "review", - "hackers_email": settings.HACKERS_EMAIL, - "breadcrumbs": [ - {"title": cf.title, "href": "/%s/" % cf.pk}, - {"title": "View patch", "href": "/%s/%s/" % (cf.pk, patch.pk)}, - ], - "title": "Add %s" % what, - }, - ) - - @login_required @transaction.atomic def status(request, patchid, status): diff --git a/pgcommitfest/urls.py b/pgcommitfest/urls.py index f6953347..d246241a 100644 --- a/pgcommitfest/urls.py +++ b/pgcommitfest/urls.py @@ -39,7 +39,6 @@ re_path(r"^patch/(\d+)/reviewer/remove_all/$", views.remove_all_reviewers), re_path(r"^patch/(\d+)/committer/(become|remove)/$", views.committer), re_path(r"^patch/(\d+)/(un)?subscribe/$", views.subscribe), - re_path(r"^patch/(\d+)/(comment|review)/", views.comment), re_path(r"^patch/(\d+)/cfbot_requeue/$", views.cfbot_requeue), re_path(r"^(\d+)/send_email/$", views.send_email), re_path(r"^patch/(\d+)/send_email/$", views.send_patch_email), From 67f6a0c62a03e97bd5e607825682a5c2fad79098 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 18:11:47 +0100 Subject: [PATCH 3/5] Explain how to reply in the help page instead of a reply link The "reply" link pointed to the exact same archives URL as the "Latest at" link right before it, so it didn't add anything. The archives' "Resend email" requires a POST from the archives page itself, so we can't link to it directly either. Instead the help page now explains how to reply to a patch thread that isn't in your inbox. --- pgcommitfest/commitfest/templates/help.html | 3 +++ pgcommitfest/commitfest/templates/patch.html | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pgcommitfest/commitfest/templates/help.html b/pgcommitfest/commitfest/templates/help.html index 609f0214..1b7d556f 100644 --- a/pgcommitfest/commitfest/templates/help.html +++ b/pgcommitfest/commitfest/templates/help.html @@ -24,6 +24,9 @@

    Patches

    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.

    +

    + To comment on or review a patch, reply to its thread on the mailinglist. If the thread is not in your inbox, open its latest message in the archives (linked on the patch page), log in, and use "Resend email" to get a copy that you can reply to. +

    There are three "Active" categories of patch status:

      diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index c816126c..2121bcf5 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -159,7 +159,7 @@
      {{t.subject}}
      First at {{t.firstmessage}} by {{t.firstauthor|hidemail}}
      - Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}} (reply)
      + Latest at {{t.latestmessage}} by {{t.latestauthor|hidemail}}
      {%for ta in t.mailthreadattachment_set.all%} {%if forloop.first%} Latest attachment ({{ta.filename}}) at {{ta.date}} from {{ta.author|hidemail}} From eaa03e6b74139572f17e9defc42d40bb39e79011 Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 25 Oct 2026 18:13:53 +0100 Subject: [PATCH 4/5] Add a Review button that explains how to post a review With the Comment/Review form gone, there was nothing on the patch page anymore that tells people how to review a patch, which is mostly a problem for newcomers that aren't subscribed to the mailinglist. The new Review button opens a small dialog that links to the latest message of each thread in the archives, and explains how to use "Resend email" there to get a copy that you can reply to. --- pgcommitfest/commitfest/templates/patch.html | 27 +++++++++++++++++++ .../commitfest/templates/patch_commands.inc | 1 + 2 files changed, 28 insertions(+) diff --git a/pgcommitfest/commitfest/templates/patch.html b/pgcommitfest/commitfest/templates/patch.html index 2121bcf5..bd26b06b 100644 --- a/pgcommitfest/commitfest/templates/patch.html +++ b/pgcommitfest/commitfest/templates/patch.html @@ -265,6 +265,33 @@

      Flag as committed

+{%comment%}review instructions dialog{%endcomment%} + + {%include "thread_attach.inc"%} {%comment%}Modal dialog for adding annotation{%endcomment%}