Page MenuHomePhabricator
Files F14002751

D9771.diff
No OneTemporary
Actions

D9771.diff
View Options

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -386,7 +386,7 @@
'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 @@
'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 @@
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 @@
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
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -423,6 +423,7 @@
'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 @@
'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
--- /dev/null
+++ b/src/applications/differential/customfield/DifferentialRequiredSignaturesField.php
@@ -0,0 +1,134 @@
+<?php
+
+final class DifferentialRequiredSignaturesField
+ extends DifferentialCoreCustomField {
+
+ public function getFieldKey() {
+ return 'differential:required-signatures';
+ }
+
+ public function getFieldName() {
+ return pht('Required Signatures');
+ }
+
+ public function getFieldDescription() {
+ return pht('Display required legal agreements.');
+ }
+
+ public function shouldAppearInPropertyView() {
+ return true;
+ }
+
+ public function shouldAppearInEditView() {
+ return false;
+ }
+
+ protected function readValueFromRevision(DifferentialRevision $revision) {
+ return self::loadForRevision($revision);
+ }
+
+ public static function loadForRevision($revision) {
+ $app_legalpad = 'PhabricatorApplicationLegalpad';
+ if (!PhabricatorApplication::isClassInstalled($app_legalpad)) {
+ return array();
+ }
+
+ if (!$revision->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
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -825,6 +825,18 @@
'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 @@
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 @@
->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
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -376,14 +376,22 @@
$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
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -79,6 +79,7 @@
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 @@
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 @@
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 @@
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
--- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
+++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
@@ -15,6 +15,7 @@
protected $addReviewerPHIDs = array();
protected $blockingReviewerPHIDs = array();
protected $buildPlans = array();
+ protected $requiredSignatureDocumentPHIDs = array();
protected $repository;
protected $affectedPackages;
@@ -143,6 +144,10 @@
return $this->blockingReviewerPHIDs;
}
+ public function getRequiredSignatureDocumentPHIDs() {
+ return $this->requiredSignatureDocumentPHIDs;
+ }
+
public function getBuildPlans() {
return $this->buildPlans;
}
@@ -347,6 +352,7 @@
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 @@
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
--- a/src/applications/herald/controller/HeraldRuleController.php
+++ b/src/applications/herald/controller/HeraldRuleController.php
@@ -595,6 +595,7 @@
'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
--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@@ -18,7 +18,7 @@
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
--- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php
+++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php
@@ -64,7 +64,6 @@
->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
--- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
+++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
@@ -75,6 +75,9 @@
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 @@
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 @@
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
--- a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php
+++ b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php
@@ -926,6 +926,13 @@
),
),
+ '%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
--- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js
+++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js
@@ -220,6 +220,7 @@
case 'buildplan':
case 'taskpriority':
case 'arcanistprojects':
+ case 'legaldocuments':
var tokenizer = this._newTokenizer(type);
input = tokenizer[0];
get_fn = tokenizer[1];

File Metadata

Mime Type
text/plain
Expires
Sat, Oct 26, 9:59 PM (2 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6751425
Default Alt Text
D9771.diff (18 KB)

Event Timeline

Phabricator · Built by Phacility