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];