From add7bc418dcb375b8bc5a28d0376ab694514d4a8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 29 Jun 2014 07:53:53 -0700 Subject: [PATCH] Allow Herald to "Require legal signatures" for reviews Summary: Ref T3116. Add a Herald action "Require legal signatures" which requires revision authors to accept legal agreements before their revisions can be accepted. - Herald will check which documents the author has signed, and trigger a "you have to sign X, Y, Z" for other documents. - If the author has already signed everything, we don't spam the revision -- basically, this only triggers when signatures are missing. - The UI will show which documents must be signed and warn that the revision can't be accepted until they're completed. - Users aren't allowed to "Accept" the revision until documents are cleared. Fixes T1157. The original install making the request (Hive) no longer uses Phabricator, and this satisfies our requirements. Test Plan: - Added a Herald rule. - Created a revision, saw the rule trigger. - Viewed as author and non-author, saw field UI (generic for non-author, specific for author), transaction UI, and accept-warning UI. - Tried to accept revision. - Signed document, saw UI update. Note that signatures don't currently //push// an update to the revision, but could eventually (like blocking tasks work). - Accepted revision. - Created another revision, saw rules not add the document (since it's already signed, this is the "no spam" case). Reviewers: btrahan, chad Reviewed By: chad Subscribers: asherkin, epriestley Maniphest Tasks: T1157, T3116 Differential Revision: https://secure.phabricator.com/D9771 --- resources/celerity/map.php | 24 ++-- src/__phutil_library_map__.php | 2 + .../DifferentialRequiredSignaturesField.php | 134 ++++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 49 +++++++ .../parser/DifferentialChangesetParser.php | 24 ++-- .../herald/adapter/HeraldAdapter.php | 5 + .../HeraldDifferentialRevisionAdapter.php | 15 ++ .../controller/HeraldRuleController.php | 1 + .../herald/storage/HeraldRule.php | 2 +- .../LegalpadDocumentSignController.php | 1 - .../edges/constants/PhabricatorEdgeConfig.php | 12 +- .../PhabricatorBaseEnglishTranslation.php | 7 + .../js/application/herald/HeraldRuleEditor.js | 1 + 13 files changed, 254 insertions(+), 23 deletions(-) create mode 100644 src/applications/differential/customfield/DifferentialRequiredSignaturesField.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b535ea0a12..3645ce0536 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -386,7 +386,7 @@ return array( 'rsrc/js/application/files/behavior-icon-composer.js' => '8ef9ab58', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-reorder-steps.js' => 'b716477f', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => '6c9e6fb8', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => '58e048fc', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-editor.js' => 'f588412e', @@ -537,7 +537,7 @@ return array( 'global-drag-and-drop-css' => '697324ad', 'harbormaster-css' => 'cec833b7', 'herald-css' => 'c544dd1c', - 'herald-rule-editor' => '6c9e6fb8', + 'herald-rule-editor' => '58e048fc', 'herald-test-css' => '778b008e', 'inline-comment-summary-css' => '8cfd34e8', 'javelin-aphlict' => '4a07e8e3', @@ -1249,6 +1249,16 @@ return array( 2 => 'javelin-vector', 3 => 'javelin-dom', ), + '58e048fc' => + array( + 0 => 'multirow-row-manager', + 1 => 'javelin-install', + 2 => 'javelin-util', + 3 => 'javelin-dom', + 4 => 'javelin-stratcom', + 5 => 'javelin-json', + 6 => 'phabricator-prefab', + ), '58f7803f' => array( 0 => 'javelin-behavior', @@ -1336,16 +1346,6 @@ return array( 0 => 'javelin-install', 1 => 'javelin-util', ), - '6c9e6fb8' => - array( - 0 => 'multirow-row-manager', - 1 => 'javelin-install', - 2 => 'javelin-util', - 3 => 'javelin-dom', - 4 => 'javelin-stratcom', - 5 => 'javelin-json', - 6 => 'phabricator-prefab', - ), '6d3e1947' => array( 0 => 'javelin-behavior', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3fd46e6365..7afed07307 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -423,6 +423,7 @@ phutil_register_library_map(array( 'DifferentialReplyHandler' => 'applications/differential/mail/DifferentialReplyHandler.php', 'DifferentialRepositoryField' => 'applications/differential/customfield/DifferentialRepositoryField.php', 'DifferentialRepositoryLookup' => 'applications/differential/query/DifferentialRepositoryLookup.php', + 'DifferentialRequiredSignaturesField' => 'applications/differential/customfield/DifferentialRequiredSignaturesField.php', 'DifferentialResultsTableView' => 'applications/differential/view/DifferentialResultsTableView.php', 'DifferentialRevertPlanField' => 'applications/differential/customfield/DifferentialRevertPlanField.php', 'DifferentialReviewedByField' => 'applications/differential/customfield/DifferentialReviewedByField.php', @@ -3121,6 +3122,7 @@ phutil_register_library_map(array( 'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler', 'DifferentialRepositoryField' => 'DifferentialCoreCustomField', 'DifferentialRepositoryLookup' => 'Phobject', + 'DifferentialRequiredSignaturesField' => 'DifferentialCoreCustomField', 'DifferentialResultsTableView' => 'AphrontView', 'DifferentialRevertPlanField' => 'DifferentialStoredCustomField', 'DifferentialReviewedByField' => 'DifferentialCoreCustomField', diff --git a/src/applications/differential/customfield/DifferentialRequiredSignaturesField.php b/src/applications/differential/customfield/DifferentialRequiredSignaturesField.php new file mode 100644 index 0000000000..809d60573c --- /dev/null +++ b/src/applications/differential/customfield/DifferentialRequiredSignaturesField.php @@ -0,0 +1,134 @@ +getPHID()) { + return array(); + } + + $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $revision->getPHID(), + PhabricatorEdgeConfig::TYPE_OBJECT_NEEDS_SIGNATURE); + + if ($phids) { + + // NOTE: We're bypassing permissions to pull these. We have to expose + // some information about signature status in order to implement this + // field meaningfully (otherwise, we could not tell reviewers that they + // can't accept the revision yet), but that's OK because the only way to + // require signatures is with a "Global" Herald rule, which requires a + // high level of access. + + $signatures = id(new LegalpadDocumentSignatureQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withDocumentPHIDs($phids) + ->withSignerPHIDs(array($revision->getAuthorPHID())) + ->execute(); + $signatures = mpull($signatures, null, 'getDocumentPHID'); + + $phids = array_fuse($phids); + foreach ($phids as $phid) { + $phids[$phid] = isset($signatures[$phid]); + } + } + + return $phids; + } + + public function getRequiredHandlePHIDsForPropertyView() { + return array_keys($this->getValue()); + } + + public function renderPropertyViewValue(array $handles) { + if (!$handles) { + return null; + } + + $author_phid = $this->getObject()->getAuthorPHID(); + $viewer_phid = $this->getViewer()->getPHID(); + + $viewer_is_author = ($author_phid == $viewer_phid); + + $view = new PHUIStatusListView(); + foreach ($handles as $handle) { + $item = id(new PHUIStatusItemView()) + ->setTarget($handle->renderLink()); + + // NOTE: If the viewer isn't the author, we just show generic document + // icons, because the granular information isn't very useful and there + // is no need to disclose it. + + // If the viewer is the author, we show exactly what they need to sign. + + if (!$viewer_is_author) { + $item->setIcon('fa-file-text-o bluegrey'); + } else { + if (idx($this->getValue(), $handle->getPHID())) { + $item->setIcon('fa-check-square-o green'); + } else { + $item->setIcon('fa-times red'); + } + } + + $view->addItem($item); + } + + return $view; + } + + public function getWarningsForDetailView() { + if (!$this->haveAnyUnsignedDocuments()) { + return array(); + } + + return array( + pht( + 'The author of this revision has not signed all the required '. + 'legal documents. The revision can not be accepted until the '. + 'documents are signed.'), + ); + } + + private function haveAnyUnsignedDocuments() { + foreach ($this->getValue() as $phid => $signed) { + if (!$signed) { + return true; + } + } + + return false; + } + + +} diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index eb61f82276..8b221ff9e0 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -825,6 +825,18 @@ final class DifferentialTransactionEditor 'You can not accept this revision because it has already been '. 'closed.'); } + + // TODO: It would be nice to make this generic at some point. + $signatures = DifferentialRequiredSignaturesField::loadForRevision( + $revision); + foreach ($signatures as $phid => $signed) { + if (!$signed) { + return pht( + 'You can not accept this revision because the author has '. + 'not signed all of the required legal documents.'); + } + } + break; case DifferentialAction::ACTION_REJECT: @@ -1377,6 +1389,15 @@ final class DifferentialTransactionEditor if (!$this->getIsCloseByCommit()) { return true; } + break; + case DifferentialTransaction::TYPE_ACTION: + switch ($xaction->getNewValue()) { + case DifferentialAction::ACTION_CLAIM: + // When users commandeer revisions, we may need to trigger + // signatures or author-based rules. + return true; + } + break; } } @@ -1501,6 +1522,34 @@ final class DifferentialTransactionEditor ->setNewValue($value); } + // Require legalpad document signatures. + $legal_phids = $adapter->getRequiredSignatureDocumentPHIDs(); + if ($legal_phids) { + // We only require signatures of documents which have not already + // been signed. In general, this reduces the amount of churn that + // signature rules cause. + + $signatures = id(new LegalpadDocumentSignatureQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withDocumentPHIDs($legal_phids) + ->withSignerPHIDs(array($object->getAuthorPHID())) + ->execute(); + $signed_phids = mpull($signatures, 'getDocumentPHID'); + $legal_phids = array_diff($legal_phids, $signed_phids); + + // If we still have something to trigger, add the edges. + if ($legal_phids) { + $edge_legal = PhabricatorEdgeConfig::TYPE_OBJECT_NEEDS_SIGNATURE; + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $edge_legal) + ->setNewValue( + array( + '+' => array_fuse($legal_phids), + )); + } + } + // Save extra email PHIDs for later. $email_phids = $adapter->getEmailPHIDsAddedByHerald(); $this->heraldEmailPHIDs = array_keys($email_phids); diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index a2df4ba918..7b9bc23397 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -376,14 +376,22 @@ final class DifferentialChangesetParser { $conn_w = $changeset->establishConnection('w'); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - queryfx( - $conn_w, - 'INSERT INTO %T (id, cache, dateCreated) VALUES (%d, %B, %d) - ON DUPLICATE KEY UPDATE cache = VALUES(cache)', - DifferentialChangeset::TABLE_CACHE, - $render_cache_key, - $cache, - time()); + try { + queryfx( + $conn_w, + 'INSERT INTO %T (id, cache, dateCreated) VALUES (%d, %B, %d) + ON DUPLICATE KEY UPDATE cache = VALUES(cache)', + DifferentialChangeset::TABLE_CACHE, + $render_cache_key, + $cache, + time()); + } catch (AphrontQueryException $ex) { + // Ignore these exceptions. A common cause is that the cache is + // larger than 'max_allowed_packet', in which case we're better off + // not writing it. + + // TODO: It would be nice to tailor this more narrowly. + } unset($unguarded); } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index dbfde93337..16b31ad86b 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -79,6 +79,7 @@ abstract class HeraldAdapter { const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers'; const ACTION_APPLY_BUILD_PLANS = 'applybuildplans'; const ACTION_BLOCK = 'block'; + const ACTION_REQUIRE_SIGNATURE = 'signature'; const VALUE_TEXT = 'text'; const VALUE_NONE = 'none'; @@ -95,6 +96,7 @@ abstract class HeraldAdapter { const VALUE_BUILD_PLAN = 'buildplan'; const VALUE_TASK_PRIORITY = 'taskpriority'; const VALUE_ARCANIST_PROJECT = 'arcanistprojects'; + const VALUE_LEGAL_DOCUMENTS = 'legaldocuments'; private $contentSource; private $isNewObject; @@ -661,6 +663,7 @@ abstract class HeraldAdapter { self::ACTION_ADD_REVIEWERS => pht('Add reviewers'), self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'), self::ACTION_APPLY_BUILD_PLANS => pht('Run build plans'), + self::ACTION_REQUIRE_SIGNATURE => pht('Require legal signatures'), self::ACTION_BLOCK => pht('Block change with message'), ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: @@ -852,6 +855,8 @@ abstract class HeraldAdapter { return self::VALUE_USER_OR_PROJECT; case self::ACTION_APPLY_BUILD_PLANS: return self::VALUE_BUILD_PLAN; + case self::ACTION_REQUIRE_SIGNATURE: + return self::VALUE_LEGAL_DOCUMENTS; case self::ACTION_BLOCK: return self::VALUE_TEXT; default: diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 30741ee2c0..93f452214c 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -15,6 +15,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { protected $addReviewerPHIDs = array(); protected $blockingReviewerPHIDs = array(); protected $buildPlans = array(); + protected $requiredSignatureDocumentPHIDs = array(); protected $repository; protected $affectedPackages; @@ -143,6 +144,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return $this->blockingReviewerPHIDs; } + public function getRequiredSignatureDocumentPHIDs() { + return $this->requiredSignatureDocumentPHIDs; + } + public function getBuildPlans() { return $this->buildPlans; } @@ -347,6 +352,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::ACTION_ADD_REVIEWERS, self::ACTION_ADD_BLOCKING_REVIEWERS, self::ACTION_APPLY_BUILD_PLANS, + self::ACTION_REQUIRE_SIGNATURE, self::ACTION_NOTHING, ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: @@ -475,6 +481,15 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { true, pht('Applied build plans.')); break; + case self::ACTION_REQUIRE_SIGNATURE: + foreach ($effect->getTarget() as $phid) { + $this->requiredSignatureDocumentPHIDs[] = $phid; + } + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Required signatures.')); + break; default: throw new Exception("No rules to handle action '{$action}'."); } diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index c720e0a5a9..771438bfb3 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -595,6 +595,7 @@ final class HeraldRuleController extends HeraldController { 'buildplan' => '/typeahead/common/buildplans/', 'taskpriority' => '/typeahead/common/taskpriority/', 'arcanistprojects' => '/typeahead/common/arcanistprojects/', + 'legaldocuments' => '/typeahead/common/legalpaddocuments/', ), 'username' => $this->getRequest()->getUser()->getUserName(), 'icons' => mpull($handles, 'getTypeIcon', 'getPHID'), diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index cdd4c5be7c..15e85646ed 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -18,7 +18,7 @@ final class HeraldRule extends HeraldDAO protected $isDisabled = 0; protected $triggerObjectPHID; - protected $configVersion = 36; + protected $configVersion = 37; // phids for which this rule has been applied private $ruleApplied = self::ATTACHABLE; diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php index 35ddd7d918..89581426cc 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -64,7 +64,6 @@ final class LegalpadDocumentSignController extends LegalpadController { ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withDocumentPHIDs(array($document->getPHID())) ->withSignerPHIDs(array($signer_phid)) - ->withDocumentVersions(array($document->getVersions())) ->executeOne(); if ($signature && !$viewer->isLoggedIn()) { diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 378dfe9cc0..d670107ccc 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -75,6 +75,9 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_OBJECT_HAS_WATCHER = 47; const TYPE_WATCHER_HAS_OBJECT = 48; + const TYPE_OBJECT_NEEDS_SIGNATURE = 49; + const TYPE_SIGNATURE_NEEDED_BY_OBJECT = 50; + const TYPE_TEST_NO_CYCLE = 9000; const TYPE_PHOB_HAS_ASANATASK = 80001; @@ -164,7 +167,12 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { self::TYPE_DASHBOARD_HAS_PANEL => self::TYPE_PANEL_HAS_DASHBOARD, self::TYPE_OBJECT_HAS_WATCHER => self::TYPE_WATCHER_HAS_OBJECT, - self::TYPE_WATCHER_HAS_OBJECT => self::TYPE_OBJECT_HAS_WATCHER + self::TYPE_WATCHER_HAS_OBJECT => self::TYPE_OBJECT_HAS_WATCHER, + + self::TYPE_OBJECT_NEEDS_SIGNATURE => + self::TYPE_SIGNATURE_NEEDED_BY_OBJECT, + self::TYPE_SIGNATURE_NEEDED_BY_OBJECT => + self::TYPE_OBJECT_NEEDS_SIGNATURE, ); return idx($map, $edge_type); @@ -352,6 +360,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return '%s added %d dashboard(s): %s.'; case self::TYPE_OBJECT_HAS_WATCHER: return '%s added %d watcher(s): %s.'; + case self::TYPE_OBJECT_NEEDS_SIGNATURE: + return '%s added %d required legal document(s): %s.'; case self::TYPE_SUBSCRIBED_TO_OBJECT: case self::TYPE_UNSUBSCRIBED_FROM_OBJECT: case self::TYPE_FILE_HAS_OBJECT: diff --git a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php index 3d6e688e3d..338eadc068 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php @@ -926,6 +926,13 @@ abstract class PhabricatorBaseEnglishTranslation ), ), + '%s added %d required legal document(s): %s.' => array( + array( + '%s added a required legal document: %3$s.', + '%s added required legal documents: %3$s.', + ), + ), + '%s updated JIRA issue(s): added %d %s; removed %d %s.' => '%s updated JIRA issues: added %3$s; removed %5$s.', diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index 566659f54b..4bfe502af6 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -220,6 +220,7 @@ JX.install('HeraldRuleEditor', { case 'buildplan': case 'taskpriority': case 'arcanistprojects': + case 'legaldocuments': var tokenizer = this._newTokenizer(type); input = tokenizer[0]; get_fn = tokenizer[1];