Page MenuHomePhabricator

Build separate mail for each recipient, honoring recipient access levels
ClosedPublic

Authored by epriestley on Jun 3 2015, 12:02 AM.

Details

Summary

Ref T6367. Removes multiplexMail()!

We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.

The new logic does this:

  • First, split recipients into groups called "targets".
    • Each target corresponds to one actual mail we're going to build.
    • Each target has a viewer (whose translation / access levels will be used to generate the mail).
    • Each target has a to/cc list (the users who we'll ultimately send the mail to).
  • For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
  • Then, deliver the mail.
Test Plan
  • Read new config help.

Then did a bunch of testing, primarily with bin/mail list-outbound and bin/mail show-outbound (to review generated mail), bin/phd debug taskmaster (to run daemons freely) and bin/worker execute --id <id> (to repeatedly test a specific piece of code after identifying an issue).

With one-mail-per-recipient on (default):

  • Sent mail to multiple users.
  • Verified mail showed up in mail list-outbound.
  • Examined mail with mail show-outbound.
  • Added a project that a subscriber could not see.
    • Verified it was not present in X-Phabricator-Projects.
    • Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
  • Added a subscriber, then changed the object policy so they could not see it and sent mail.
    • Verified I received mail but the other user did not.
  • Enabled public replies and verified mail generated with public addresses.
  • Disabld public replies and verified mail generated with private addresses.

With one-mail-per-recipient off:

  • Verified that one mail is sent to all recipients.
  • Verified users who can not see the object are still filtered.
  • Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
  • Enabled public replies and verified the mail generated with "Reply To".

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Build separate mail for each recipient, honoring recipient access levels.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
226

I locked this because changing it now violates policies.

Still needs actual testing and some stuff came up so I might not get to it tonight.

  • Minor technical fixes from testing.
carlsverre added inline comments.
src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
226

Just to understand this code a bit, setLocked here means that the config must be set via the command line config tool rather than the UI?

src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
226

That's correct. You can set it with bin/config set metamta.one-mail-per-recipient false, although your existing database setting will still be honored.

I didn't test it extensively but I believe there was a bug with this in the removed code, so I'd expect that upgrading will fix things for you without additional changes (except for a daemon restart) once this lands.

src/applications/config/option/PhabricatorMetaMTAConfigOptions.php
226

Excellent, looking forward to doing the upgrade.

btrahan awarded a token.
btrahan edited edge metadata.
This revision is now accepted and ready to land.Jun 3 2015, 11:10 PM
This revision was automatically updated to reflect the committed changes.