From 489f9e7dfee82c57d88c8d49eb5a06eca0ca3886 Mon Sep 17 00:00:00 2001 From: Anh Nhan Nguyen Date: Thu, 21 Mar 2013 10:26:32 -0700 Subject: [PATCH] Added subscriptions to Phriction documents Summary: Fixes T2694 added edge infrastructure for Phriction added mail subject prefix option for Phriction added messy mail support for subscribers adds edges to the phriction db, along with the subscriber interface which gives us subscriptions for free. simple display of subscribers, adequate to the current design and sufficient fallbacks for exceptional cases. @chad may be mailed about that one more UI element may be added to his redesign mail support is messy. not generic at all. only sends to subscribed non-authors. Test Plan: tried out all kinds of stuff. applied patch, subscribed, unsubscribed with multiple accs. verified proper edited documents, verified that mail was sent in MetaMTA. Verified contents, tos and stuff by looking into the db, comparing PHIDs etc. functional testing per serious MTA (that is, AWS SES) worked wonderfully. Here's how the subscription list looks like: {F36320, layout=link} Reviewers: epriestley, chad, btrahan Reviewed By: epriestley CC: hfcorriez, aran, Korvin Maniphest Tasks: T2686, T2694 Differential Revision: https://secure.phabricator.com/D5372 Conflicts: src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php --- .../sql/patches/20130317.phrictionedge.sql | 16 +++++ src/__phutil_library_map__.php | 1 + .../PhabricatorPhrictionConfigOptions.php | 5 +- .../PhrictionDocumentController.php | 29 ++++++++- .../editor/PhrictionDocumentEditor.php | 63 +++++++++++++++++++ .../phriction/storage/PhrictionDocument.php | 6 +- .../edges/constants/PhabricatorEdgeConfig.php | 1 + .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 8 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 resources/sql/patches/20130317.phrictionedge.sql diff --git a/resources/sql/patches/20130317.phrictionedge.sql b/resources/sql/patches/20130317.phrictionedge.sql new file mode 100644 index 0000000000..fdeed8f178 --- /dev/null +++ b/resources/sql/patches/20130317.phrictionedge.sql @@ -0,0 +1,16 @@ +CREATE TABLE {$NAMESPACE}_phriction.edge ( + src VARCHAR(64) NOT NULL COLLATE utf8_bin, + type INT UNSIGNED NOT NULL COLLATE utf8_bin, + dst VARCHAR(64) NOT NULL COLLATE utf8_bin, + dateCreated INT UNSIGNED NOT NULL, + seq INT UNSIGNED NOT NULL, + dataID INT UNSIGNED, + PRIMARY KEY (src, type, dst), + KEY (src, type, dateCreated, seq) +) ENGINE=InnoDB, COLLATE utf8_general_ci; + +CREATE TABLE {$NAMESPACE}_phriction.edgedata ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + data LONGTEXT NOT NULL COLLATE utf8_bin +) ENGINE=InnoDB, COLLATE utf8_general_ci; + diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8d52d7d482..d0535ba46a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3166,6 +3166,7 @@ phutil_register_library_map(array( array( 0 => 'PhrictionDAO', 1 => 'PhabricatorPolicyInterface', + 2 => 'PhabricatorSubscribableInterface', ), 'PhrictionDocumentController' => 'PhrictionController', 'PhrictionDocumentEditor' => 'PhabricatorEditor', diff --git a/src/applications/phriction/config/PhabricatorPhrictionConfigOptions.php b/src/applications/phriction/config/PhabricatorPhrictionConfigOptions.php index 0b49152ff8..d9a2da237a 100644 --- a/src/applications/phriction/config/PhabricatorPhrictionConfigOptions.php +++ b/src/applications/phriction/config/PhabricatorPhrictionConfigOptions.php @@ -19,7 +19,10 @@ final class PhabricatorPhrictionConfigOptions pht("Enable Phriction"), pht("Disable Phriction"), )) - ->setDescription(pht("Enable or disable Phriction.")) + ->setDescription(pht("Enable or disable Phriction.")), + $this->newOption( + 'metamta.phriction.subject-prefix', 'string', '[Phriction]') + ->setDescription(pht("Subject prefix for Phriction email.")), ); } diff --git a/src/applications/phriction/controller/PhrictionDocumentController.php b/src/applications/phriction/controller/PhrictionDocumentController.php index 3133ca747e..f6a6ca2d0a 100644 --- a/src/applications/phriction/controller/PhrictionDocumentController.php +++ b/src/applications/phriction/controller/PhrictionDocumentController.php @@ -97,11 +97,19 @@ final class PhrictionDocumentController } } + $subscribers = PhabricatorSubscribersQuery::loadSubscribersForPHID( + $document->getPHID()); + $phids = array_filter( array( $content->getAuthorPHID(), $project_phid, )); + + if ($subscribers) { + $phids = array_merge($phids, $subscribers); + } + $handles = $this->loadViewerHandles($phids); $age = time() - $content->getDateCreated(); @@ -131,12 +139,29 @@ final class PhrictionDocumentController ), pht('Document Index')); + $subscriber_view = null; + if ($subscribers) { + $subcriber_list = array(); + foreach ($subscribers as $subscriber) { + $subcriber_list[] = $handles[$subscriber]; + } + + $subcriber_list = phutil_implode_html(', ', + mpull($subcriber_list, 'renderLink')); + + $subscriber_view = array( + hsprintf('
Subscribers: '), + $subcriber_list, + ); + } + $byline = hsprintf( - '
%s%s
', + '
%s%s%s
', pht('Last updated %s by %s.', $when, $handles[$content->getAuthorPHID()]->renderLink()), - $project_info); + $project_info, + $subscriber_view); $doc_status = $document->getStatus(); diff --git a/src/applications/phriction/editor/PhrictionDocumentEditor.php b/src/applications/phriction/editor/PhrictionDocumentEditor.php index c32166c13b..55ebdfe22d 100644 --- a/src/applications/phriction/editor/PhrictionDocumentEditor.php +++ b/src/applications/phriction/editor/PhrictionDocumentEditor.php @@ -278,7 +278,70 @@ final class PhrictionDocumentEditor extends PhabricatorEditor { ->publish(); } + // TODO: Migrate to ApplicationTransactions fast, so we get rid of this code + $subscribers = PhabricatorSubscribersQuery::loadSubscribersForPHID( + $document->getPHID()); + $this->sendMailToSubscribers($subscribers, $content); + return $this; } + private function sendMailToSubscribers(array $subscribers, $old_content) { + if (!$subscribers) { + return; + } + + $author_phid = $this->getActor()->getPHID(); + $document = $this->document; + $content = $document->getContent(); + + $old_title = $old_content->getTitle(); + $title = $content->getTitle(); + + // TODO: Currently, this produces something like + // Phrictioh Document Xyz was Edit + // I'm too lazy to build my own action string everywhere + // Plus, it does not have pht() anyway + $action = PhrictionChangeType::getChangeTypeLabel( + $content->getChangeType()); + $name = pht("Phriction Document %s was %s", $title, $action); + + $body = array($name); + // Content may have changed, you never know + if ($content->getChangeType() == PhrictionChangeType::CHANGE_EDIT) { + + if ($old_title != $title) { + $body[] = pht('Title was changed from "%s" to "%s"', + $old_title, $title); + } + + // The Remarkup text renderer comes in handy + // TODO: Consider sending a diff instead? + $body[] = pht("Content was changed to \n%s", $content->getContent()); + } + + $body = implode("\n\n", $body); + + $subject_prefix = $this->getMailSubjectPrefix(); + + $mail = new PhabricatorMetaMTAMail(); + $mail->setSubject($name) + ->setSubjectPrefix($subject_prefix) + ->setVarySubjectPrefix('['.$action.']') + ->addHeader('Thread-Topic', $name) + ->setFrom($author_phid) + ->addTos($subscribers) + ->setBody($body) + ->setRelatedPHID($document->getPHID()) + ->setIsBulk(true); + + $mail->saveAndSend(); + } + + /* --( For less copy-pasting when switching to ApplicationTransactions )--- */ + + protected function getMailSubjectPrefix() { + return PhabricatorEnv::getEnvConfig('metamta.phriction.subject-prefix'); + } + } diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 0deb4cdb09..f3bc38a705 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -4,7 +4,7 @@ * @group phriction */ final class PhrictionDocument extends PhrictionDAO - implements PhabricatorPolicyInterface { + implements PhabricatorPolicyInterface, PhabricatorSubscribableInterface { protected $id; protected $phid; @@ -125,4 +125,8 @@ final class PhrictionDocument extends PhrictionDAO } return false; } + + public function isAutomaticallySubscribed($phid) { + return false; + } } diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 10c39c0726..162341368f 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -116,6 +116,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { PhabricatorPHIDConstants::PHID_TYPE_MOCK => 'PholioMock', PhabricatorPHIDConstants::PHID_TYPE_MCRO => 'PhabricatorFileImageMacro', PhabricatorPHIDConstants::PHID_TYPE_CONP => 'ConpherenceThread', + PhabricatorPHIDConstants::PHID_TYPE_WIKI => 'PhrictionDocument', ); diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index fcfa0518c0..acf82627a2 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1186,6 +1186,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20130320.phlux.sql'), ), + '20130317.phrictionedge.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130317.phrictionedge.sql'), + ), ); }