Add a "documents I've signed" view to Legalpad

Summary:
Ref T3116. Allow documents to be queried for ones the viewer has signed, and make this the default view.

This also relaxes the versioning stuff a little bit, and stops invalidating signatures on older versions of documents. While I think we should do that eventually, it should be more explicit and have better coordination in the UI. For now, we'll track and show older signatures, but not invalidate them.

I imagine eventually differentiating between "minor edits" (typo / link fixes, for example) and major edits which actually require re-signature.

Test Plan: {F171650}

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T3116

Differential Revision: https://secure.phabricator.com/D9769

src/applications/legalpad/controller/LegalpadDocumentEditController.php

@@ -31,7 +31,6 @@ final class LegalpadDocumentEditController extends LegalpadController {
       $document = id(new LegalpadDocumentQuery())
         ->setViewer($user)
         ->needDocumentBodies(true)
-        ->needSignatures(true)
         ->requireCapabilities(
           array(
             PhabricatorPolicyCapability::CAN_VIEW,
@@ -154,14 +153,7 @@ final class LegalpadDocumentEditController extends LegalpadController {
           $this->getApplicationURI('view/'.$document->getID()));
       $title = pht('Update Document');
       $short = pht('Update');
-      $signatures = $document->getSignatures();
-      if ($signatures) {
-        $form->appendInstructions(pht(
-          'Warning: there are %d signature(s) already for this document. '.
-          'Updating the title or text will invalidate these signatures and '.
-          'users will need to sign again. Proceed carefully.',
-          count($signatures)));
-      }
+
       $crumbs->addTextCrumb(
         $document->getMonogram(),
         $this->getApplicationURI('view/'.$document->getID()));

src/applications/legalpad/query/LegalpadDocumentQuery.php

@@ -77,10 +77,11 @@ final class LegalpadDocumentQuery
 
     $data = queryfx_all(
       $conn_r,
-      'SELECT d.* FROM %T d %Q %Q %Q %Q',
+      'SELECT d.* FROM %T d %Q %Q %Q %Q %Q',
       $table->getTableName(),
       $this->buildJoinClause($conn_r),
       $this->buildWhereClause($conn_r),
+      $this->buildGroupClause($conn_r),
       $this->buildOrderClause($conn_r),
       $this->buildLimitClause($conn_r));
 
@@ -90,28 +91,6 @@ final class LegalpadDocumentQuery
   }
 
   protected function willFilterPage(array $documents) {
-    if ($this->signerPHIDs) {
-      $document_map = mpull($documents, null, 'getPHID');
-      $signatures = id(new LegalpadDocumentSignatureQuery())
-        ->setViewer($this->getViewer())
-        ->withDocumentPHIDs(array_keys($document_map))
-        ->withSignerPHIDs($this->signerPHIDs)
-        ->execute();
-      $signatures = mgroup($signatures, 'getDocumentPHID');
-      foreach ($document_map as $document_phid => $document) {
-        $sigs = idx($signatures, $document_phid, array());
-        foreach ($sigs as $index => $sig) {
-          if ($sig->getDocumentVersion() != $document->getVersions()) {
-            unset($sigs[$index]);
-          }
-        }
-        $signer_phids = mpull($sigs, 'getSignerPHID');
-        if (array_diff($this->signerPHIDs, $signer_phids)) {
-          unset($documents[$document->getID()]);
-        }
-      }
-    }
-
     if ($this->needDocumentBodies) {
       $documents = $this->loadDocumentBodies($documents);
     }
@@ -126,7 +105,6 @@ final class LegalpadDocumentQuery
 
     if ($this->needViewerSignatures) {
       if ($documents) {
-
         if ($this->getViewer()->getPHID()) {
           $signatures = id(new LegalpadDocumentSignatureQuery())
             ->setViewer($this->getViewer())
@@ -153,63 +131,81 @@ final class LegalpadDocumentQuery
   private function buildJoinClause($conn_r) {
     $joins = array();
 
-    if ($this->contributorPHIDs) {
+    if ($this->contributorPHIDs !== null) {
       $joins[] = qsprintf(
         $conn_r,
-        'JOIN edge e ON e.src = d.phid');
+        'JOIN edge contributor ON contributor.src = d.phid
+          AND contributor.type = %d',
+        PhabricatorEdgeConfig::TYPE_OBJECT_HAS_CONTRIBUTOR);
+    }
+
+    if ($this->signerPHIDs !== null) {
+      $joins[] = qsprintf(
+        $conn_r,
+        'JOIN %T signer ON signer.documentPHID = d.phid
+          AND signer.signerPHID IN (%Ls)',
+        id(new LegalpadDocumentSignature())->getTableName(),
+        $this->signerPHIDs);
     }
 
     return implode(' ', $joins);
   }
 
+  private function buildGroupClause(AphrontDatabaseConnection $conn_r) {
+    if ($this->contributorPHIDs || $this->signerPHIDs) {
+      return 'GROUP BY d.id';
+    } else {
+      return '';
+    }
+  }
+
   protected function buildWhereClause($conn_r) {
     $where = array();
 
-    $where[] = $this->buildPagingClause($conn_r);
-
-    if ($this->ids) {
+    if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn_r,
         'd.id IN (%Ld)',
         $this->ids);
     }
 
-    if ($this->phids) {
+    if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn_r,
         'd.phid IN (%Ls)',
         $this->phids);
     }
 
-    if ($this->creatorPHIDs) {
+    if ($this->creatorPHIDs !== null) {
       $where[] = qsprintf(
         $conn_r,
         'd.creatorPHID IN (%Ls)',
         $this->creatorPHIDs);
     }
 
-    if ($this->dateCreatedAfter) {
+    if ($this->dateCreatedAfter !== null) {
       $where[] = qsprintf(
         $conn_r,
         'd.dateCreated >= %d',
         $this->dateCreatedAfter);
     }
 
-    if ($this->dateCreatedBefore) {
+    if ($this->dateCreatedBefore !== null) {
       $where[] = qsprintf(
         $conn_r,
         'd.dateCreated <= %d',
         $this->dateCreatedBefore);
     }
 
-    if ($this->contributorPHIDs) {
+    if ($this->contributorPHIDs !== null) {
       $where[] = qsprintf(
         $conn_r,
-        'e.type = %s AND e.dst IN (%Ls)',
-        PhabricatorEdgeConfig::TYPE_OBJECT_HAS_CONTRIBUTOR,
+        'contributor.dst IN (%Ls)',
         $this->contributorPHIDs);
     }
 
+    $where[] = $this->buildPagingClause($conn_r);
+
     return $this->formatWhereClause($where);
   }
 
@@ -256,11 +252,6 @@ final class LegalpadDocumentQuery
 
     foreach ($documents as $document) {
       $sigs = idx($signatures, $document->getPHID(), array());
-      foreach ($sigs as $index => $sig) {
-        if ($sig->getDocumentVersion() != $document->getVersions()) {
-          unset($sigs[$index]);
-        }
-      }
       $document->attachSignatures($sigs);
     }
 

src/applications/legalpad/query/LegalpadDocumentSearchEngine.php

@@ -21,6 +21,10 @@ final class LegalpadDocumentSearchEngine
       'contributorPHIDs',
       $this->readUsersFromRequest($request, 'contributors'));
 
+    $saved->setParameter(
+      'withViewerSignature',
+      $request->getBool('withViewerSignature'));
+
     $saved->setParameter('createdStart', $request->getStr('createdStart'));
     $saved->setParameter('createdEnd', $request->getStr('createdEnd'));
 
@@ -29,9 +33,24 @@ final class LegalpadDocumentSearchEngine
 
   public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) {
     $query = id(new LegalpadDocumentQuery())
-      ->needViewerSignatures(true)
-      ->withCreatorPHIDs($saved->getParameter('creatorPHIDs', array()))
-      ->withContributorPHIDs($saved->getParameter('contributorPHIDs', array()));
+      ->needViewerSignatures(true);
+
+    $creator_phids = $saved->getParameter('creatorPHIDs', array());
+    if ($creator_phids) {
+      $query->withCreatorPHIDs($creator_phids);
+    }
+
+    $contributor_phids = $saved->getParameter('contributorPHIDs', array());
+    if ($contributor_phids) {
+      $query->withContributorPHIDs($contributor_phids);
+    }
+
+    if ($saved->getParameter('withViewerSignature')) {
+      $viewer_phid = $this->requireViewer()->getPHID();
+      if ($viewer_phid) {
+        $query->withSignerPHIDs(array($viewer_phid));
+      }
+    }
 
     $start = $this->parseDateTime($saved->getParameter('createdStart'));
     $end = $this->parseDateTime($saved->getParameter('createdEnd'));
@@ -60,7 +79,20 @@ final class LegalpadDocumentSearchEngine
       ->withPHIDs($phids)
       ->execute();
 
+    $viewer_signature = $saved_query->getParameter('withViewerSignature');
+    if (!$this->requireViewer()->getPHID()) {
+      $viewer_signature = false;
+    }
+
     $form
+      ->appendChild(
+        id(new AphrontFormCheckboxControl())
+          ->addCheckbox(
+            'withViewerSignature',
+            1,
+            pht('Show only documents I have signed.'),
+            $viewer_signature)
+          ->setDisabled(!$this->requireViewer()->getPHID()))
       ->appendChild(
         id(new AphrontFormTokenizerControl())
           ->setDatasource('/typeahead/common/users/')
@@ -89,9 +121,13 @@ final class LegalpadDocumentSearchEngine
   }
 
   public function getBuiltinQueryNames() {
-    $names = array(
-      'all' => pht('All Documents'),
-    );
+    $names = array();
+
+    if ($this->requireViewer()->isLoggedIn()) {
+      $names['signed'] = pht('Signed Documents');
+    }
+
+    $names['all'] = pht('All Documents');
 
     return $names;
   }
@@ -102,6 +138,9 @@ final class LegalpadDocumentSearchEngine
     $query->setQueryKey($query_key);
 
     switch ($query_key) {
+      case 'signed':
+        return $query
+          ->setParameter('withViewerSignature', true);
       case 'all':
         return $query;
     }

src/applications/policy/__tests__/PhabricatorPolicyDataTestCase.php

@@ -14,12 +14,14 @@ final class PhabricatorPolicyDataTestCase extends PhabricatorTestCase {
     $proj_a = id(new PhabricatorProject())
       ->setName('A')
       ->setAuthorPHID($author->getPHID())
-      ->setIcon('fa-briefcase')
+      ->setIcon(PhabricatorProject::DEFAULT_ICON)
+      ->setColor(PhabricatorProject::DEFAULT_COLOR)
       ->save();
     $proj_b = id(new PhabricatorProject())
       ->setName('B')
       ->setAuthorPHID($author->getPHID())
-      ->setIcon('fa-briefcase')
+      ->setIcon(PhabricatorProject::DEFAULT_ICON)
+      ->setColor(PhabricatorProject::DEFAULT_COLOR)
       ->save();
 
     $proj_a->setViewPolicy($proj_b->getPHID())->save();

src/applications/policy/rule/PhabricatorPolicyRuleLegalpadSignature.php

@@ -15,6 +15,9 @@ final class PhabricatorPolicyRuleLegalpadSignature
       return;
     }
 
+    // TODO: This accepts signature of any version of the document, even an
+    // older version.
+
     $documents = id(new LegalpadDocumentQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withPHIDs($values)

src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php

@@ -890,17 +890,6 @@ abstract class PhabricatorBaseEnglishTranslation
         '%d Users Need Approval',
       ),
 
-      'Warning: there are %d signature(s) already for this document. '.
-      'Updating the title or text will invalidate these signatures and users '.
-      'will need to sign again. Proceed carefully.' => array(
-        'Warning: there is %d signature already for this document. '.
-        'Updating the title or text will invalidate this signature and the '.
-        'user will need to sign again. Proceed carefully.',
-        'Warning: there are %d signatures already for this document. '.
-        'Updating the title or text will invalidate these signatures and '.
-        'users will need to sign again. Proceed carefully.',
-      ),
-
       '%s older changes(s) are hidden.' => array(
         '%d older change is hidden.',
         '%d older changes are hidden.',