Modularize the Legalpad "Require Signature" Herald Action

Ref T8726. Modularizes "Require Signatures" for Legalpad.

resources/sql/autopatches/20150730.herald.6.sql

@@ -0,0 +1,6 @@
+UPDATE {$NAMESPACE}_herald.herald_action a
+  JOIN {$NAMESPACE}_herald.herald_rule r
+  ON a.ruleID = r.id
+  SET a.action = 'legalpad.require'
+  WHERE r.ruleType != 'personal'
+  AND a.action = 'signature';

src/__phutil_library_map__.php

@@ -1124,6 +1124,7 @@ phutil_register_library_map(array(
     'LegalpadMailReceiver' => 'applications/legalpad/mail/LegalpadMailReceiver.php',
     'LegalpadObjectNeedsSignatureEdgeType' => 'applications/legalpad/edge/LegalpadObjectNeedsSignatureEdgeType.php',
     'LegalpadReplyHandler' => 'applications/legalpad/mail/LegalpadReplyHandler.php',
+    'LegalpadRequireSignatureHeraldAction' => 'applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php',
     'LegalpadSchemaSpec' => 'applications/legalpad/storage/LegalpadSchemaSpec.php',
     'LegalpadSignatureNeededByObjectEdgeType' => 'applications/legalpad/edge/LegalpadSignatureNeededByObjectEdgeType.php',
     'LegalpadTransaction' => 'applications/legalpad/storage/LegalpadTransaction.php',
@@ -4852,6 +4853,7 @@ phutil_register_library_map(array(
     'LegalpadMailReceiver' => 'PhabricatorObjectMailReceiver',
     'LegalpadObjectNeedsSignatureEdgeType' => 'PhabricatorEdgeType',
     'LegalpadReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler',
+    'LegalpadRequireSignatureHeraldAction' => 'HeraldAction',
     'LegalpadSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'LegalpadSignatureNeededByObjectEdgeType' => 'PhabricatorEdgeType',
     'LegalpadTransaction' => 'PhabricatorApplicationTransaction',

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -1596,43 +1596,13 @@ final class DifferentialTransactionEditor
     HeraldAdapter $adapter,
     HeraldTranscript $transcript) {
 
-    $xactions = array();
-
-    // 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 = LegalpadObjectNeedsSignatureEdgeType::EDGECONST;
-        $xactions[] = id(new DifferentialTransaction())
-          ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-          ->setMetadataValue('edge:type', $edge_legal)
-          ->setNewValue(
-            array(
-              '+' => array_fuse($legal_phids),
-            ));
-      }
-    }
-
     // Apply build plans.
     HarbormasterBuildable::applyBuildPlans(
       $adapter->getDiff()->getPHID(),
       $adapter->getPHID(),
       $adapter->getBuildPlans());
 
-    return $xactions;
+    return array();
   }
 
   /**

src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php

@@ -6,7 +6,6 @@ final class HeraldDifferentialRevisionAdapter
   protected $revision;
 
   protected $buildPlans = array();
-  protected $requiredSignatureDocumentPHIDs = array();
 
   protected $affectedPackages;
   protected $changesets;
@@ -80,10 +79,6 @@ final class HeraldDifferentialRevisionAdapter
     return $object;
   }
 
-  public function getRequiredSignatureDocumentPHIDs() {
-    return $this->requiredSignatureDocumentPHIDs;
-  }
-
   public function getBuildPlans() {
     return $this->buildPlans;
   }
@@ -141,7 +136,6 @@ final class HeraldDifferentialRevisionAdapter
         return array_merge(
           array(
             self::ACTION_APPLY_BUILD_PLANS,
-            self::ACTION_REQUIRE_SIGNATURE,
           ),
           parent::getActions($rule_type));
       case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
@@ -168,15 +162,6 @@ final class HeraldDifferentialRevisionAdapter
             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:
           $result[] = $this->applyStandardEffect($effect);
           break;

src/applications/herald/adapter/HeraldAdapter.php

@@ -29,7 +29,6 @@ abstract class HeraldAdapter extends Phobject {
   const ACTION_AUDIT        = 'audit';
   const ACTION_APPLY_BUILD_PLANS = 'applybuildplans';
   const ACTION_BLOCK = 'block';
-  const ACTION_REQUIRE_SIGNATURE = 'signature';
 
   private $contentSource;
   private $isNewObject;
@@ -716,7 +715,6 @@ abstract class HeraldAdapter extends Phobject {
         $standard = array(
           self::ACTION_AUDIT        => pht('Trigger an Audit by'),
           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'),
         );
         break;
@@ -805,9 +803,6 @@ abstract class HeraldAdapter extends Phobject {
         case self::ACTION_APPLY_BUILD_PLANS:
           return $this->buildTokenizerFieldValue(
             new HarbormasterBuildPlanDatasource());
-        case self::ACTION_REQUIRE_SIGNATURE:
-          return $this->buildTokenizerFieldValue(
-            new LegalpadDocumentDatasource());
         case self::ACTION_BLOCK:
           return new HeraldTextFieldValue();
       }

src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php

@@ -0,0 +1,197 @@
+<?php
+
+final class LegalpadRequireSignatureHeraldAction
+  extends HeraldAction {
+
+  const DO_NO_TARGETS = 'do.no-targets';
+  const DO_ALREADY_REQUIRED = 'do.already-required';
+  const DO_INVALID = 'do.invalid';
+  const DO_SIGNED = 'do.signed';
+  const DO_REQUIRED = 'do.required';
+
+  const ACTIONCONST = 'legalpad.require';
+
+  public function getActionGroupKey() {
+    return HeraldSupportActionGroup::ACTIONGROUPKEY;
+  }
+
+  public function supportsObject($object) {
+    // TODO: This could probably be more general. Note that we call
+    // getAuthorPHID() on the object explicitly below, and this also needs to
+    // be generalized.
+    return ($object instanceof DifferentialRevision);
+  }
+
+  protected function applyRequire(array $phids) {
+    $adapter = $this->getAdapter();
+    $edgetype_legal = LegalpadObjectNeedsSignatureEdgeType::EDGECONST;
+
+    $phids = array_fuse($phids);
+
+    if (!$phids) {
+      $this->logEffect(self::DO_NO_TARGETS);
+      return;
+    }
+
+    $current = $adapter->loadEdgePHIDs($edgetype_legal);
+
+    $already = array();
+    foreach ($phids as $phid) {
+      if (isset($current[$phid])) {
+        $already[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($already) {
+      $this->logEffect(self::DO_ALREADY_REQUIRED, $phids);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $documents = id(new LegalpadDocumentQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withPHIDs($phids)
+      ->execute();
+    $documents = mpull($documents, null, 'getPHID');
+
+    $invalid = array();
+    foreach ($phids as $phid) {
+      if (empty($documents[$phid])) {
+        $invalid[] = $phid;
+        unset($documents[$phid]);
+      }
+    }
+
+    if ($invalid) {
+      $this->logEffect(self::DO_INVALID, $phids);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $object = $adapter->getObject();
+    $author_phid = $object->getAuthorPHID();
+
+    $signatures = id(new LegalpadDocumentSignatureQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withDocumentPHIDs($phids)
+      ->withSignerPHIDs(array($author_phid))
+      ->execute();
+    $signatures = mpull($signatures, null, 'getDocumentPHID');
+
+    $signed = array();
+    foreach ($phids as $phid) {
+      if (isset($signatures[$phid])) {
+        $signed[] = $phid;
+        unset($phids[$phid]);
+      }
+    }
+
+    if ($signed) {
+      $this->logEffect(self::DO_SIGNED, $phids);
+    }
+
+    if (!$phids) {
+      return;
+    }
+
+    $xaction = $adapter->newTransaction()
+      ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+      ->setMetadataValue('edge:type', $edgetype_legal)
+      ->setNewValue(
+        array(
+          '+' => $phids,
+        ));
+
+    $adapter->queueTransaction($xaction);
+
+    $this->logEffect(self::DO_REQUIRED, $phids);
+  }
+
+  protected function getActionEffectMap() {
+    return array(
+      self::DO_NO_TARGETS => array(
+        'icon' => 'fa-ban',
+        'color' => 'grey',
+        'name' => pht('No Targets'),
+      ),
+      self::DO_INVALID => array(
+        'icon' => 'fa-ban',
+        'color' => 'red',
+        'name' => pht('Invalid Targets'),
+      ),
+      self::DO_ALREADY_REQUIRED => array(
+        'icon' => 'fa-terminal',
+        'color' => 'grey',
+        'name' => pht('Already Required'),
+      ),
+      self::DO_SIGNED => array(
+        'icon' => 'fa-terminal',
+        'color' => 'green',
+        'name' => pht('Already Signed'),
+      ),
+      self::DO_REQUIRED => array(
+        'icon' => 'fa-terminal',
+        'color' => 'green',
+        'name' => pht('Required Signature'),
+      ),
+    );
+  }
+
+  public function renderActionEffectDescription($type, $data) {
+    switch ($type) {
+      case self::DO_NO_TARGETS:
+        return pht('Rule lists no targets.');
+      case self::DO_INVALID:
+        return pht(
+          '%s document(s) are not valid: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_ALREADY_REQUIRED:
+        return pht(
+          '%s document signature(s) are already required: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_SIGNED:
+        return pht(
+          '%s document(s) are already signed: %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+      case self::DO_REQUIRED:
+        return pht(
+          'Required %s signature(s): %s.',
+          new PhutilNumber(count($data)),
+          $this->renderHandleList($data));
+    }
+  }
+
+  public function getHeraldActionName() {
+    return pht('Require signatures');
+  }
+
+  public function supportsRuleType($rule_type) {
+    return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL);
+  }
+
+  public function applyEffect($object, HeraldEffect $effect) {
+    return $this->applyRequire($effect->getTarget());
+  }
+
+  public function getHeraldActionStandardType() {
+    return self::STANDARD_PHID_LIST;
+  }
+
+  protected function getDatasource() {
+    return new LegalpadDocumentDatasource();
+  }
+
+  public function renderActionDescription($value) {
+    return pht(
+      'Require document signatures: %s.',
+      $this->renderHandleList($value));
+  }
+}

src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php

@@ -1333,6 +1333,11 @@ final class PhabricatorUSEnglishTranslation
         'Added blocking reviewers: %2$s.',
       ),
 
+      'Required %s signature(s): %s.' => array(
+        'Required a signature: %2$s.',
+        'Required signatures: %2$s.',
+      ),
+
     );
   }