Page MenuHomePhabricator

Herald - add support for application emails.
ClosedPublic

Authored by btrahan on Jan 29 2015, 7:13 PM.
Tags
None
Referenced Files
F14099651: D11564.id27837.diff
Tue, Nov 26, 12:10 PM
Unknown Object (File)
Sat, Nov 23, 7:51 PM
Unknown Object (File)
Tue, Oct 29, 3:55 PM
Unknown Object (File)
Tue, Oct 29, 3:54 PM
Unknown Object (File)
Oct 20 2024, 11:12 PM
Unknown Object (File)
Oct 18 2024, 7:24 AM
Unknown Object (File)
Oct 16 2024, 4:44 AM
Unknown Object (File)
Oct 1 2024, 7:12 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.