Page MenuHomePhabricator

Refactor email rendering into worker tasks
AbandonedPublic

Authored by fabe on Jan 11 2015, 5:01 PM.

Details

Summary

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.

Test Plan

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 Warnings
SeverityLocationCodeMessage
Warningsrc/applications/maniphest/worker/PhabricatorManiphestTransactionNotificationWorker.php:37TXT3Line Too Long
Warningsrc/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1905TXT3Line Too Long
Warningsrc/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1935TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:96TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:118TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:141TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:142TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:179TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:213TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:215TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:216TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:403TXT3Line Too Long
Advicesrc/applications/maniphest/editor/ManiphestTransactionEditor.php:436XHP16TODO Comment
Advicesrc/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2148XHP16TODO Comment
Advicesrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:218XHP16TODO Comment
Advicesrc/applications/transactions/worker/PhabricatorApplicationTransactionNotificationWorker.php:337XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 3884
Build 3896: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

fabe retitled this revision from to Refactor email rendering into worker tasks.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
fabe added a reviewer: Blessed Reviewers.
fabe added a subscriber: epriestley.
fabe planned changes to this revision.Jan 11 2015, 5:01 PM

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.
We could still leave it in for code that uses this function directly.

fabe edited edge metadata.
  • further refactoring. adding feeds to workers and a lot of cleanup.
  • provide a migration path for the other applications
  • just remove some lint warnings. makes the diff easier to read

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.