1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 02:32:42 +01:00
Commit graph

15403 commits

Author SHA1 Message Date
epriestley
d45952344b Use setOptions() to trigger mailer option validation, not validateOptions() 2018-02-08 17:55:02 -08:00
epriestley
09b446b269 Don't run older mail setup checks if "cluster.mailers" is configured
Summary: Ref T12677. Skip these checks if we're doing the new stuff. Also, allow priority to be unspecified.

Test Plan: Will deploy.

Maniphest Tasks: T12677

Differential Revision: https://secure.phabricator.com/D19043
2018-02-08 17:51:32 -08:00
epriestley
7d4362690f Fix transposed name/email in Mailgun adapter
Summary: Ref T12677. This argument order was swapped.

Test Plan: Will push/verify.

Maniphest Tasks: T12677

Differential Revision: https://secure.phabricator.com/D19042
2018-02-08 17:49:32 -08:00
epriestley
261a4a0e51 Add inline comment counts to the filetree view
Summary: See PHI356. Adds inline comment and done counts to the filetree. Also makes the filetree wider by default.

Test Plan: Fiddled with filetrees in different browsers on different revisions. Added inlines, marked them done/undone.

Differential Revision: https://secure.phabricator.com/D19041
2018-02-08 17:15:36 -08:00
epriestley
6ea1b8df9b Colorize filetree for adds, moves, and deletes
Summary: See PHI356. Makes it easier to pick out change types in the filetree view in Differential.

Test Plan: Created a diff with adds, copies, moves, deletions, and binary files. Viewed in Differential, had an easier time picking stuff out.

Differential Revision: https://secure.phabricator.com/D19040
2018-02-08 16:11:35 -08:00
epriestley
e26a784dcf Allow tasks to be filtered and ordered by closed date
Summary: Depends on D19038. Fixes T4434. Updates the SearchEngine and Query to handle these fields.

Test Plan: Filtered and ordered by date and closer.

Maniphest Tasks: T4434

Differential Revision: https://secure.phabricator.com/D19039
2018-02-08 15:42:26 -08:00
epriestley
4c4707e467 Provide task closed date via Conduit API, data export pipeline, and in list UI
Summary:
Depends on D19037. Ref T4434. Adds closed date to `maniphest.search` and "Export Data".

When a task has been closed, show the closed date with a checkmark in the UI instead of the modified date.

Test Plan:
  - Exported data to CSV, saw close information.
  - Saw close information in `/maniphest/`.
  - Queried for close information via `maniphest.search`.

Maniphest Tasks: T4434

Differential Revision: https://secure.phabricator.com/D19038
2018-02-08 15:41:54 -08:00
epriestley
f028aa6f60 Track closed date and closing user for tasks explicitly
Summary:
Ref T4434. Although some of the use cases for this data are better fits for Facts, this data is reasonable to track separately.

I have an approximate view of it already ("closed, ordered by date modified") that's useful to review things that were fixed recently. This lets us make that view more effective.

This just adds (and populates) the storage. Followups will add Conduit, Export, Search, and UI support.

This is slightly tricky because merges work oddly (see T13020).

Test Plan:
  - Ran migration, checked database for sensible results.
  - Created a task in open/closed status, got the right database values.
  - Modified a task to close/open it, got the right values.
  - Merged an open task, got updates.

Maniphest Tasks: T4434

Differential Revision: https://secure.phabricator.com/D19037
2018-02-08 15:40:49 -08:00
epriestley
d1e273daf6 Remove completely pointless load of every repository when viewing a repository URI
Summary:
See D18176. This query has no effect (other than wasting resources) and the result is unused.

`$repository` already has the URI loaded because we load them unconditionally during request initialization.

Test Plan: Viewed repository URIs.

Subscribers: jmeador

Differential Revision: https://secure.phabricator.com/D19036
2018-02-08 12:47:48 -08:00
epriestley
705ff8d33d Remove addHighlight() action view methods
Summary: These didn't actually get used by D19033.

Test Plan: Grep.

Differential Revision: https://secure.phabricator.com/D19034
2018-02-08 12:15:24 -08:00
epriestley
ab04d2179b Add "Mute/Unmute" for subscribable objects
Summary: Ref T13053. See PHI126. Add an explicit "Mute" action to kill mail and notifications for a particular object.

Test Plan: Muted and umuted an object while interacting with it. Saw mail route appropriately.

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19033
2018-02-08 11:06:22 -08:00
epriestley
0402a79e0e Render object remarkup references in a text context as "Dxxx <uri>"
Summary:
Depends on D19031. Fixes T11389. Currently, we render `Dxxx` in a text context (plain text email) as just a URI.

Instead, render it like `Dxxx <uri>`. This is more faithful to the original intent and preserves `T123/T456` as two separate, usable links.

Test Plan: Wrote `T123/T234` in a task, pulled mail for it with `bin/mail show-outbound`, saw separate clickable links.

Maniphest Tasks: T11389

Differential Revision: https://secure.phabricator.com/D19032
2018-02-08 09:52:41 -08:00
epriestley
bca9c08953 Add an "Acting user" field to Herald
Summary:
Ref T13053. Fixes T7804. Adds "Acting user" so you can have "always email me" stuff skip things you did or keep an eye on suspicious interns.

For the test console, the current user is the acting user.

For pushes, the pusher is the acting user.

Test Plan: Wrote acting user rules, triggered them via test console and via multiple actors on real objects.

Maniphest Tasks: T13053, T7804

Differential Revision: https://secure.phabricator.com/D19031
2018-02-08 09:52:18 -08:00
epriestley
6186f0aa91 Briefly document mail stamps and remove obsolete header documentation
Summary: Fixes T10189. Ref T13053. We haven't sent these headers in a very long time. Briefly mention the new stamps header instead, although I expect to integrate stamp documentation into the UI in a more cohesive way in the future.

Test Plan: Read documentation.

Maniphest Tasks: T13053, T10189

Differential Revision: https://secure.phabricator.com/D19030
2018-02-08 09:31:12 -08:00
epriestley
bae9f459ab Pass HTML bodies to push email
Summary: Depends on D19028. Ref T13053. Fixes T6576. An HTML body was built here, but not passed to the actual mail message.

Test Plan: Will verify production push mail.

Maniphest Tasks: T13053, T6576

Differential Revision: https://secure.phabricator.com/D19029
2018-02-08 09:12:03 -08:00
epriestley
a8f937d313 Only add the Mail "STAMPS" body section if there are stamps
Summary: Ref T13053. Some mail (like push notification mail) doesn't currently generate any stamps. Drop this section if there aren't any stamps on the mail.

Test Plan: Will check push mail in production.

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19028
2018-02-08 09:09:28 -08:00
epriestley
942b17a980 Improve correctness of email address escaping in Mailgun/Postmark
Summary: Ref T13053. Uses the changes in D19026 to escape mail addresses. Those rules may not be right yet, but they're at least all in one place, have test coverage, and aren't obviously incorrect.

Test Plan: Will vet this more extensively when re-testing all mailers.

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19027
2018-02-08 09:02:50 -08:00
epriestley
948b0ceca4 Configure a whitelist of remote addresses for Postmark inbound webhooks
Summary:
Ref T13053. Postmark support recommends testing requests against a whitelist of known remote addresses to determine request authenticity. Today, the list can be found here:

<https://postmarkapp.com/support/article/800-ips-for-firewalls>

This is potentially less robust than, e.g., HMAC verification, since they may need to add new datacenters or support IPv6 or something. Users might also have weird network topologies where everything is proxied, and this makes testing/simulating more difficult.

Allow users to configure the list so that they don't need to hack things apart if Postmark adds a new datacenter or remote addresses are unreliable for some other reason, but ship with safe defaults for today.

Test Plan:
Tried to make local requests, got kicked out. Added `0.0.0.0/0` to the list, stopped getting kicked out.

I don't have a convenient way to route real Postmark traffic to my development laptop with an authentic remote address so I haven't verified that the published remote address is legitimate, but I'll vet that in production when I go through all the other mailers.

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19025
2018-02-08 08:23:14 -08:00
epriestley
2bb4fc9ece Fix a Phortune billing issue where subscription autopay could charge disabled cards
Summary:
See support email. There's nothing tricky here, we were just missing a check. The different parts of this got built at different times so I think this was simply overlooked.

Also add a redundant check just to future-proof this and be on the safe side.

Test Plan: Used `bin/phortune invoice` to charge a pact subscription. After deleting the card, the charge failed with an appropriate error.

Reviewers: amckinley

Differential Revision: https://secure.phabricator.com/D19020
2018-02-08 06:30:59 -08:00
epriestley
d0a2e3c54f Fix an issue where some Differential edit pathways may not have reviewers attached
Summary:
Depends on D19021. Ref T13053. When you "Subscribe", or make some other types of edits, we don't necessarily have reviewer data, but may now need it to do the new recipient list logic.

I don't have a totally clean way to deal with this in the general case in mind, but just load it for now so that things don't fatal.

Test Plan: Subscribed to a revision with the "Subscribe" action.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19022
2018-02-08 06:30:00 -08:00
epriestley
1cd3a59378 When users resign from revisions, stop expanding projects/packages to include them
Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.

Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.

When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).

`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.

Test Plan:
  - Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
  - Resigned as ducker, stopped getting future mail.
  - Subscribed explicitly, got mail again.
  - (Plus some `var_dump()` sanity checking in the internals.)

Reviewers: amckinley

Maniphest Tasks: T13053, T12689

Differential Revision: https://secure.phabricator.com/D19021
2018-02-08 06:29:13 -08:00
epriestley
f214abb63f When a change removes recipients from an object, send them one last email
Summary:
Depends on D19018. Fixes T4776. Ref T13053. When you remove someone from an object's recipient list (for example, by removing them a reviewer, auditor, subscriber, owner or author) we currently do not send them mail about it because they're no longer connected to the object.

In many of these cases (Commandeer, Reassign) the actual action in the UI adds them back to the object somehow (as a reviewer or subscriber, respectively) so this doesn't actually matter. However, there's no recovery mechanism for reviewer or subscriber removal.

This is slightly bad from a policy/threat viewpoint since it means an attacker can remove all the recipients of an object "somewhat" silently. This isn't really silent, but it's less un-silent than it should be.

It's also just not very good from a human interaction perspective: if Alice removes Bob as a reviewer, possibly "against his will", he should be notified about that. In the good case, Alice wrote a nice goodbye note that he should get to read. In the bad case, he should get a chance to correct the mistake.

Also add a `removed(@user)` mail stamp so you can route these locally if you want.

Test Plan:
  - Created and edited some different objects without catching anything broken.
  - Removed subscribers from tasks, saw the final email include the removed recipients with a `removed()` stamp.

I'm not totally sure this doesn't have any surprising behavior or break any weird objects, but I think anything that crops up should be easy to fix.

Reviewers: amckinley

Subscribers: sophiebits

Maniphest Tasks: T13053, T4776

Differential Revision: https://secure.phabricator.com/D19019
2018-02-08 06:28:11 -08:00
epriestley
dbe479f0d9 Don't send error/exception mail to unverified addresses
Summary:
Depends on D19017. Fixes T12491. Ref T13053. After SES threw us in the dungeon for sending mail to a spamtrap we changed outbound mail rules to stop sending to unverified addresses, except a small amount of registration mail which we can't avoid.

However, we'll still reply to random inbound messages with a helpful error, even if the sender is unverified.

Instead, only send exception mail back if we know who the sender is.

Test Plan: Processed inbound mail with `scripts/mail/mail_handler.php`. No more outbound mail for "bad address", etc. Still got outbound mail for "unknown command !quack".

Reviewers: amckinley

Maniphest Tasks: T13053, T12491

Differential Revision: https://secure.phabricator.com/D19018
2018-02-08 06:26:16 -08:00
epriestley
5792032dc9 Support Postmark inbound mail via webhook
Summary: Depends on D19016. Ref T13053. Adds a listener for the Postmark webhook.

Test Plan:
Processed some test mail locally, at least:

{F5416053}

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19017
2018-02-08 06:25:26 -08:00
epriestley
0986c7f673 Add a "View Object" button on the web mail view page
Summary:
Depends on D19015. Ref T13053. Currently, we don't link up hyperlinks in the body of mail viewed in the web UI. We should, but this is a little tricky (see T13053#235074).

As a general improvement to make working with "Must Encrypt" mail less painful, add a big button to jump to the related object.

Test Plan: {F5415990}

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19016
2018-02-08 06:24:19 -08:00
epriestley
085221b0d6 In HTML mail, make the text for mail stamps in mail bodies smaller and lighter
Summary: Depends on D19014. Ref T13053.

Test Plan: Used `./bin/mail show-outbound --id <id> --dump-html > out.html && open out.html` to look at HTML mail, saw smaller, lighter stamp text with better spacing.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19015
2018-02-08 06:23:37 -08:00
epriestley
6e5df2dd71 Document that disabling "metamta.one-mail-per-recipient" leaks recipients for "Must Encrypt"
Summary:
Depends on D19013. Ref T13053. When mail is marked "Must Encrypt", we normally do not include recipient information.

However, when `metamta.one-mail-per-recipient` is disabled, the recipient list will leak in the "To" and "Cc" headers. This interaction is probably not very surprising, but document it explicitly for completeness.

(Also use "Mail messages" instead of "Mails".)

Test Plan: Read documentation in the "Config" application.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19014
2018-02-08 06:23:08 -08:00
epriestley
aa74af1983 Remove all "originalTitle"/"originalName" fields from objects
Summary:
Depends on D19012. Ref T13053. In D19012, I've changed "Thread-Topic" to always use PHIDs.

This change drops the selective on-object storage we have to track the original, human-readable title for objects.

Even if we end up backing out the "Thread-Topic" change, we'd be better off storing this in a table in the Mail app which just has `<objectPHID, first subject we used when sending mail for that object>`, since then we get the right behavior without needing every object to have this separate field.

Test Plan: Grepped for `original`, `originalName`, `originalTitle`, etc.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19013
2018-02-08 06:22:03 -08:00
epriestley
f090fa7426 Use object PHIDs for "Thread-Topic" headers in mail
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.

I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.

In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.

Then allow this header through for "Must Encrypt" mail.

Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.

Reviewers: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19012
2018-02-08 06:21:00 -08:00
epriestley
19b3fb8863 Add a Postmark mail adapter so it can be configured as an outbound mailer
Summary: Depends on D19007. Ref T12677.

Test Plan: Used `bin/mail send-test ... --mailer postmark` to deliver some mail via Postmark.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12677

Differential Revision: https://secure.phabricator.com/D19009
2018-02-08 06:18:23 -08:00
epriestley
1f53aa27e4 Add unit tests for mail failover behaviors when multiple mailers are configured
Summary: Depends on D19006. Ref T13053. Ref T12677. When multiple mailers are configured but one or more fail, test that we recover (or don't) appropriately.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19007
2018-02-08 06:17:49 -08:00
epriestley
9947eee182 Add some test coverage for mailers configuration
Summary: Depends on D19005. Ref T12677. Ref T13053. Tests that turning `cluster.mailers` into an actual list of mailers more or less works as expected.

Test Plan: Ran unit tests.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19006
2018-02-08 06:17:19 -08:00
epriestley
994d2e8e15 Use "cluster.mailers" if it is configured
Summary:
Depends on D19004. Ref T13053. Ref T12677. If the new `cluster.mailers` is configured, make use of it. Also use it in the Sengrid/Mailgun inbound stuff.

Also fix a bug where "Must Encrypt" mail to no recipients could fatal because no `$mail` was returned.

Test Plan: Processed some mail locally. The testing on this is still pretty flimsy, but I plan to solidify it in an upcoming change.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19005
2018-02-08 06:12:54 -08:00
epriestley
4236952cdb Add a bin/config set <key> --stdin < value.json flag to make CLI configuration of complex values easier
Summary:
Depends on D19003. Ref T12677. Ref T13053. For the first time, we're requiring CLI configuration of a complex value (not just a string, integer, bool, etc) to do something fairly standard (send mail).

Users sometimes have very reasonable difficulty figuring out how to `./bin/config set key <some big JSON mess>`. Provide an easy way to handle this and make sure it gets appropriate callouts in the documentation.

(Also, hide the `cluster.mailers` value rather than just locking it, since it may have API keys or SMTP passwords.)

Test Plan: Read documentation, used old and new flags to set configuration.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19004
2018-02-08 06:09:09 -08:00
epriestley
c868ee9c07 Introduce and document a new cluster.mailers option for configuring multiple mailers
Summary:
Depends on D19002. Ref T13053. Ref T12677. Adds a new option to allow configuration of multiple mailers.

Nothing actually uses this yet.

Test Plan: Tried to set it to various bad values, got reasonable error messages. Read documentation.

Reviewers: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19003
2018-02-08 06:08:34 -08:00
epriestley
7f2c90fbd1 Prepare for multiple mailers of the same type
Summary:
Depends on D19000. Ref T13053. Ref T12677. Currently, most mailers are configured with a bunch of `<mailer>.setting-name` global config options.

This means that you can't configure two different SMTP servers, which is a reasonable thing to want to do in the brave new world of mail failover.

It also means you can't configure two Mailgun accounts or two SES accounts. Although this might seem a little silly, we've had more service disruptions because of policy issues / administrative error (where a particular account was disabled) than actual downtime, so maybe it's not completely ridiculous.

Realign mailers so they can take configuration directly in an explicit way. A later change will add new configuration to take advantage of this and let us move away from having ~10 global options for this stuff eventually.

(This also makes writing third-party mailers easier.)

Test Plan:
Processed some mail, ran existing unit tests. But I wasn't especially thorough.

I expect later changes to provide some tools to make this more testable, so I'll vet each provider more thoroughly and add coverage for multiple mailers after that stuff is ready.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D19002
2018-02-08 06:00:59 -08:00
epriestley
7765299f83 Mask the sender for "Must Encrypt" mail
Summary:
Depends on D18998. Ref T13053. When we send "Must Encrypt" mail, we currently send it with a normal "From" address.

This discloses a little information about the object (for example, if the Director of Silly Walks is interacting with a "must encrypt" object, the vulnerability is probably related to Silly Walks), so anonymize who is interacting with the object.

Test Plan: Processed some mail. (The actual final "From" is ephemeral and a little tricky to examine and I didn't actually transmit mail over the network, but it should be obvious if this works or not on `secure`.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D19000
2018-02-08 05:59:35 -08:00
epriestley
1485debcbd Prepare mail transmission to support failover across multiple mailers
Summary:
Ref T13053. Ref T12677. This restructures the calls and error handling logic so that we can pass in a list of multiple mailers and get retry logic.

This doesn't actually ever use multiple mailers yet, and shouldn't change any behavior. I'll add multiple-mailer coverage a little further in, since there's currently no way to effectively test which of several mailers ended up transmitting a message.

Test Plan:
  - This has test coverage; tests still pass.
  - Poked around locally doing things that send mail, saw mail appear to send. I'm not attached to a real mailer though so my confidence in local testing is only so-so.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053, T12677

Differential Revision: https://secure.phabricator.com/D18998
2018-02-08 05:49:06 -08:00
epriestley
150a04791c Fix bad NUX link in Legalpad search view
Summary:
See <https://discourse.phabricator-community.org/t/clicking-the-create-a-document-button-on-fresly-installed-phabricators-legalpad-results-in-404/1088>.

This URI isn't correct.

Test Plan: Visited {nav Use Results > New User State} in developer mode, clicked green button. Before: 404. After: taken to the edit screen.

Differential Revision: https://secure.phabricator.com/D19024
2018-02-08 05:47:02 -08:00
epriestley
a5bbadbaba Fix another Git 2.16.0 CLI compatibility issue
Summary:
This command also needs a "." instead of an empty string now.

(This powers the file browser typeahead in Diffusion.)

Test Plan: Will test in production since there's still no easy 2.16 installer for macOS.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19010
2018-02-07 17:54:39 -08:00
epriestley
56bf069080 Try running Herald when performing inverse edge edits
Summary:
Ref T13053. When you mention one object on another (or link two objects together with an action like "Edit Parent Revisions"), we write a transaction on each side to add the "alice added subtask X" and "alice added parent task Y" items to the timeline.

This behavior now causes problems in T13053 with the "Must Encrypt" flag because it prevents the flag from being applied to the corresponding "inverse edge" mail.

This was added in rP5050389f as a quick workaround for a fatal related to Editors not having enough data to apply Herald on mentions. However, that was in 2014, and since then:

  - Herald got a significant rewrite to modularize all the rules and adapters.
  - Editing got a significant upgrade in EditEngine and most edit workflows now operate through EditEngine.
  - We generally do more editing on more pathways, everything is more modular, and we have standardized how data is loaded to a greater degree.

I suspect there's no longer a problem with just running Herald here, and can't reproduce one. If anything does crop up, it's probably easy (and desirable) to fix it.

This makes Herald fire a little more often: if someone writes a rule, mentioning or creating a relationship to old tasks will now make the rule act. Offhand, that seems fine. If it turns out to be weird, we can probably tailor Herald's behavior.

Test Plan:
I wasn't able to break anything:

  - Mentioned a task on another task (original issue).
  - Linked tasks with commits, mocks, revisions.
  - Linked revisions with commits, tasks.
  - Mentioned users, revisions, and commits.
  - Verified that mail generated by creating links (e.g., Revision > Edit Tasks) now gets the "Must Encrypt" flag properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18999
2018-02-06 12:18:56 -08:00
epriestley
1bf64e5cbc Add Differential and Herald mail stamps and some refinements
Summary:
Ref T13053. Adds revision stamps (status, reviewers, etc). Adds Herald rule stamps, like the existing X-Herald-Rules header.

Removes the "self" stamps, since you can just write a rule against `whatever(@epriestley)` equivalently. If there's routing logic around this, it can live in the routing layer. This avoids tons of self-actor, self-mention, self-reviewer, self-blocking-reviewer, self-resigned-reviewer, etc., stamps.

Use `natcasesort()` instead of `sort()` so that numeric values (like monograms) sort `9, 80, 700` instead of `700, 80, 9`.

Remove the commas from rendering since they don't really add anything.

Test Plan: Edited tasks and revisions, looked at mail stamps, saw stamps that looked pretty reasonable (with no more self stuff, no more commas, sorting numbers, and Herald stamps).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18997
2018-02-06 04:06:07 -08:00
epriestley
7d475eb09a Add more mail stamps: tasks, subscribers, projects, spaces
Summary: Ref T13053. Adds task stamps plus the major infrastructure applications.

Test Plan: {F5413058}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18996
2018-02-06 04:05:46 -08:00
epriestley
3131e733a8 Add Editor-based mail stamps: actor, via, silent, encrypted, new, mention, self-actor, self-mention
Summary: Ref T13053. Adds more mail tags with information available on the Editor object.

Test Plan: Banged around in Maniphest, viewed the resulting mail, all the stamps seemed to align with reality.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18995
2018-02-06 04:04:52 -08:00
epriestley
9de54aedb5 Remove inconsistent and confusing use of the term "multiplex" in mail
Summary:
Ref T13053. Because I previously misunderstood what "multiplex" means, I used it in various contradictory and inconsistent ways.

We can send mail in two ways: either one mail to everyone with a big "To" and a big "Cc" (not default; better for mailing lists) or one mail to each recipient with just them in "To" (default; better for almost everything else).

"Multiplexing" is combining multiple signals over a single channel, so it more accurately describes the big to/cc. However, it is sometimes used to descibe the other approach. Since it's ambiguous and I've tainted it through misuse, get rid of it and use more clear language.

(There's still some likely misuse in the SMS stuff, and a couple of legitimate uses in other contexts.)

Test Plan: Grepped for `multiplex`, saw less of it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13053

Differential Revision: https://secure.phabricator.com/D18994
2018-02-06 04:04:34 -08:00
epriestley
c3f95bc410 Add basic support for mail "stamps" to improve client mail routing
Summary:
Ref T10448. Currently, we use "mail tags" (in {nav Settings > Email Preferences})  to give users some ability to route mail. There are a number of major issues with this:

  - It isn't modular and can't be extended by third-party applications.
  - The UI is a giant mess of 5,000 individual settings.
  - Settings don't map clearly to actual edits.
  - A lot of stuff isn't covered by any setting.

This adds a new system, called "mail stamps", which is similar to "mail tags" but tries to fix all these problems.

I called these "stamps" because: stamps make sense with mail; we can't throw away the old system just yet and need to keep it around for a bit; we don't use this term for anything else; it avoids confusion with project tags.

(Conceptually, imagine these as ink stamps like "RETURN TO SENDER" or "FRAGILE", not actual postage stamps.)

The only real "trick" here is that later versions of this will need to enumerate possible stamps for an object and maybe all possible stamps for all objects in the system. This is why stamp generation is separated into a "template" phase and a "value" phase. In future changes, the "template" phase can be used on its own to generate documentation and typeaheads and let users build rules. This may need some more refinement before it really works since I haven't built any of that yet.

Also adds a preference for getting stamps in the header only (default) or header and body (better for Gmail, which can't route based on headers).

Test Plan:
Fiddled with preference, sent some mail and saw a "STAMPS" setting in the body and an "X-Phabricator-Stamps" header.

{F5411694}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T10448

Differential Revision: https://secure.phabricator.com/D18991
2018-02-06 04:04:13 -08:00
epriestley
ef121b3e17 Fix a Herald repetition policy selection error for rule types which support only one policy
Summary:
Ref T13048. See <https://discourse.phabricator-community.org/t/configuring-commit-hook-commit-content-rules-fail-with-exception/1077/3>.

When a rule supports only one repetition policy (always "every time") like "Commit Hook" rules, we don't render a control for `repetition_policy` and fail to update it when saving.

Before the changes to support the new "if the rule did not match the last time" policy, this workflow just defaulted to "every time" if the input was invalid, but this was changed by accident in D18926 when I removed some of the toInt/toString juggling code.

(This patch also prevents users from fiddling with the form to create a rule which evaluates with an invalid policy; this wasn't validated before.)

Test Plan:
  - Created new "Commit Hook" (only one policy available) rule.
  - Saved existing "Commit Hook" rule.
  - Created new "Task" (multiple policies) rule.
  - Saved existing Task rule.
  - Set task rule to each repetition policy, saved, verified the save worked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

Differential Revision: https://secure.phabricator.com/D18992
2018-02-05 13:35:36 -08:00
epriestley
b3880975e5 Add aliases for "party" emoji (🎉)
Summary:
This is currently `🎉`, which I'd never have guessed.

(This isn't a super scalable approach, but this emoji is in particularly common use. See also T12644.)

Test Plan: Typed `:party`, `:confet`, etc.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18993
2018-02-05 12:23:26 -08:00
epriestley
7e6bae471c Always setlocale() to en_US.UTF-8 for the main process
Summary:
Depends on D18987. See PHI343. Fixes T13060. See also T7339.

When the main process starts up with `LANG=POSIX` (this is the default on Ubuntu) and we later try to run a subprocess with a UTF8 character in the argument list (like `git cat-file blob 🐑.txt`), the argument is not passed to the subprocess correctly.

We already set `LANG=en_US.UTF-8` in the //subprocess// environment, but this only controls behavior for the subprocess itself. It appears that the argument list encoding before the actual subprocess starts depends on the parent process's locale setting, which makes some degree of sense.

Setting `putenv('LANG=en_US.UTF-8')` has no effect on this, but my guess is that the parent process's locale setting is read at startup (rather than read anew from `LANG` every time) and not changed by further modifications of `LANG`.

Using `setlocale(...)` does appear to fix this.

Ideally, installs would probably set some UTF-8-compatible LANG setting as the default. However, this makes setup harder and I couldn't figure out how to do it on our production Ubuntu AMI after spending a reasonable amount of time at it (see T13060).

Since it's very rare that this setting matters, try to just do the right thing. This may fail if "en_US.UTF-8" isn't available, but I think warnings/remedies to this are in the scope of T7339, since we want this locale to exist for other legitimate reasons anyway.

Test Plan:
  - Applied this fix in production, processed the failing worker task from PHI343 after kicking Apache hard enough.
  - Ran locally with `setlocale(LC_ALL, 'duck.quack')` to make sure a bad/invalid/unavailable setting didn't break anything, didn't hit any issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13060

Differential Revision: https://secure.phabricator.com/D18988
2018-02-05 12:23:06 -08:00
epriestley
956c4058e6 Add a bin/conduit call support binary
Summary:
Ref T13060. See PHI343. Triaging this bug required figuring out where in the pipeline UTF8 was being dropped, and bisecting the pipeline required making calls to Conduit.

Currently, there's no easy way to debug/inspect arbitrary Conduit calls, especially when they are `diffusion.*` calls which route to a different host (even if you have a real session and use the web console for these, you just see an HTTP service call to the target host in DarkConsole).

Add a `bin/conduit` utility to make this kind of debugging easier, with an eye toward the Phacility production cluster (or other similar clusters) specifically.

Test Plan:
  - Ran `echo '{}' | bin/conduit call --method conduit.ping --input -` and similar.
  - Used a similar approach to successfully diagnose the UTF8 issue in T13060.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13060

Differential Revision: https://secure.phabricator.com/D18987
2018-02-05 12:20:49 -08:00