Render emails per user receiving it in a worker task. This allows for recipient based permission rendering
and (to be added later) using the recipients locale.
Details
Changed maniphest task permissions, commented on it and received/not-received email notification depending on the permissions set.
Also added projects as subscribers and embedded tasks/repos into comment specific users on the subscription list did not have access to.
Diff Detail
- Repository
- rP Phabricator
- Branch
- ccrightsnoemail
- Lint
Lint Passed Severity Location Code Message Advice src/applications/maniphest/editor/ManiphestTransactionEditor.php:436 XHP16 TODO Comment Advice src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2150 XHP16 TODO Comment Advice src/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:234 XHP16 TODO Comment Advice src/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:353 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 4013 Build 4026: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
Basically i've just moved some code around. (I've just added comments to the old stuff so you can see where it comes from)
All the mail functions from ApplicationTransactionEditor went into the ApplicationTransactionEmailWorker
The mail functions from ManiphestTransactionEditor into ManiphestTransactionEmailWorker.
The actual changes are mainly in the TransactionEmailWorker::doWork() (which now uses a loop per user to render templates).
and queueing the worker task in ApplicationTransactionEditor::sendMail()
This currently works for me and will correctly render tasks/repos that are accessible to only some users on the notification list accordingly.
(If a user doesen't have permission she'll see "restricted project" or only a task number).
Currently broken is still the feedpublish stuff in the ApplicationTransactionEditor.
If this generally idea sounds good i'd just have to move the mail stuff from the other classes extending the ApplicationTransactionEditor and using emails (about 15) into workers.
src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php | ||
---|---|---|
200 | This was from the previous patch is no longer needed for this change as multiplexMail is no longer in use form ApplicationTransactions. |
- further refactoring. adding feeds to workers and a lot of cleanup.
- provide a migration path for the other applications
This needs a major rebase.
Let me know when/if you are interested to move on this and if you think refactoring the stuff into workers is the right approach.
I finally have some time to work on this, sorry it took so long! I generally like this approach, but want to try to make the change more gradually. In particular:
- I'm looking at doing daemons and mail changes separately, so we move to the daemons first and then start splitting mail out.
- Although it might make sense to move mail code out of TransactionEditors eventually, this doesn't feel very important to me. The classes are sometimes pretty large (and the base class is enormous) but the code feels fairly cohesive and consistent to me. I think we can leave the code where it is for now, and not have to introduce new classes in each application.
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php | ||
---|---|---|
1919–1928 | This is a good catch, I missed it! | |
1943 | This one too. |
The more gradual implementation is probably best. This diff spiralled a bit out of control.
However i'd still vote for the separate Editor / Publisher classes just for the sake of read/maintainability.
The automatic state save/load to transfer it to the workers feels quite dirty.