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.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- Restricted Maniphest Task
T5952: Move object creation email address configuration to applications - Commits
- Restricted Diffusion Commit
rP53b06408f448: MetaMTA - add (basic) application emails and deploy to Maniphest
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.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Looks reasonable to me so far. Your reasoning on separate table + real PHIDs makes sense to me.
| resources/sql/autopatches/20150115.applicationemails.sql | ||
|---|---|---|
| 6 | To fix the warning, use {$COLLATE_SORT} here. Basically:
| |
Comment Actions
This is nice, and I'm really looking forward to it. A few inlines but nothing really substantive, most of it could probably be ignored or dealt with in followups.
UI-wise, it might be nice to let applications provide some kind of help or hint text somewhere in the UI, either on the main page or the edit page or both, since we don't actually say what the addresses do anywhere. Just having a one-liner like "Send mail to these addresses to create tasks. Learn More" with a link to the inbound mail docs would be good enough, I think, and then apps could customize it ("Send text to these addresses to create a paste.", etc).
| resources/sql/autopatches/20150116.maniphestapplicationemails.php | ||
|---|---|---|
| 6 | This should probably be getEnvConfigIfExists(), because getEnvConfig() will start throwing after the option is removed. | |
| 11 | Consider an initializeNew... method to reduce the chance we break this by accident if we add another property later which needs a default value, since no one is going to be able to / remember to test this. | |
| src/applications/maniphest/mail/ManiphestCreateMailReceiver.php | ||
| 12–15 | This is probably way premature, but I wonder if we should support some kind of "role" for addresses. I can't come up with a good example, which really suggests that this is ways out, but you could maaaybe imagine Calendar having, say, a "Create an event" address (event@..., "event" role) and a separate "send me a reminder" (reminder@..., "reminder" role) address. Well, that that example sucks. Nevermind. If we come up with an actual use case we could do this. | |
| src/applications/meta/controller/PhabricatorApplicationEditEmailController.php | ||
| 7–9 | Instead, just require CAN_EDIT on the application itself (you already do this below). That's sufficient, slightly simpler, and will probably be more correct in the future. Some day, when we have a use case, I imagine making that configurable -- so I could let a trusted PM reconfigure Maniphest, but only an Ops guy can reconfigure Almanac. (And then CAN_EDIT on the meta-application would control who can control who can control applications.) | |
| src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php | ||
| 8 | This is implicit if you specify AUX_PHID. | |
| 23–26 | This key is implicit if you specify AUX_PHID. | |
| 59 | We don't really need to do this, but ideally:
That would give us:
| |
| 64 | With the change above, this would also proxy. | |
| src/applications/phid/PhabricatorMetaMTAApplicationEmailPHIDType.php | ||
| 42 | Just omit this, we'll correctly leave it unlinked. | |