Page MenuHomePhabricator

Verify WMF use cases are accommodated after Herald Outbound Rules resolves
Open, WishlistPublic

Description

Originally reported in the WMF Phab install as https://phabricator.wikimedia.org/T117186

(My summary not given by the original reporter: This is yet another "Please add more mail headers so people watching tons of projects or repositories can more actively filter/tag their email, for those who primarily use their email for managing tasks/commits." request.)

The notification emails lacks headers representing the project a patch is made against as well as metadata for what the mail is about. As a result, when watching several repositories, there is no good way to filter them in different folders.

An example for a revision creation. The mail shows:

dduvall added a task: T117131: Include architectural overview in scap3 documentation.
dduvall created this revision.
dduvall added a reviewer: mobrovac.
Herald added a reviewer: Release-Engineering-Team.

REVISION SUMMARY
Starting off with a pretty little activity diagram.

REPOSITORY
rMSCA scap

BRANCH
T117131

Mail headers:

Subject: [Differential] [Request, 73 lines] D31: WIP Include scap3 architectural overview
X-Phabricator-Sent-This-Message: Yes
X-Phabricator-Mail-Tags: <differential-review-request>, <differential-other>, <differential-reviewers>
Thread-Topic: D31: WIP Include scap3 architectural overview
X-Herald-Rules: <101>, <96>, <102>
X-Phabricator-To: <PHID-USER-7bifphmxtcqgvdofuzla>
X-Phabricator-To: <PHID-PROJ-uier7rukzszoewbhj7ja>
X-Phabricator-To: <PHID-USER-hxwwywcyzpooynxuo7a2>
X-Phabricator-Cc: <PHID-USER-x7ti5ksby4ubsabntlxa>

An example for an accepted change:

mmodell accepted this revision.
mmodell added a comment.
This revision is now accepted and ready to land.

Looks good to me but I'm not sure how to test

REPOSITORY
rMSCA scap

Mail headers:

X-Phabricator-Sent-This-Message: Yes
X-Phabricator-Mail-Tags: <differential-other>, <differential-reviewers>, <differential-comment>
Thread-Topic: D24: Handle git-update-server from git itself
X-Herald-Rules: <96>, <101>, <102>
X-Phabricator-To: <PHID-USER-oetk6bbl6omm354ejz3b>
X-Phabricator-To: <PHID-USER-5ewyncd6mpezaymyxfal>
X-Phabricator-To: <PHID-USER-7bifphmxtcqgvdofuzla>
X-Phabricator-To: <PHID-PROJ-uier7rukzszoewbhj7ja>
X-Phabricator-To: <PHID-USER-fn7qnpccfbitivgtw2rt>

In Gerrit, the repository is mentioned with the standard List-Id header such as:
List-Id: <gerrit-mediawiki-extensions-MultimediaViewer.gerrit.wikimedia.org>

And an indication about the type of notification which let one highlight them with different colors:

X-Gerrit-MessageType: abandon
X-Gerrit-MessageType: comment
X-Gerrit-MessageType: merged

Event Timeline

This is basically all reasonable but entangled with the giant mess in T5791 and T9161.

Very briefly:

  • Within mail handling, we receive relatively few requests for more powerful filtering/sorting tools, and relatively more requests to fix "too much mail" in some form.
  • "Too much mail" is a complicated problem, discussion in T9161. Briefly, we have some evidence that the root cause of this problem is not that we send too much mail.
  • Since I'm confident we can solve "insufficiently powerful tools" in a lot of ways but less confident we can solve "too much mail" at all, I want "too much mail" to be the primary problem informing how we approach mail delivery. Particularly, I don't want to finish T5791 (which adds a lot of powerful tools) in its current form if it might make "too much mail" worse, or make fixing it harder or more cumbersome.
  • Many users use mail clients (primarily Gmail) which can not do any kind of header filtering.

If you want to fork, you can add a least the repository header trivially in DifferentialTransactionEditor->buildMailTemplate(), and it will likely remain stable until upstream support moves ahead. Something like this:

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index fabd251..8e60263 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1188,9 +1188,16 @@ final class DifferentialTransactionEditor
     $subject = "D{$id}: {$title}";
     $thread_topic = "D{$id}: {$original_title}";
 
-    return id(new PhabricatorMetaMTAMail())
+    $mail = id(new PhabricatorMetaMTAMail())
       ->setSubject($subject)
       ->addHeader('Thread-Topic', $thread_topic);
+
+    $repository_phid = $object->getRepositoryPHID();
+    if ($repository_phid) {
+      $mail->addHeader('X-Custom-Repository-PHID', $repository_phid);;
+    }
+
+    return $mail;
   }
 
   protected function buildMailBody(

However, this won't work with Gmail and won't work with Herald outbound rules (T5791), and will almost certainly be obsoleted by a more powerful mechanism once T5791 moves further forward, so I don't want to bring it upstream because we'd get stuck with it even after it was obsolete.

(I think you should already be able to use the "Subject:" header to filter state changes?)

I'll leave this open for now and we can circle back after T5791 to make sure it covers everything, but I don't expect to make the changes you request outside of the scope of that, which is large on its own ("Build a more powerful Gmail in Phabricator so users who use Gmail can still do header filtering") and blocked on T9161 which is a very soft user experience task that we don't really even know how to move forward on.

epriestley renamed this task from Differential notification emails lack headers for repository and state change to Verify WMF use cases are accommodated after Herald Outbound Rules resolves.Dec 3 2015, 2:46 PM
epriestley triaged this task as Wishlist priority.