Summary: Fixes T7088. Mainly this updates the documentation but I also snuck in tweaking how the domain reply handler is built. This does two main things -- makes the behavior consistent as some applications who didn't override this behavior would send out emails with reply tos AND makes it easier for us to deprecate the custom domain thing on a per application basis, which is just silly. On that note, the main documentation doesn't get into how this can be overridden, though I left in that mini blurb on the config setting itself. We could deprecate this harder and LOCK things if you want as well.
Test Plan: read docs, looked good. reasoned through re-factor
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7088
Differential Revision: https://secure.phabricator.com/D11725
Summary: Ref T7094. This one is really straight-forward since $this->actor is always populated and the right thing to do here.
Test Plan: used the ole thinking noodle since testing email w/ attachments is really hard
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11589
Summary: Fixes T3404 (post D11565), fixes T5952. This infrastructure has been getting deployed against Maniphest and its time to get these other two applications going on it.
Test Plan: created an email address for paste and used `./bin/mail receive-test` ; a paste was successfully created
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952, T3404
Differential Revision: https://secure.phabricator.com/D11570
Summary: Ref T3404. The only mildly sketchy bit is these codepaths all load the application email directly, by-passing privacy. I think this is necessary because not getting to see an application doesn't mean you should be able to break the application by registering a colliding email address.
Test Plan:
Tried to add a registered application email to a user account via the web ui and got a pretty error.
Ran unit tests.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404
Differential Revision: https://secure.phabricator.com/D11565
Summary:
Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???"
Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective.
Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11564
Summary: Ref T5039. This will be necessary for Herald integration so users can make rules like "if app email is one of x, y, or z add projects foo, bar, and metallica." I think its best to do an actual typeahead here -- users select full email addresses -- rather than support prefix, suffix, etc stuff on the email address. I think the latter approach would yield lots of confusion, as well as prevent us from (more) easily providing diagnostic tools about what happened when and why.
Test Plan: hacked a maniphest tokenizer to use this new datasource and it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5039
Differential Revision: https://secure.phabricator.com/D11546
Summary: Ref T5952. This adds support for a "default author" and deploys it on Maniphest.
Test Plan: used augmented (by this diff) bin/mail receive-test to test creation via an application email with a default author configured and no author specified. a task was created with the author as the default author i configured.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5952
Differential Revision: https://secure.phabricator.com/D11446
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary: Ref T5952, T3404. This lays the basic plumbing for how this will work, all the way to deploying on Maniphest. Aside from what is mentioned on T5952, I think page(s) on editing application emails could use a little more helpful text about what's going on, similar to how the config page that's getting deprecated works.
Test Plan: ran migration and noted my create email address migrated successfully. used bin/mail to make a task. added another email and used bin/mail to make a task. deleted an email. edited an email. invoked various error states and they all looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T3404, T5952
Differential Revision: https://secure.phabricator.com/D11418
Summary: Ref T6822. This method is only called from within the `PhabricatorWorker::executeTask()` and `PhabricatorWorker::scheduleTask()` methods.
Test Plan: `grep`ped for `->doWork`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11406
Summary: Ref T5402. This more or less "fixes" it but there's probably some polish to do?
Test Plan:
stopped and started daemons. error logs look good.
ran bin/storage upgrade. noted that `adjust` added the appropriate indices for active and archive task.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11044
Summary: Ref T1217, Add link to email preferences to email template
Test Plan: Add comment to object like Maniphest task, check that email has a footer with a link to email preferences.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T1217
Differential Revision: https://secure.phabricator.com/D10883
Summary: Ref T6343, adding HTMLMailMode to remarkup, and most objects should now be processed and appear pretty in emails.
Test Plan: Add a comment to a Maniphest task containing a mention of an object like '{T1}' or 'T1'. Emails should show a styled version of the object similar to how the object looks in the context of the Maniphest task in the UI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T6343, T2617
Differential Revision: https://secure.phabricator.com/D10859
Summary: Ref T6576. This avoids generating almost-empty HTML mail bodies for mail which incorrectly has no HTML body.
Test Plan: Generated some mail locally; the specific hook case is a pain for me to hit right now. Will push and dig in if that doesn't fix it.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T6576
Differential Revision: https://secure.phabricator.com/D10863
Summary: Fixes T6525, adds cc and tos to html emails
Test Plan: send html and plain emails, see new stuff
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6525
Differential Revision: https://secure.phabricator.com/D10857
Summary: Fixes T6343. Grepped for all callsites and added addLinkSection where needed.
Test Plan: Tested Differential, Maniphest, Conpherence, Ponder and Macro. Inspect HTML mail for anchor tags. Inspect text mails for non-disruption.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: talshiri, Korvin, epriestley
Maniphest Tasks: T6343
Differential Revision: https://secure.phabricator.com/D10762
Summary: Fixes T6372. Apparently ye olde error logs get some crazy spam action as is... Looking around at call sites, we do not specify $config (which could specify the supportage of message id header) so it seems correct to default this to something. I went with "true" as the spot we use this seems like pretty easy stuff that will always work.
Test Plan: lots of thinking
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6372
Differential Revision: https://secure.phabricator.com/D10749
Summary:
Ref T2787. When a user purchases a product in Phortune, transition the cart through a purchased state and invoke product callbacks so applications can respond to the workflow.
Also shore up some stuff like preventing negative amounts of funding.
Test Plan: Backed an initiative and saw it show up on the initiative after completing the purcahsing workflow.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D10635
Summary: thanks mailbox
Test Plan: unit tests
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10629
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary: Ref T1191. Handful of minor things here (T6150, T6149, T6148, T6147, T6146) but nothing very noteworthy.
Test Plan: Viewed web UI, saw fewer errors.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10527
Summary:
Fixes T5956. We changed the default mail encoding to `quoted-printable` to fix delivery via SendGrid via SMTP, but this broke multiple other mailers.
- Change the default back to 8bit (which works everywhere except SendGrid).
- Add a configuration setting for selecting `quoted-printable`.
- Document this issue.
- Discourage use of SendGrid in documentation.
(IMPORTANT) @klimek @nickz This reverts the `quoted-printable` fix for SendGrid. You will need to adjust your configurations (set `phpmailer.smtp-encoding` to `quoted-printable`) and restart your daemons or mail will get double newlines again.
Test Plan:
- Sent mail via SendGrid with various `phpmailer.smtp-encoding` settings, saw mail arrive with specified encoding.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: klimek, nickz, epriestley
Maniphest Tasks: T5956
Differential Revision: https://secure.phabricator.com/D10397
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: Ref T992. This makes HTML mail layout more consistent with text mail layout and fixes my greatest annoyance with it.
Test Plan: Used `bin/mail list-outbound --id <id> --dump-html` to view mail in Safari, saw it have a normal amount of whitespace between sections.
Reviewers: btrahan, talshiri, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10344
Summary: Clean up some arg handling stuff.
Test Plan: Used this while debugging.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10314
Summary:
Ref T992.
- Format text/HTML bodies explicitly in `bin/mail show-outbound`.
- Provide `bin/mail show-outbound --dump-html` so you can do something like `bin/mail show-outbound --dump-html > dump.html; open dump.html` to get a browser preview somewhat easily.
Test Plan: Ran `bin/mail show-outbound` with and without `--dump-html` flag.
Reviewers: talshiri, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D10272
Summary:
Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Test Plan:
Been running this in our deployment for a few months now.
====Well behaved clients====
- Gmail
- Mail.app
====Bad clients====
- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.
====Need testing====
- Outlook (Windows + Mac)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: webframp, taoqiping, chad, epriestley, Korvin
Maniphest Tasks: T992
Differential Revision: https://secure.phabricator.com/D9375
Summary: Fixes T5769. Fixes T5861. Add mail tags for "unblock" and "column change".
Test Plan: Did unblocks and column changes, verified the mail got the right mailtags and recipient nondelivery flags.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5861, T5769
Differential Revision: https://secure.phabricator.com/D10241
Summary:
Ref T5861. Ref T5769. If users don't care at all about something, allow them to ignore it.
We have some higher-volume notifications either built now (column changes) or coming (mentions) which users might reasonably want to ignore completely.
Test Plan:
Ignored some notifications, then took appropraite actions. Saw my user culled from the notification subscriber list.
{F189531}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5769, T5861
Differential Revision: https://secure.phabricator.com/D10240
Summary: Ref T5861. Adds an option to opt out of all notification email. We'll still send you password resets, email verifications, etc.
Test Plan:
{F189484}
- Added unit tests.
- With preference set to different things, tried to send myself mail. Mail respected preferences.
- Sent password reset email, which got through the preference.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: rush898, epriestley
Maniphest Tasks: T5861
Differential Revision: https://secure.phabricator.com/D10237
Summary:
Fixes T5185. The fundamental issue is that this `excludePHIDs` property was not saved, so the logic went like this:
- Generate `excludePHIDs` correctly.
- Pass `excludePHIDs` through the stack.
- Perform some other computations correctly.
- Queue the mail for the daemons, throwing it away. {icon bomb}
- Daemons process mail with empty `excludePHIDs` list.
Store it in the persistent properties array instead.
Also remove the "override self mail" thing, since it's only used by `bin/mail send-test` and suffers from the same issue. I think it's too useless to fix, since even if you get caught by it, `bin/mail` makes it clear why the message was dropped.
Test Plan:
Notable:
- `exclude` present in properties
- Exclusion reason under RECIPIENTS header
{P1229}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5185
Differential Revision: https://secure.phabricator.com/D10234
Summary:
Fixes T5233.
- The mail adapter API currently expects plain addresses (like `a@b.com`) in `addTos()`, and some adapters can not accept fancy verbose addresses (like `"name" <a@b.com>`).
- When we try to send error email, we pass the entire "From" header into the API. This is incorrect.
- Since it would be nice to make this just work in the future, fix it inside the API.
- Specifically, this is reached with: send email -> generates error -> we try to send you an email back -> we send it to your "From" -> some mailers choke on the fancy name if you have one.
Test Plan: Processed an errorneous email with a fancy "From", got a response error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5233
Differential Revision: https://secure.phabricator.com/D10232
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary:
Some mailers remove the duplicate entries themselves, but some (Mailgun) don't.
This affects installations with metamta.one-mail-per-recipient set to false, and will cause
- ugly looking "to" entries. Gmail, for example, collapses to+cc entries to one list, so you get something that looks like "to: me me john"
- It sometimes causes duplicate delivery of the same message when used in conjuction with Google Groups. I suspect that their message de-dup mechanism is confused by it (I fuzzed it directly with Mailgun, and saw the same message delivered twice - once directly through mailgun, and bounced again through Google Groups). This doesn't happen when the entries are not duplicated.
Test Plan: Created some tasks. Added subscribers. Things seem to work reasonably well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9978
Summary: Fixes T5336. Currently, `PhabricatorWorkerLeaseQuery` is basically FIFO. It makes more sense for the queue to be a priority-queue, and to assign higher priorities to alerts (email and SMS).
Test Plan: Created dummy tasks in the queue (with different priorities). Verified that the priority field was set correctly in the DB and that the priority was shown on the `/daemon/` page. Started a `PhabricatorTaskmasterDaemon` and verified that the higher priority tasks were executed before lower priority tasks.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5336
Differential Revision: https://secure.phabricator.com/D9871
Summary:
Fixes T3732. Ref T1205. Ref T3116.
External accounts (like emails used as identities, Facebook accounts, LDAP accounts, etc.) are stored in "ExternalAccount" objects.
Currently, we have a very restrictive `CAN_VIEW` policy for ExternalAccounts, to add an extra layer of protection to make sure users can't use them in unintended ways. For example, it would be bad if a user could link their Phabricator account to a Facebook account without proper authentication. All of the controllers which do sensitive things have checks anyway, but a restrictive CAN_VIEW provided an extra layer of protection. Se T3116 for some discussion.
However, this means that when grey/external users take actions (via email, or via applications like Legalpad) other users can't load the account handles and can't see anything about the actor (they just see "Restricted External Account" or similar).
Balancing these concerns is mostly about not making a huge mess while doing it. This seems like a reasonable approach:
- Add `CAN_EDIT` on these objects.
- Make that very restricted, but open up `CAN_VIEW`.
- Require `CAN_EDIT` any time we're going to do something authentication/identity related.
This is slightly easier to get wrong (forget CAN_EDIT) than other approaches, but pretty simple, and we always have extra checks in place anyway -- this is just a safety net.
I'm not quite sure how we should identify external accounts, so for now we're just rendering "Email User" or similar -- clearly not a bug, but not identifying. We can figure out what to render in the long term elsewhere.
Test Plan:
- Viewed external accounts.
- Linked an external account.
- Refreshed an external account.
- Edited profile picture.
- Viewed sessions panel.
- Published a bunch of stuff to Asana/JIRA.
- Legalpad signature page now shows external accounts.
{F171595}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3732, T1205, T3116
Differential Revision: https://secure.phabricator.com/D9767
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: The adapter was mostly copy-paste, and I missed the supportsMessageIDHeader stuff.
Test Plan: Sent a message, checked headers.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9715
