From a9ceebbdb116f321ff14651116139a85b101dc95 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 2 Jun 2015 06:13:04 -0700 Subject: [PATCH] Move all ApplicationTransaction publishing to daemons Summary: Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors. There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly. Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker: - This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them. - This is more correct, since we want to freeze the lists at this moment in time. Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook. --- This is a bit fragile and I'm not //thrilled// about it. It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc. Test Plan: Directly updated editors: - Created a new Phriction page, saw "Document Content". - Edited a Phriction page, saw "Document Diff". - Edited a revision, got normal looking mail. - Faked in `changedPriorToCommitURI` and verified it survived the state boundary. - Sent Audit mail. - Sent invoice mail. Indirect editors - for these, I just made a change and made sure the mail generated: - Updated a paste. - Updated an event. - Updated a thread. - Updated a task. - Updated a mock. - Updated a question. - Updated a project. - Updated a file. - Updated an initiative. - Updated a Legalpad document. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley, fabe Maniphest Tasks: T6367 Differential Revision: https://secure.phabricator.com/D13115 --- .../audit/editor/PhabricatorAuditEditor.php | 24 +++- .../editor/DifferentialTransactionEditor.php | 11 ++ .../paste/editor/PhabricatorPasteEditor.php | 4 - .../phortune/editor/PhortuneCartEditor.php | 22 +++- .../editor/PhrictionTransactionEditor.php | 48 ++++--- .../PhabricatorProjectTransactionEditor.php | 7 +- ...habricatorApplicationTransactionEditor.php | 121 +++++++++++++----- 7 files changed, 174 insertions(+), 63 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 24bc342f05..8110dffb20 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -8,6 +8,7 @@ final class PhabricatorAuditEditor private $auditReasonMap = array(); private $affectedFiles; private $rawPatch; + private $auditorPHIDs = array(); private $didExpandInlineState; @@ -342,6 +343,9 @@ final class PhabricatorAuditEditor $object->writeImportStatusFlag($import_status_flag); } + // Collect auditor PHIDs for building mail. + $this->auditorPHIDs = mpull($object->getAudits(), 'getAuditorPHID'); + return $xactions; } @@ -689,10 +693,7 @@ final class PhabricatorAuditEditor $user_phids[$committer_phid][] = pht('Committer'); } - // we loaded this in applyFinalEffects - $audit_requests = $object->getAudits(); - $auditor_phids = mpull($audit_requests, 'getAuditorPHID'); - foreach ($auditor_phids as $auditor_phid) { + foreach ($this->auditorPHIDs as $auditor_phid) { $user_phids[$auditor_phid][] = pht('Auditor'); } @@ -983,4 +984,19 @@ final class PhabricatorAuditEditor return $this->shouldPublishRepositoryActivity($object, $xactions); } + protected function getCustomWorkerState() { + return array( + 'rawPatch' => $this->rawPatch, + 'affectedFiles' => $this->affectedFiles, + 'auditorPHIDs' => $this->auditorPHIDs, + ); + } + + protected function loadCustomWorkerState(array $state) { + $this->rawPatch = idx($state, 'rawPatch'); + $this->affectedFiles = idx($state, 'affectedFiles'); + $this->auditorPHIDs = idx($state, 'auditorPHIDs'); + return $this; + } + } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 6a43f35d5a..09a7e676b2 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1902,4 +1902,15 @@ final class DifferentialTransactionEditor return $section; } + protected function getCustomWorkerState() { + return array( + 'changedPriorToCommitURI' => $this->changedPriorToCommitURI, + ); + } + + protected function loadCustomWorkerState(array $state) { + $this->changedPriorToCommitURI = idx($state, 'changedPriorToCommitURI'); + return $this; + } + } diff --git a/src/applications/paste/editor/PhabricatorPasteEditor.php b/src/applications/paste/editor/PhabricatorPasteEditor.php index c1aa39b353..732db28bd7 100644 --- a/src/applications/paste/editor/PhabricatorPasteEditor.php +++ b/src/applications/paste/editor/PhabricatorPasteEditor.php @@ -184,8 +184,4 @@ final class PhabricatorPasteEditor return false; } - protected function supportsWorkers() { - return true; - } - } diff --git a/src/applications/phortune/editor/PhortuneCartEditor.php b/src/applications/phortune/editor/PhortuneCartEditor.php index 0597c82d87..02f9b729de 100644 --- a/src/applications/phortune/editor/PhortuneCartEditor.php +++ b/src/applications/phortune/editor/PhortuneCartEditor.php @@ -189,7 +189,7 @@ final class PhortuneCartEditor protected function getMailTo(PhabricatorLiskDAO $object) { $phids = array(); - // Relaod the cart to pull merchant and account information, in case we + // Reload the cart to pull merchant and account information, in case we // just created the object. $cart = id(new PhortuneCartQuery()) ->setViewer($this->requireActor()) @@ -220,4 +220,24 @@ final class PhortuneCartEditor ->setMailReceiver($object); } + protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { + // We need the purchases in order to build mail. + return id(new PhortuneCartQuery()) + ->setViewer($this->getActor()) + ->withIDs(array($object->getID())) + ->needPurchases(true) + ->executeOne(); + } + + protected function getCustomWorkerState() { + return array( + 'invoiceIssues' => $this->invoiceIssues, + ); + } + + protected function loadCustomWorkerState(array $state) { + $this->invoiceIssues = idx($state, 'invoiceIssues'); + return $this; + } + } diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index 9d8aedcc3a..bc5b24e8e6 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -13,6 +13,7 @@ final class PhrictionTransactionEditor private $skipAncestorCheck; private $contentVersion; private $processContentVersionError = true; + private $contentDiffURI; public function setDescription($description) { $this->description = $description; @@ -348,6 +349,20 @@ final class PhrictionTransactionEditor ->applyTransactions($this->moveAwayDocument, $move_away_xactions); } + // Compute the content diff URI for the publishing phase. + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case PhrictionTransaction::TYPE_CONTENT: + $uri = id(new PhutilURI('/phriction/diff/'.$object->getID().'/')) + ->alter('l', $this->getOldContent()->getVersion()) + ->alter('r', $this->getNewContent()->getVersion()); + $this->contentDiffURI = (string)$uri; + break 2; + default: + break; + } + } + return $xactions; } @@ -403,24 +418,10 @@ final class PhrictionTransactionEditor $body->addTextSection( pht('DOCUMENT CONTENT'), $object->getContent()->getContent()); - } else { - - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PhrictionTransaction::TYPE_CONTENT: - $diff_uri = id(new PhutilURI( - '/phriction/diff/'.$object->getID().'/')) - ->alter('l', $this->getOldContent()->getVersion()) - ->alter('r', $this->getNewContent()->getVersion()); - $body->addLinkSection( - pht('DOCUMENT DIFF'), - PhabricatorEnv::getProductionURI($diff_uri)); - break 2; - default: - break; - } - } - + } else if ($this->contentDiffURI) { + $body->addLinkSection( + pht('DOCUMENT DIFF'), + PhabricatorEnv::getProductionURI($this->contentDiffURI)); } $body->addLinkSection( @@ -810,4 +811,15 @@ final class PhrictionTransactionEditor return $new_content; } + protected function getCustomWorkerState() { + return array( + 'contentDiffURI' => $this->contentDiffURI, + ); + } + + protected function loadCustomWorkerState(array $state) { + $this->contentDiffURI = idx($state, 'contentDiffURI'); + return $this; + } + } diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index d449b62801..4193c2e4c4 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -397,14 +397,13 @@ final class PhabricatorProjectTransactionEditor return parent::requireCapabilities($object, $xaction); } - protected function loadEdges( - PhabricatorLiskDAO $object, - array $xactions) { - + protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { $member_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( $object->getPHID(), PhabricatorProjectProjectHasMemberEdgeType::EDGECONST); $object->attachMemberPHIDs($member_phids); + + return $object; } protected function shouldSendMail( diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 127c0de8eb..4a8f485733 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1,6 +1,36 @@ heraldHeader = $herald_header; - if ($this->supportsWorkers()) { - PhabricatorWorker::scheduleTask( - 'PhabricatorApplicationTransactionPublishWorker', - array( - 'objectPHID' => $object->getPHID(), - 'actorPHID' => $this->getActingAsPHID(), - 'xactionPHIDs' => mpull($xactions, 'getPHID'), - 'state' => $this->getWorkerState(), - ), - array( - 'objectPHID' => $object->getPHID(), - 'priority' => PhabricatorWorker::PRIORITY_ALERTS, - )); - } else { - $this->publishTransactions($object, $xactions); + // We're going to compute some of the data we'll use to publish these + // transactions here, before queueing a worker. + // + // Primarily, this is more correct: we want to publish the object as it + // exists right now. The worker may not execute for some time, and we want + // to use the current To/CC list, not respect any changes which may occur + // between now and when the worker executes. + // + // As a secondary benefit, this tends to reduce the amount of state that + // Editors need to pass into workers. + $object = $this->willPublish($object, $xactions); + + if (!$this->getDisableEmail()) { + if ($this->shouldSendMail($object, $xactions)) { + $this->mailToPHIDs = $this->getMailTo($object); + $this->mailCCPHIDs = $this->getMailCC($object); + } } + if ($this->shouldPublishFeedStory($object, $xactions)) { + $this->feedRelatedPHIDs = $this->getFeedRelatedPHIDs($object, $xactions); + $this->feedNotifyPHIDs = $this->getFeedNotifyPHIDs($object, $xactions); + } + + PhabricatorWorker::scheduleTask( + 'PhabricatorApplicationTransactionPublishWorker', + array( + 'objectPHID' => $object->getPHID(), + 'actorPHID' => $this->getActingAsPHID(), + 'xactionPHIDs' => mpull($xactions, 'getPHID'), + 'state' => $this->getWorkerState(), + ), + array( + 'objectPHID' => $object->getPHID(), + 'priority' => PhabricatorWorker::PRIORITY_ALERTS, + )); + return $xactions; } @@ -936,7 +990,7 @@ abstract class PhabricatorApplicationTransactionEditor // in various transaction phases). $this->loadSubscribers($object); // Hook for other edges that may need (re-)loading - $this->loadEdges($object, $xactions); + $object = $this->willPublish($object, $xactions); $this->loadHandles($xactions); @@ -1042,12 +1096,6 @@ abstract class PhabricatorApplicationTransactionEditor } } - protected function loadEdges( - PhabricatorLiskDAO $object, - array $xactions) { - return; - } - private function validateEditParameters( PhabricatorLiskDAO $object, array $xactions) { @@ -2084,8 +2132,8 @@ abstract class PhabricatorApplicationTransactionEditor } $email_force = array(); - $email_to = $this->getMailTo($object); - $email_cc = $this->getMailCC($object); + $email_to = $this->mailToPHIDs; + $email_cc = $this->mailCCPHIDs; $email_cc = array_merge($email_cc, $this->heraldEmailPHIDs); $email_force = $this->heraldForcedEmailPHIDs; @@ -2511,8 +2559,8 @@ abstract class PhabricatorApplicationTransactionEditor return; } - $related_phids = $this->getFeedRelatedPHIDs($object, $xactions); - $subscribed_phids = $this->getFeedNotifyPHIDs($object, $xactions); + $related_phids = $this->feedRelatedPHIDs; + $subscribed_phids = $this->feedNotifyPHIDs; $story_type = $this->getFeedStoryType(); $story_data = $this->getFeedStoryData($object, $xactions); @@ -2800,11 +2848,17 @@ abstract class PhabricatorApplicationTransactionEditor /** + * Load any object state which is required to publish transactions. + * + * This hook is invoked in the main process before we compute data related + * to publishing transactions (like email "To" and "CC" lists), and again in + * the worker before publishing occurs. + * + * @return object Publishable object. * @task workers */ - protected function supportsWorkers() { - // TODO: Remove this method once everything supports workers. - return false; + protected function willPublish(PhabricatorLiskDAO $object, array $xactions) { + return $object; } @@ -2825,11 +2879,10 @@ abstract class PhabricatorApplicationTransactionEditor $state += array( 'excludeMailRecipientPHIDs' => $this->getExcludeMailRecipientPHIDs(), - ); - - return $state + array( 'custom' => $this->getCustomWorkerState(), ); + + return $state; } @@ -2900,6 +2953,10 @@ abstract class PhabricatorApplicationTransactionEditor 'heraldEmailPHIDs', 'heraldForcedEmailPHIDs', 'heraldHeader', + 'mailToPHIDs', + 'mailCCPHIDs', + 'feedNotifyPHIDs', + 'feedRelatedPHIDs', ); }