diff --git a/src/applications/legalpad/controller/LegalpadDocumentEditController.php b/src/applications/legalpad/controller/LegalpadDocumentEditController.php index d8ebc2d0c2..0fe6a5e9cf 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentEditController.php +++ b/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())); diff --git a/src/applications/legalpad/query/LegalpadDocumentQuery.php b/src/applications/legalpad/query/LegalpadDocumentQuery.php index 4936433c92..2d7c4ffa09 100644 --- a/src/applications/legalpad/query/LegalpadDocumentQuery.php +++ b/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); } diff --git a/src/applications/legalpad/query/LegalpadDocumentSearchEngine.php b/src/applications/legalpad/query/LegalpadDocumentSearchEngine.php index 951838d7f2..7d2fe8d96d 100644 --- a/src/applications/legalpad/query/LegalpadDocumentSearchEngine.php +++ b/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; } diff --git a/src/applications/policy/__tests__/PhabricatorPolicyDataTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyDataTestCase.php index e0f66c1569..1e74f8bc9e 100644 --- a/src/applications/policy/__tests__/PhabricatorPolicyDataTestCase.php +++ b/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(); diff --git a/src/applications/policy/rule/PhabricatorPolicyRuleLegalpadSignature.php b/src/applications/policy/rule/PhabricatorPolicyRuleLegalpadSignature.php index 8941d53b28..4c41b94ac9 100644 --- a/src/applications/policy/rule/PhabricatorPolicyRuleLegalpadSignature.php +++ b/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) diff --git a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php index c633b16241..3d6e688e3d 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php +++ b/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.',