Page MenuHomePhabricator

Herald - add support for application emails.
ClosedPublic

Authored by btrahan on Jan 29 2015, 7:13 PM.
Tags
None
Referenced Files
F15414416: D11564.id.diff
Thu, Mar 20, 12:31 AM
F15414057: D11564.id27822.diff
Wed, Mar 19, 10:51 PM
F15411323: D11564.diff
Wed, Mar 19, 9:00 AM
F15411266: D11564.diff
Wed, Mar 19, 8:57 AM
F15411187: D11564.diff
Wed, Mar 19, 8:53 AM
F15411054: D11564.diff
Wed, Mar 19, 8:48 AM
F15407447: D11564.diff
Tue, Mar 18, 5:34 PM
F15404316: D11564.diff
Tue, Mar 18, 7:13 AM
Subscribers
Tokens
"Pterodactyl" token, awarded by joshuaspence.

Details

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

Diff Detail

Repository
rP Phabricator
Branch
T5039
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4158
Build 4171: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

btrahan retitled this revision from to Herald - add support for application emails..
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.
epriestley edited edge metadata.

One inline, I think.

src/applications/herald/adapter/HeraldAdapter.php
188

Could this fatal for tasks not created via an application email?

i.e., Maniphest -> Create Task -> Fatal: calling getPHID() on a non-object?

332

This label might not be very obvious. Maybe, "Receiving email address" or "Via email to"? Not sure if those are better.

This revision is now accepted and ready to land.Jan 29 2015, 9:56 PM
btrahan edited edge metadata.

changes as suggested

This revision was automatically updated to reflect the committed changes.
src/applications/maniphest/mail/ManiphestReplyHandler.php
174

I made a problem here since I use a typehint...

Should I either

  • conditionally set

OR

  • default it to null?

I like setting conditionally too, it feels more explicit and cleaner to me.