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.

Event Timeline

fabe updated this revision to Diff 27202.Jan 11 2015, 5:01 PM
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
fabe added a comment.Jan 11 2015, 5:07 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.

fabe added inline comments.Jan 11 2015, 5:12 PM
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 updated this revision to Diff 27431.Jan 16 2015, 12:51 PM
fabe edited edge metadata.
  • further refactoring. adding feeds to workers and a lot of cleanup.
  • provide a migration path for the other applications
fabe updated this revision to Diff 27432.Jan 16 2015, 12:58 PM
  • just remove some lint warnings. makes the diff easier to read
fabe planned changes to this revision.May 25 2015, 12:41 PM

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.

epriestley edited edge metadata.Jun 2 2015, 12:52 AM

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.

fabe abandoned this revision.Jun 5 2015, 9:09 AM

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.