Skip to content

Commit 72bb129

Browse files
JelteFFennemAI
andauthored
Stop sending email on behalf of users (#126)
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. This removes the Comment/Review form entirely. Instead a new Review button on the patch page 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 you can reply to from your own email client. The help page explains the same. 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 --------- Co-authored-by: Jelterminator FennemAI-Neo <fennemai@jeltef.nl>
1 parent 5e72c55 commit 72bb129

9 files changed

Lines changed: 100 additions & 253 deletions

File tree

‎pgcommitfest/commitfest/forms.py‎

Lines changed: 1 addition & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
from django.http import Http404
66

77
from .ajax import _archivesAPI
8-
from .models import MailThread, Patch, PatchOnCommitFest, Tag, TargetVersion
8+
from .models import Patch, PatchOnCommitFest, Tag, TargetVersion
99
from .widgets import ThreadPickWidget
1010

1111

@@ -214,91 +214,6 @@ def clean_threadmsgid(self):
214214
return self.cleaned_data["threadmsgid"]
215215

216216

217-
def _fetch_thread_choices(patch):
218-
for mt in patch.mailthread_set.order_by("-latestmessage"):
219-
ti = sorted(
220-
_archivesAPI("/message-id.json/%s" % mt.messageid),
221-
key=lambda x: x["date"],
222-
reverse=True,
223-
)
224-
yield [
225-
mt.subject,
226-
[
227-
(
228-
"%s,%s" % (mt.messageid, t["msgid"]),
229-
"From %s at %s" % (t["from"], t["date"]),
230-
)
231-
for t in ti
232-
],
233-
]
234-
235-
236-
review_state_choices = (
237-
(0, "Tested"),
238-
(1, "Passed"),
239-
)
240-
241-
242-
def reviewfield(label):
243-
return forms.MultipleChoiceField(
244-
choices=review_state_choices,
245-
label=label,
246-
widget=forms.CheckboxSelectMultiple,
247-
required=False,
248-
)
249-
250-
251-
class CommentForm(forms.Form):
252-
responseto = forms.ChoiceField(choices=[], required=True, label="In response to")
253-
254-
# Specific checkbox fields for reviews
255-
review_installcheck = reviewfield("make installcheck-world")
256-
review_implements = reviewfield("Implements feature")
257-
review_spec = reviewfield("Spec compliant")
258-
review_doc = reviewfield("Documentation")
259-
260-
message = forms.CharField(required=True, widget=forms.Textarea)
261-
newstatus = forms.ChoiceField(
262-
choices=PatchOnCommitFest.OPEN_STATUS_CHOICES(), label="New status"
263-
)
264-
265-
def __init__(self, patch, poc, is_review, *args, **kwargs):
266-
super(CommentForm, self).__init__(*args, **kwargs)
267-
self.is_review = is_review
268-
269-
self.fields["responseto"].choices = _fetch_thread_choices(patch)
270-
self.fields["newstatus"].initial = poc.status
271-
if not is_review:
272-
del self.fields["review_installcheck"]
273-
del self.fields["review_implements"]
274-
del self.fields["review_spec"]
275-
del self.fields["review_doc"]
276-
277-
def clean_responseto(self):
278-
try:
279-
(threadid, respid) = self.cleaned_data["responseto"].split(",")
280-
self.thread = MailThread.objects.get(messageid=threadid)
281-
self.respid = respid
282-
except MailThread.DoesNotExist:
283-
raise ValidationError("Selected thread appears to no longer exist")
284-
except Exception:
285-
raise ValidationError("Invalid message selected")
286-
return self.cleaned_data["responseto"]
287-
288-
def clean(self):
289-
if self.is_review:
290-
for fn, f in self.fields.items():
291-
if fn.startswith("review_") and fn in self.cleaned_data:
292-
if (
293-
"1" in self.cleaned_data[fn]
294-
and "0" not in self.cleaned_data[fn]
295-
):
296-
self.errors[fn] = (
297-
("Cannot pass a test without performing it!"),
298-
)
299-
return self.cleaned_data
300-
301-
302217
class BulkEmailForm(forms.Form):
303218
reviewers = forms.CharField(required=False, widget=HiddenInput())
304219
authors = forms.CharField(required=False, widget=HiddenInput())

‎pgcommitfest/commitfest/templates/base_form.html‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,7 @@
5151
<div class="alert alert-danger">{{e}}</div>
5252
{%endfor%}
5353
{%endif%}
54-
{%if field.name|slice:":7" == 'review_' %}
55-
{%for choice in field %}
56-
<div class="form-check form-check-inline">
57-
<input type="checkbox" class="form-check-input" name="{{field.name}}" value="{{choice.data.value}}" id="{{choice.id_for_label}}" {% if choice.data.selected %}checked{% endif %}>
58-
<label class="form-check-label" for="{{choice.id_for_label}}">{{choice.choice_label}}</label>
59-
</div>
60-
{%endfor%}
61-
{%elif not field.name in form.selectize_fields%}{{field|field_class:"form-control"}}{%else%}{{field}}{%endif%}
54+
{%if not field.name in form.selectize_fields%}{{field|field_class:"form-control"}}{%else%}{{field}}{%endif%}
6255
{%if field.help_text%}<br/>{{field.help_text|safe}}{%endif%}</div>
6356
</div>
6457
{%endif%}

‎pgcommitfest/commitfest/templates/help.html‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ <h2>Patches</h2>
2424
<p>
2525
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.
2626
</p>
27+
<p>
28+
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.
29+
</p>
2730
<p>
2831
There are three "Active" categories of patch status:
2932
<ul>

‎pgcommitfest/commitfest/templates/patch.html‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,33 @@ <h4 class="modal-title">Flag as committed</h4>
265265
</div>
266266
</div>
267267

268+
{%comment%}review instructions dialog{%endcomment%}
269+
<div class="modal fade" id="reviewModal" role="dialog">
270+
<div class="modal-dialog">
271+
<div class="modal-content">
272+
<div class="modal-header">
273+
<h4 class="modal-title">Review</h4>
274+
<button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
275+
</div>
276+
<div class="modal-body">
277+
<p>Reviews are posted by replying to the patch's thread on the mailinglist. If the thread isn't in your inbox:</p>
278+
<ol>
279+
<li>Open the latest message in the archives (or whichever message you want to reply to):
280+
<ul>
281+
{%for t in patch.mailthread_set.all%}
282+
<li><a href="https://www.postgresql.org/message-id/{{t.latestmsgid}}">{{t.subject}}</a></li>
283+
{%endfor%}
284+
</ul>
285+
</li>
286+
<li>Log in and click "Resend email" to get a copy in your inbox.</li>
287+
<li>Reply all to that email with your review.</li>
288+
</ol>
289+
<p class="mb-0">Afterwards, use "Change Status" if needed.</p>
290+
</div>
291+
</div>
292+
</div>
293+
</div>
294+
268295
{%include "thread_attach.inc"%}
269296
{%comment%}Modal dialog for adding annotation{%endcomment%}
270297
<div class="modal fade" id="annotateModal" role="dialog">

‎pgcommitfest/commitfest/templates/patch_commands.inc‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,6 @@
11
<div style="margin-bottom:5px;">
22
<a class="btn btn-secondary" href="edit/">Edit</a>
3-
4-
<div class="btn-group">
5-
<button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">Comment/Review</button>
6-
<ul class="dropdown-menu">
7-
<li><a class="dropdown-item" href="comment/">Comment</a></li>
8-
<li><a class="dropdown-item" href="review/">Review</a></li>
9-
</ul>
10-
</div>
3+
<button type="button" class="btn btn-secondary" data-bs-toggle="modal" data-bs-target="#reviewModal">Review</button>
114

125
<div class="btn-group">
136
<button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">Change Status</button>
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
from datetime import datetime
2+
from email import message_from_string
3+
4+
import pytest
5+
6+
from pgcommitfest.commitfest.models import Patch, PatchOnCommitFest
7+
from pgcommitfest.mailqueue.models import QueuedMail
8+
9+
pytestmark = pytest.mark.django_db
10+
11+
12+
def test_staff_email_not_sent_from_manager_address(
13+
client, open_cf, alice, charlie, settings
14+
):
15+
patch = Patch.objects.create(name="Test patch")
16+
patch.authors.add(alice)
17+
PatchOnCommitFest.objects.create(
18+
patch=patch, commitfest=open_cf, enterdate=datetime.now()
19+
)
20+
charlie.is_staff = True
21+
charlie.save()
22+
client.force_login(charlie)
23+
24+
client.post(
25+
f"/patch/{patch.id}/send_email/",
26+
{
27+
"authors": patch.id,
28+
"reviewers": "",
29+
"subject": "Hi",
30+
"body": "Rebase",
31+
"confirm": "on",
32+
},
33+
)
34+
35+
mail = QueuedMail.objects.get()
36+
msg = message_from_string(mail.fullmsg)
37+
assert mail.sender == settings.NOTIFICATION_FROM
38+
assert msg["Reply-To"] == "Charlie Chen <charlie@example.com>"

0 commit comments

Comments
 (0)