revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-29 16:08:22 +01:00
Code Issues Releases Wiki Activity

Allow Herald to "Require legal signatures" for reviews

Browse source 
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
This commit is contained in:
epriestley 2014-06-29 07:53:53 -07:00
parent ffc1b5c26a
commit add7bc418d
13 changed files with 254 additions and 23 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
resources/celerity/map.php
View file

@ -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',

2
src/__phutil_library_map__.php
View file

@ -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',

134
src/applications/differential/customfield/DifferentialRequiredSignaturesField.php Normal file
View file

@ -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;
}
}

49
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -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);

24
src/applications/differential/parser/DifferentialChangesetParser.php
View file

@ -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);
}

5
src/applications/herald/adapter/HeraldAdapter.php
View file

@ -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:

15
src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
View file

@ -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}'.");
}

1
src/applications/herald/controller/HeraldRuleController.php
View file

@ -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'),

2
src/applications/herald/storage/HeraldRule.php
View file

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

1
src/applications/legalpad/controller/LegalpadDocumentSignController.php
View file

@ -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()) {

12
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
View file

@ -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:

7
src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php
View file

@ -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.',

1
webroot/rsrc/js/application/herald/HeraldRuleEditor.js
View file

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