Stop sending email on behalf of users

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())

pgcommitfest/commitfest/templates/base_form.html

@@ -51,14 +51,7 @@
          <div class="alert alert-danger">{{e}}</div>
         {%endfor%}
        {%endif%}
-       {%if field.name|slice:":7" == 'review_' %}
-        {%for choice in field %}
-         <div class="form-check form-check-inline">
-          <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 %}>
-          <label class="form-check-label" for="{{choice.id_for_label}}">{{choice.choice_label}}</label>
-         </div>
-        {%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%}<br/>{{field.help_text|safe}}{%endif%}</div>
      </div>
     {%endif%}

pgcommitfest/commitfest/templates/help.html

@@ -24,6 +24,9 @@ <h2>Patches</h2>
  <p>
   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.
  </p>
+ <p>
+  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.
+ </p>
  <p>
   There are three "Active" categories of patch status:
   <ul>

pgcommitfest/commitfest/templates/patch.html

@@ -265,6 +265,33 @@ <h4 class="modal-title">Flag as committed</h4>
   </div>
  </div>
 
+{%comment%}review instructions dialog{%endcomment%}
+ <div class="modal fade" id="reviewModal" role="dialog">
+  <div class="modal-dialog">
+   <div class="modal-content">
+    <div class="modal-header">
+     <h4 class="modal-title">Review</h4>
+     <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
+    </div>
+    <div class="modal-body">
+     <p>Reviews are posted by replying to the patch's thread on the mailinglist. If the thread isn't in your inbox:</p>
+     <ol>
+      <li>Open the latest message in the archives (or whichever message you want to reply to):
+       <ul>
+        {%for t in patch.mailthread_set.all%}
+         <li><a href="https://www.postgresql.org/message-id/{{t.latestmsgid}}">{{t.subject}}</a></li>
+        {%endfor%}
+       </ul>
+      </li>
+      <li>Log in and click "Resend email" to get a copy in your inbox.</li>
+      <li>Reply all to that email with your review.</li>
+     </ol>
+     <p class="mb-0">Afterwards, use "Change Status" if needed.</p>
+    </div>
+   </div>
+  </div>
+ </div>
+
  {%include "thread_attach.inc"%}
 {%comment%}Modal dialog for adding annotation{%endcomment%}
  <div class="modal fade" id="annotateModal" role="dialog">

pgcommitfest/commitfest/templates/patch_commands.inc

@@ -1,13 +1,6 @@
 <div style="margin-bottom:5px;">
  <a class="btn btn-secondary" href="edit/">Edit</a>
-
- <div class="btn-group">
-  <button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">Comment/Review</button>
-  <ul class="dropdown-menu">
-   <li><a class="dropdown-item" href="comment/">Comment</a></li>
-   <li><a class="dropdown-item" href="review/">Review</a></li>
-  </ul>
- </div>
+ <button type="button" class="btn btn-secondary" data-bs-toggle="modal" data-bs-target="#reviewModal">Review</button>
 
  <div class="btn-group">
   <button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">Change Status</button>

pgcommitfest/commitfest/tests/test_patch_email.py

@@ -0,0 +1,38 @@
+from datetime import datetime
+from email import message_from_string
+
+import pytest
+
+from pgcommitfest.commitfest.models import Patch, PatchOnCommitFest
+from pgcommitfest.mailqueue.models import QueuedMail
+
+pytestmark = pytest.mark.django_db
+
+
+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)
+    PatchOnCommitFest.objects.create(
+        patch=patch, commitfest=open_cf, enterdate=datetime.now()
+    )
+    charlie.is_staff = True
+    charlie.save()
+    client.force_login(charlie)
+
+    client.post(
+        f"/patch/{patch.id}/send_email/",
+        {
+            "authors": patch.id,
+            "reviewers": "",
+            "subject": "Hi",
+            "body": "Rebase",
+            "confirm": "on",
+        },
+    )
+
+    mail = QueuedMail.objects.get()
+    msg = message_from_string(mail.fullmsg)
+    assert mail.sender == settings.NOTIFICATION_FROM
+    assert msg["Reply-To"] == "Charlie Chen <charlie@example.com>"