Page MenuHomePhabricator

Refactor email rendering into worker tasks
AbandonedPublic

Authored by fabe on Jan 11 2015, 5:01 PM.
Tags
None
Referenced Files
F13060971: D11329.id.diff
Fri, Apr 19, 6:40 PM
Unknown Object (File)
Wed, Apr 17, 3:25 PM
Unknown Object (File)
Thu, Apr 4, 2:30 PM
Unknown Object (File)
Tue, Apr 2, 2:23 PM
Unknown Object (File)
Tue, Apr 2, 5:22 AM
Unknown Object (File)
Fri, Mar 29, 1:12 PM
Unknown Object (File)
Mar 3 2024, 7:03 AM
Unknown Object (File)
Feb 3 2024, 1:53 AM

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/transactions/editor/PhabricatorApplicationTransactionEditor.php:1913TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:45XHP9Naming Conventions
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:51XHP9Naming Conventions
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:57TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:72XHP9Naming Conventions
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:73XHP9Naming Conventions
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:113TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:147TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:148TXT3Line Too Long
Warningsrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:149TXT3Line Too Long
Advicesrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:151XHP16TODO Comment
Advicesrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:239XHP16TODO Comment
Advicesrc/applications/transactions/worker/PhabricatorApplicationTransactionEmailWorker.php:282XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 3760
Build 3771: [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
1915–1924

This is a good catch, I missed it!

1939

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.