From de7f836f03e62d08697cb54540d190a715358604 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 27 Jan 2018 05:22:09 -0800 Subject: [PATCH 01/33] Wrap edge transaction readers in a translation layer Summary: Ref T13051. This puts a translation layer between the raw edge data in the transaction table and the UI that uses it. The intent is to start writing new, more compact data soon. This class give us a consistent API for interacting with either the new or old data format, so we don't have to migrate everything upfront. Test Plan: Browsed around, saw existing edge transactions render properly in transactions and feed. Added and removed subscribers and projects, saw good transaction rendering. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13051 Differential Revision: https://secure.phabricator.com/D18946 --- src/__phutil_library_map__.php | 2 + .../storage/DifferentialTransaction.php | 13 ---- .../PhabricatorApplicationTransaction.php | 25 ++++--- .../util/PhabricatorEdgeChangeRecord.php | 68 +++++++++++++++++++ 4 files changed, 82 insertions(+), 26 deletions(-) create mode 100644 src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 44d67c6259..c40f79b052 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2747,6 +2747,7 @@ phutil_register_library_map(array( 'PhabricatorDraftEngine' => 'applications/transactions/draft/PhabricatorDraftEngine.php', 'PhabricatorDraftInterface' => 'applications/transactions/draft/PhabricatorDraftInterface.php', 'PhabricatorDrydockApplication' => 'applications/drydock/application/PhabricatorDrydockApplication.php', + 'PhabricatorEdgeChangeRecord' => 'infrastructure/edges/util/PhabricatorEdgeChangeRecord.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', @@ -8170,6 +8171,7 @@ phutil_register_library_map(array( 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorDraftEngine' => 'Phobject', 'PhabricatorDrydockApplication' => 'PhabricatorApplication', + 'PhabricatorEdgeChangeRecord' => 'Phobject', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', 'PhabricatorEdgeConstants' => 'Phobject', 'PhabricatorEdgeCycleException' => 'Exception', diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 667341cef4..f7dcf29860 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -87,19 +87,6 @@ final class DifferentialTransaction } break; - case PhabricatorTransactions::TYPE_EDGE: - $add = array_diff_key($new, $old); - $rem = array_diff_key($old, $new); - - // Hide metadata-only edge transactions. These correspond to users - // accepting or rejecting revisions, but the change is always explicit - // because of the TYPE_ACTION transaction. Rendering these transactions - // just creates clutter. - - if (!$add && !$rem) { - return true; - } - break; case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: // Don't hide the initial "X requested review: ..." transaction from // mail or feed even when it occurs during creation. We need this diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index d49c3bf675..97b57009ec 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -302,8 +302,8 @@ abstract class PhabricatorApplicationTransaction $phids[] = $new; break; case PhabricatorTransactions::TYPE_EDGE: - $phids[] = ipull($old, 'dst'); - $phids[] = ipull($new, 'dst'); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $phids[] = $record->getChangedPHIDs(); break; case PhabricatorTransactions::TYPE_COLUMNS: foreach ($new as $move) { @@ -632,9 +632,8 @@ abstract class PhabricatorApplicationTransaction return true; break; case PhabricatorObjectMentionedByObjectEdgeType::EDGECONST: - $new = ipull($this->getNewValue(), 'dst'); - $old = ipull($this->getOldValue(), 'dst'); - $add = array_diff($new, $old); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); $add_value = reset($add); $add_handle = $this->getHandle($add_value); if ($add_handle->getPolicyFiltered()) { @@ -933,10 +932,10 @@ abstract class PhabricatorApplicationTransaction } break; case PhabricatorTransactions::TYPE_EDGE: - $new = ipull($new, 'dst'); - $old = ipull($old, 'dst'); - $add = array_diff($new, $old); - $rem = array_diff($old, $new); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); + $rem = $record->getRemovedPHIDs(); + $type = $this->getMetadata('edge:type'); $type = head($type); @@ -1172,10 +1171,10 @@ abstract class PhabricatorApplicationTransaction $this->renderHandleLink($new)); } case PhabricatorTransactions::TYPE_EDGE: - $new = ipull($new, 'dst'); - $old = ipull($old, 'dst'); - $add = array_diff($new, $old); - $rem = array_diff($old, $new); + $record = PhabricatorEdgeChangeRecord::newFromTransaction($this); + $add = $record->getAddedPHIDs(); + $rem = $record->getRemovedPHIDs(); + $type = $this->getMetadata('edge:type'); $type = head($type); diff --git a/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php new file mode 100644 index 0000000000..6e0cb1d3e0 --- /dev/null +++ b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php @@ -0,0 +1,68 @@ +xaction = $xaction; + return $record; + } + + public function getChangedPHIDs() { + $add = $this->getAddedPHIDs(); + $rem = $this->getRemovedPHIDs(); + + $add = array_fuse($add); + $rem = array_fuse($rem); + + return array_keys($add + $rem); + } + + public function getAddedPHIDs() { + $old = $this->getOldDestinationPHIDs(); + $new = $this->getNewDestinationPHIDs(); + + $old = array_fuse($old); + $new = array_fuse($new); + + $add = array_diff_key($new, $old); + return array_keys($add); + } + + public function getRemovedPHIDs() { + $old = $this->getOldDestinationPHIDs(); + $new = $this->getNewDestinationPHIDs(); + + $old = array_fuse($old); + $new = array_fuse($new); + + $rem = array_diff_key($old, $new); + return array_keys($rem); + } + + private function getOldDestinationPHIDs() { + if ($this->xaction) { + $old = $this->xaction->getOldValue(); + return ipull($old, 'dst'); + } + + throw new Exception( + pht('Edge change record is not configured with any change data.')); + } + + private function getNewDestinationPHIDs() { + if ($this->xaction) { + $new = $this->xaction->getNewValue(); + return ipull($new, 'dst'); + } + + throw new Exception( + pht('Edge change record is not configured with any change data.')); + } + + +} From e5639a8ed90fad70cc3e61cf01c2c9cfc185ac66 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 27 Jan 2018 05:54:21 -0800 Subject: [PATCH 02/33] Write edge transactions in a more compact way Summary: Depends on D18946. Ref T13051. Begins writing edge transactions as just a list of changed PHIDs. Test Plan: Added, edited, and removed projects. Reviewed transaction record and database. Saw no user-facing changes but a far more compact database representation. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13051 Differential Revision: https://secure.phabricator.com/D18947 --- ...habricatorApplicationTransactionEditor.php | 26 +++++++++++++++++- .../util/PhabricatorEdgeChangeRecord.php | 27 +++++++++++++++++-- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 7dbd41f1e4..155592fc4e 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -999,7 +999,31 @@ abstract class PhabricatorApplicationTransactionEditor $xaction->setPHID($xaction->generatePHID()); $comment_editor->applyEdit($xaction, $xaction->getComment()); } else { - $xaction->save(); + + // TODO: This is a transitional hack to let us migrate edge + // transactions to a more efficient storage format. For now, we're + // going to write a new slim format to the database but keep the old + // bulky format on the objects so we don't have to upgrade all the + // edit logic to the new format yet. See T13051. + + $edge_type = PhabricatorTransactions::TYPE_EDGE; + if ($xaction->getTransactionType() == $edge_type) { + $bulky_old = $xaction->getOldValue(); + $bulky_new = $xaction->getNewValue(); + + $record = PhabricatorEdgeChangeRecord::newFromTransaction($xaction); + $slim_old = $record->getModernOldEdgeTransactionData(); + $slim_new = $record->getModernNewEdgeTransactionData(); + + $xaction->setOldValue($slim_old); + $xaction->setNewValue($slim_new); + $xaction->save(); + + $xaction->setOldValue($bulky_old); + $xaction->setNewValue($bulky_new); + } else { + $xaction->save(); + } } } diff --git a/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php index 6e0cb1d3e0..38557dc09f 100644 --- a/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php +++ b/src/infrastructure/edges/util/PhabricatorEdgeChangeRecord.php @@ -44,10 +44,18 @@ final class PhabricatorEdgeChangeRecord return array_keys($rem); } + public function getModernOldEdgeTransactionData() { + return $this->getRemovedPHIDs(); + } + + public function getModernNewEdgeTransactionData() { + return $this->getAddedPHIDs(); + } + private function getOldDestinationPHIDs() { if ($this->xaction) { $old = $this->xaction->getOldValue(); - return ipull($old, 'dst'); + return $this->getPHIDsFromTransactionValue($old); } throw new Exception( @@ -57,12 +65,27 @@ final class PhabricatorEdgeChangeRecord private function getNewDestinationPHIDs() { if ($this->xaction) { $new = $this->xaction->getNewValue(); - return ipull($new, 'dst'); + return $this->getPHIDsFromTransactionValue($new); } throw new Exception( pht('Edge change record is not configured with any change data.')); } + private function getPHIDsFromTransactionValue($value) { + if (!$value) { + return array(); + } + + // If the list items are arrays, this is an older-style map of + // dictionaries. + $head = head($value); + if (is_array($head)) { + return ipull($value, 'dst'); + } + + // If the list items are not arrays, this is a newer-style list of PHIDs. + return $value; + } } From 6d2d1d3a9770adea6f165a82d42ab9ed613b2876 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 27 Jan 2018 06:12:03 -0800 Subject: [PATCH 03/33] Add `bin/garbage compact-edges` to compact edges into the new format Summary: Depends on D18947. Ref T13051. This goes through transaction tables and compacts the edge storage into the slim format. I put this on `bin/garbage` instead of `bin/storage` because `bin/storage` has a lot of weird stuff about how it manages databases so that it can run before configuration (e.g., all the `--user`, `--password` type flags for configuring DB connections). Test Plan: Loaded an object with a bunch of transactions. Ran migration. Spot checked table for sanity. Loaded another copy of the object in the web UI, compared the two pages, saw no user-visible changes. Here's a concrete example of the migration effect -- old row: ``` *************************** 44. row *************************** id: 757 phid: PHID-XACT-PSTE-5gnaaway2vnyen5 authorPHID: PHID-USER-cvfydnwadpdj7vdon36z objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7 viewPolicy: public editPolicy: PHID-USER-cvfydnwadpdj7vdon36z commentPHID: NULL commentVersion: 0 transactionType: core:edge oldValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]}} newValue: {"PHID-PROJ-wh32nih7q5scvc5lvipv":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-wh32nih7q5scvc5lvipv","dateCreated":"1449170691","seq":"0","dataID":null,"data":[]},"PHID-PROJ-5r2ed5v27xrgltvou5or":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-5r2ed5v27xrgltvou5or","dateCreated":"1449170683","seq":"0","dataID":null,"data":[]},"PHID-PROJ-zfp44q7loir643b5i4v4":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-zfp44q7loir643b5i4v4","dateCreated":"1449170668","seq":"0","dataID":null,"data":[]},"PHID-PROJ-okljqs7prifhajtvia3t":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-okljqs7prifhajtvia3t","dateCreated":"1448902756","seq":"0","dataID":null,"data":[]},"PHID-PROJ-3cuwfuuh4pwqyuof2hhr":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-3cuwfuuh4pwqyuof2hhr","dateCreated":"1448899367","seq":"0","dataID":null,"data":[]},"PHID-PROJ-amvkc5zw2gsy7tyvocug":{"src":"PHID-PSTE-5uj6oqv4kmhtr6ctwcq7","type":"41","dst":"PHID-PROJ-amvkc5zw2gsy7tyvocug","dateCreated":"1448833330","seq":"0","dataID":null,"data":[]},"PHID-PROJ-tbowhnwinujwhb346q36":{"dst":"PHID-PROJ-tbowhnwinujwhb346q36","type":41,"data":[]},"PHID-PROJ-izrto7uflimduo6uw2tp":{"dst":"PHID-PROJ-izrto7uflimduo6uw2tp","type":41,"data":[]}} contentSource: {"source":"web","params":[]} metadata: {"edge:type":41} dateCreated: 1450197571 dateModified: 1450197571 ``` New row: ``` *************************** 44. row *************************** id: 757 phid: PHID-XACT-PSTE-5gnaaway2vnyen5 authorPHID: PHID-USER-cvfydnwadpdj7vdon36z objectPHID: PHID-PSTE-5uj6oqv4kmhtr6ctwcq7 viewPolicy: public editPolicy: PHID-USER-cvfydnwadpdj7vdon36z commentPHID: NULL commentVersion: 0 transactionType: core:edge oldValue: [] newValue: ["PHID-PROJ-tbowhnwinujwhb346q36","PHID-PROJ-izrto7uflimduo6uw2tp"] contentSource: {"source":"web","params":[]} metadata: {"edge:type":41} dateCreated: 1450197571 dateModified: 1450197571 ``` Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13051 Differential Revision: https://secure.phabricator.com/D18948 --- src/__phutil_library_map__.php | 2 + ...ollectorManagementCompactEdgesWorkflow.php | 103 ++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 src/infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c40f79b052..eaaa4d9e66 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3017,6 +3017,7 @@ phutil_register_library_map(array( 'PhabricatorGDSetupCheck' => 'applications/config/check/PhabricatorGDSetupCheck.php', 'PhabricatorGarbageCollector' => 'infrastructure/daemon/garbagecollector/PhabricatorGarbageCollector.php', 'PhabricatorGarbageCollectorManagementCollectWorkflow' => 'infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCollectWorkflow.php', + 'PhabricatorGarbageCollectorManagementCompactEdgesWorkflow' => 'infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php', 'PhabricatorGarbageCollectorManagementSetPolicyWorkflow' => 'infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementSetPolicyWorkflow.php', 'PhabricatorGarbageCollectorManagementWorkflow' => 'infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementWorkflow.php', 'PhabricatorGeneralCachePurger' => 'applications/cache/purger/PhabricatorGeneralCachePurger.php', @@ -8484,6 +8485,7 @@ phutil_register_library_map(array( 'PhabricatorGDSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorGarbageCollector' => 'Phobject', 'PhabricatorGarbageCollectorManagementCollectWorkflow' => 'PhabricatorGarbageCollectorManagementWorkflow', + 'PhabricatorGarbageCollectorManagementCompactEdgesWorkflow' => 'PhabricatorGarbageCollectorManagementWorkflow', 'PhabricatorGarbageCollectorManagementSetPolicyWorkflow' => 'PhabricatorGarbageCollectorManagementWorkflow', 'PhabricatorGarbageCollectorManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorGeneralCachePurger' => 'PhabricatorCachePurger', diff --git a/src/infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php b/src/infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php new file mode 100644 index 0000000000..c47d159850 --- /dev/null +++ b/src/infrastructure/daemon/garbagecollector/management/PhabricatorGarbageCollectorManagementCompactEdgesWorkflow.php @@ -0,0 +1,103 @@ +setName('compact-edges') + ->setExamples('**compact-edges**') + ->setSynopsis( + pht( + 'Rebuild old edge transactions storage to use a more compact '. + 'format.')) + ->setArguments(array()); + } + + public function execute(PhutilArgumentParser $args) { + $tables = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorApplicationTransaction') + ->execute(); + + foreach ($tables as $table) { + $this->compactEdges($table); + } + + return 0; + } + + private function compactEdges(PhabricatorApplicationTransaction $table) { + $conn = $table->establishConnection('w'); + $class = get_class($table); + + echo tsprintf( + "%s\n", + pht( + 'Rebuilding transactions for "%s"...', + $class)); + + $cursor = 0; + $updated = 0; + while (true) { + $rows = $table->loadAllWhere( + 'transactionType = %s + AND id > %d + AND (oldValue LIKE %> OR newValue LIKE %>) + ORDER BY id ASC LIMIT 100', + PhabricatorTransactions::TYPE_EDGE, + $cursor, + // We're looking for transactions with JSON objects in their value + // fields: the new style transactions have JSON lists instead and + // start with "[" rather than "{". + '{', + '{'); + + if (!$rows) { + break; + } + + foreach ($rows as $row) { + $id = $row->getID(); + + $old = $row->getOldValue(); + $new = $row->getNewValue(); + + if (!is_array($old) || !is_array($new)) { + echo tsprintf( + "%s\n", + pht( + 'Transaction %s (of type %s) has unexpected data, skipping.', + $id, + $class)); + } + + $record = PhabricatorEdgeChangeRecord::newFromTransaction($row); + + $old_data = $record->getModernOldEdgeTransactionData(); + $old_json = phutil_json_encode($old_data); + + $new_data = $record->getModernNewEdgeTransactionData(); + $new_json = phutil_json_encode($new_data); + + queryfx( + $conn, + 'UPDATE %T SET oldValue = %s, newValue = %s WHERE id = %d', + $table->getTableName(), + $old_json, + $new_json, + $id); + + $updated++; + + $cursor = $row->getID(); + } + } + + echo tsprintf( + "%s\n", + pht( + 'Done, compacted %s edge transactions.', + new PhutilNumber($updated))); + } + +} From 98402b885b62b945499c12efc4e2bb6dc1e59d49 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 27 Jan 2018 07:30:26 -0800 Subject: [PATCH 04/33] Add a bit of test coverage for bulky vs compact edge data representations Summary: Depends on D18948. Ref T13051. The actual logic ended up so simple that this doesn't really feel terribly valuable, but maybe it'll catch something later on. Test Plan: Ran test. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13051 Differential Revision: https://secure.phabricator.com/D18949 --- src/__phutil_library_map__.php | 2 + .../PhabricatorEdgeChangeRecordTestCase.php | 158 ++++++++++++++++++ 2 files changed, 160 insertions(+) create mode 100644 src/infrastructure/edges/__tests__/PhabricatorEdgeChangeRecordTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index eaaa4d9e66..1ad3565809 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2748,6 +2748,7 @@ phutil_register_library_map(array( 'PhabricatorDraftInterface' => 'applications/transactions/draft/PhabricatorDraftInterface.php', 'PhabricatorDrydockApplication' => 'applications/drydock/application/PhabricatorDrydockApplication.php', 'PhabricatorEdgeChangeRecord' => 'infrastructure/edges/util/PhabricatorEdgeChangeRecord.php', + 'PhabricatorEdgeChangeRecordTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeChangeRecordTestCase.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', @@ -8173,6 +8174,7 @@ phutil_register_library_map(array( 'PhabricatorDraftEngine' => 'Phobject', 'PhabricatorDrydockApplication' => 'PhabricatorApplication', 'PhabricatorEdgeChangeRecord' => 'Phobject', + 'PhabricatorEdgeChangeRecordTestCase' => 'PhabricatorTestCase', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', 'PhabricatorEdgeConstants' => 'Phobject', 'PhabricatorEdgeCycleException' => 'Exception', diff --git a/src/infrastructure/edges/__tests__/PhabricatorEdgeChangeRecordTestCase.php b/src/infrastructure/edges/__tests__/PhabricatorEdgeChangeRecordTestCase.php new file mode 100644 index 0000000000..cf46c00d4d --- /dev/null +++ b/src/infrastructure/edges/__tests__/PhabricatorEdgeChangeRecordTestCase.php @@ -0,0 +1,158 @@ +setOldValue($old_bulky); + $bulky_xaction->setNewValue($new_bulky); + + $slim_xaction = new ManiphestTransaction(); + $slim_xaction->setOldValue($old_slim); + $slim_xaction->setNewValue($new_slim); + + $bulky_record = PhabricatorEdgeChangeRecord::newFromTransaction( + $bulky_xaction); + + $slim_record = PhabricatorEdgeChangeRecord::newFromTransaction( + $slim_xaction); + + $this->assertEqual( + $bulky_record->getAddedPHIDs(), + $slim_record->getAddedPHIDs()); + + $this->assertEqual( + $bulky_record->getRemovedPHIDs(), + $slim_record->getRemovedPHIDs()); + } + +} From d8f51dff6ec3d374e13a462e59c63b194570ded7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 28 Jan 2018 17:49:30 -0800 Subject: [PATCH 05/33] Use the configured viewer more consistently in the Herald commit adapter Summary: See PHI276. Ref T13048. The fix in D18933 got one callsite, but missed the one in the `callConduit()` method, so the issue isn't fully fixed in production. Convert this adapter to use a real viewer (if one is available) more thoroughly. Test Plan: Ran rules in test console, saw field values. Will test in production again. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13048 Differential Revision: https://secure.phabricator.com/D18950 --- .../diffusion/herald/HeraldCommitAdapter.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index e0e15352b6..fc3c8c4f5b 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -135,13 +135,16 @@ final class HeraldCommitAdapter } public function loadAffectedPaths() { + $viewer = $this->getViewer(); + if ($this->affectedPaths === null) { $result = PhabricatorOwnerPathQuery::loadAffectedPaths( $this->getRepository(), $this->commit, - PhabricatorUser::getOmnipotentUser()); + $viewer); $this->affectedPaths = $result; } + return $this->affectedPaths; } @@ -172,6 +175,8 @@ final class HeraldCommitAdapter } public function loadDifferentialRevision() { + $viewer = $this->getViewer(); + if ($this->affectedRevision === null) { $this->affectedRevision = false; @@ -189,7 +194,7 @@ final class HeraldCommitAdapter $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($viewer) ->needReviewers(true) ->executeOne(); if ($revision) { @@ -197,6 +202,7 @@ final class HeraldCommitAdapter } } } + return $this->affectedRevision; } @@ -323,7 +329,7 @@ final class HeraldCommitAdapter } private function callConduit($method, array $params) { - $viewer = PhabricatorUser::getOmnipotentUser(); + $viewer = $this->getViewer(); $drequest = DiffusionRequest::newFromDictionary( array( From 40e9806e3c88500a67d382a5ec07594325f1b000 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 12:54:04 -0800 Subject: [PATCH 06/33] Remove the caret dropdown from transaction lists when no actions are available Summary: See PHI325. When a transaction group in Differential (or Pholio) only has an inline comment, it renders with a "V" caret but no actual dropdown menu. This caret renders in a "disabled" color, but the color is "kinda grey". The "active" color is "kinda grey with a dab of blue". Here's what they look like today: {F5401581} Just remove it. Test Plan: Viewed one of these, no longer saw the inactive caret. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18963 --- resources/celerity/map.php | 6 +++--- src/view/phui/PHUITimelineEventView.php | 8 +++----- webroot/rsrc/css/phui/phui-timeline-view.css | 4 ---- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index fece1e10ee..661ac58c15 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => '075f9867', + 'core.pkg.css' => '51debec3', 'core.pkg.js' => '4c79d74f', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -176,7 +176,7 @@ return array( 'rsrc/css/phui/phui-spacing.css' => '042804d6', 'rsrc/css/phui/phui-status.css' => 'd5263e49', 'rsrc/css/phui/phui-tag-view.css' => 'b4719c50', - 'rsrc/css/phui/phui-timeline-view.css' => 'e2ef62b1', + 'rsrc/css/phui/phui-timeline-view.css' => '6ddf8126', 'rsrc/css/phui/phui-two-column-view.css' => '44ec4951', 'rsrc/css/phui/workboards/phui-workboard-color.css' => '783cdff5', 'rsrc/css/phui/workboards/phui-workboard.css' => '3bc85455', @@ -873,7 +873,7 @@ return array( 'phui-status-list-view-css' => 'd5263e49', 'phui-tag-view-css' => 'b4719c50', 'phui-theme-css' => '9f261c6b', - 'phui-timeline-view-css' => 'e2ef62b1', + 'phui-timeline-view-css' => '6ddf8126', 'phui-two-column-view-css' => '44ec4951', 'phui-workboard-color-css' => '783cdff5', 'phui-workboard-view-css' => '3bc85455', diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index 161dd0c944..e64b9b06b2 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -301,18 +301,14 @@ final class PHUITimelineEventView extends AphrontView { $menu = null; $items = array(); - $has_menu = false; if (!$this->getIsPreview() && !$this->getHideCommentOptions()) { foreach ($this->getEventGroup() as $event) { $items[] = $event->getMenuItems($this->anchor); - if ($event->hasChildren()) { - $has_menu = true; - } } $items = array_mergev($items); } - if ($items || $has_menu) { + if ($items) { $icon = id(new PHUIIconView()) ->setIcon('fa-caret-down'); $aural = javelin_tag( @@ -351,6 +347,8 @@ final class PHUITimelineEventView extends AphrontView { )); $has_menu = true; + } else { + $has_menu = false; } // Render "extra" information (timestamp, etc). diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css index b0ae9c0044..6fae6802be 100644 --- a/webroot/rsrc/css/phui/phui-timeline-view.css +++ b/webroot/rsrc/css/phui/phui-timeline-view.css @@ -390,10 +390,6 @@ outline: none; } -.phui-timeline-menu .phui-icon-view { - color: {$lightgreytext}; -} - a.phui-timeline-menu .phui-icon-view { color: {$bluetext}; } From 1db281bcd1bbe141817fe6bc14a9d5dae3e612c5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 13:16:46 -0800 Subject: [PATCH 07/33] Fix a possible `count(null)` in PHUIInfoView Summary: See . PHP7.2 raises a warning about `count(scalar)` (GREAT!) and we have one here if the caller doesn't `setErrors(...)`. Test Plan: Sanity-checked usage of `$this->errors`. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18964 --- src/view/phui/PHUIInfoView.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/view/phui/PHUIInfoView.php b/src/view/phui/PHUIInfoView.php index 161e49108b..69d0549299 100644 --- a/src/view/phui/PHUIInfoView.php +++ b/src/view/phui/PHUIInfoView.php @@ -10,7 +10,7 @@ final class PHUIInfoView extends AphrontTagView { const SEVERITY_PLAIN = 'plain'; private $title; - private $errors; + private $errors = array(); private $severity = null; private $id; private $buttons = array(); From 213eb8e93de567fb4577fa8688873b69f1b01593 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 28 Jan 2018 18:24:42 -0800 Subject: [PATCH 08/33] Define common ID and PHID export fields in SearchEngine Summary: Ref T13049. All exportable objects should always have these fields, so make them builtins. This also sets things up for extensions (like custom fields). Test Plan: Exported user data, got the same export as before. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18951 --- .../query/DiffusionPullLogSearchEngine.php | 10 +-- .../query/PhabricatorPeopleSearchEngine.php | 10 +-- ...PhabricatorApplicationSearchController.php | 11 ---- .../PhabricatorApplicationSearchEngine.php | 65 ++++++++++++++++++- 4 files changed, 66 insertions(+), 30 deletions(-) diff --git a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php index 8d6102d4eb..fc7cee52ce 100644 --- a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php +++ b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php @@ -49,12 +49,6 @@ final class DiffusionPullLogSearchEngine protected function newExportFields() { return array( - id(new PhabricatorIDExportField()) - ->setKey('id') - ->setLabel(pht('ID')), - id(new PhabricatorPHIDExportField()) - ->setKey('phid') - ->setLabel(pht('PHID')), id(new PhabricatorPHIDExportField()) ->setKey('repositoryPHID') ->setLabel(pht('Repository PHID')), @@ -82,7 +76,7 @@ final class DiffusionPullLogSearchEngine ); } - public function newExport(array $events) { + protected function newExportData(array $events) { $viewer = $this->requireViewer(); $phids = array(); @@ -112,8 +106,6 @@ final class DiffusionPullLogSearchEngine } $export[] = array( - 'id' => $event->getID(), - 'phid' => $event->getPHID(), 'repositoryPHID' => $repository_phid, 'repository' => $repository_name, 'pullerPHID' => $puller_phid, diff --git a/src/applications/people/query/PhabricatorPeopleSearchEngine.php b/src/applications/people/query/PhabricatorPeopleSearchEngine.php index db2256a8b8..e0e1b5070e 100644 --- a/src/applications/people/query/PhabricatorPeopleSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleSearchEngine.php @@ -322,12 +322,6 @@ final class PhabricatorPeopleSearchEngine protected function newExportFields() { return array( - id(new PhabricatorIDExportField()) - ->setKey('id') - ->setLabel(pht('ID')), - id(new PhabricatorPHIDExportField()) - ->setKey('phid') - ->setLabel(pht('PHID')), id(new PhabricatorStringExportField()) ->setKey('username') ->setLabel(pht('Username')), @@ -340,14 +334,12 @@ final class PhabricatorPeopleSearchEngine ); } - public function newExport(array $users) { + protected function newExportData(array $users) { $viewer = $this->requireViewer(); $export = array(); foreach ($users as $user) { $export[] = array( - 'id' => $user->getID(), - 'phid' => $user->getPHID(), 'username' => $user->getUsername(), 'realName' => $user->getRealName(), 'created' => $user->getDateCreated(), diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index d3b05d1e3e..037a626f26 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -449,18 +449,7 @@ final class PhabricatorApplicationSearchController $format->setViewer($viewer); $export_data = $engine->newExport($objects); - - if (count($export_data) !== count($objects)) { - throw new Exception( - pht( - 'Search engine exported the wrong number of objects, expected '. - '%s but got %s.', - phutil_count($objects), - phutil_count($export_data))); - } - $objects = array_values($objects); - $export_data = array_values($export_data); $field_list = $engine->newExportFieldList(); $field_list = mpull($field_list, null, 'getKey'); diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 33efd890bb..e77c06a62b 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -1455,11 +1455,74 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } final public function newExportFieldList() { - return $this->newExportFields(); + $builtin_fields = array( + id(new PhabricatorIDExportField()) + ->setKey('id') + ->setLabel(pht('ID')), + id(new PhabricatorPHIDExportField()) + ->setKey('phid') + ->setLabel(pht('PHID')), + ); + + $fields = mpull($builtin_fields, null, 'getKey'); + + $export_fields = $this->newExportFields(); + foreach ($export_fields as $export_field) { + $key = $export_field->getKey(); + + if (isset($fields[$key])) { + throw new Exception( + pht( + 'Search engine ("%s") defines an export field with a key ("%s") '. + 'that collides with another field. Each field must have a '. + 'unique key.', + get_class($this), + $key)); + } + + $fields[$key] = $export_field; + } + + return $fields; + } + + final public function newExport(array $objects) { + $objects = array_values($objects); + $n = count($objects); + + $maps = array(); + foreach ($objects as $object) { + $maps[] = array( + 'id' => $object->getID(), + 'phid' => $object->getPHID(), + ); + } + + $export_data = $this->newExportData($objects); + $export_data = array_values($export_data); + if (count($export_data) !== count($objects)) { + throw new Exception( + pht( + 'Search engine ("%s") exported the wrong number of objects, '. + 'expected %s but got %s.', + get_class($this), + phutil_count($objects), + phutil_count($export_data))); + } + + for ($ii = 0; $ii < $n; $ii++) { + $maps[$ii] += $export_data[$ii]; + } + + return $maps; } protected function newExportFields() { return array(); } + protected function newExportData(array $objects) { + throw new PhutilMethodNotImplementedException(); + } + } From 0de6210808d7de78b0af0bea4e45f8537435be85 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 28 Jan 2018 18:32:22 -0800 Subject: [PATCH 09/33] Give data exporters a header row Summary: Depends on D18951. Ref T13049. When we export to CSV or plain text, add a header row in the first line of the file to explain what each column means. This often isn't obvious with PHIDs, etc. JSON has keys and is essentially self-labeling, so don't do anything special. Test Plan: Exported CSV and text, saw new headers. Exported JSON, no changes. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18952 --- .../PhabricatorApplicationSearchController.php | 1 + .../export/PhabricatorCSVExportFormat.php | 17 +++++++++++++++-- .../export/PhabricatorExportFormat.php | 4 ++++ .../export/PhabricatorTextExportFormat.php | 18 +++++++++++++++--- 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 037a626f26..0402102e2b 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -454,6 +454,7 @@ final class PhabricatorApplicationSearchController $field_list = $engine->newExportFieldList(); $field_list = mpull($field_list, null, 'getKey'); + $format->addHeaders($field_list); for ($ii = 0; $ii < count($objects); $ii++) { $format->addObject($objects[$ii], $field_list, $export_data[$ii]); } diff --git a/src/infrastructure/export/PhabricatorCSVExportFormat.php b/src/infrastructure/export/PhabricatorCSVExportFormat.php index d9662467ec..67f0a4963a 100644 --- a/src/infrastructure/export/PhabricatorCSVExportFormat.php +++ b/src/infrastructure/export/PhabricatorCSVExportFormat.php @@ -23,21 +23,34 @@ final class PhabricatorCSVExportFormat return 'text/csv'; } + public function addHeaders(array $fields) { + $headers = mpull($fields, 'getLabel'); + $this->addRow($headers); + } + public function addObject($object, array $fields, array $map) { $values = array(); foreach ($fields as $key => $field) { $value = $map[$key]; $value = $field->getTextValue($value); + $values[] = $value; + } + $this->addRow($values); + } + + private function addRow(array $values) { + $row = array(); + foreach ($values as $value) { if (preg_match('/\s|,|\"/', $value)) { $value = str_replace('"', '""', $value); $value = '"'.$value.'"'; } - $values[] = $value; + $row[] = $value; } - $this->rows[] = implode(',', $values); + $this->rows[] = implode(',', $row); } public function newFileData() { diff --git a/src/infrastructure/export/PhabricatorExportFormat.php b/src/infrastructure/export/PhabricatorExportFormat.php index a1da4e90d8..9a8e035c58 100644 --- a/src/infrastructure/export/PhabricatorExportFormat.php +++ b/src/infrastructure/export/PhabricatorExportFormat.php @@ -22,6 +22,10 @@ abstract class PhabricatorExportFormat abstract public function getMIMEContentType(); abstract public function getFileExtension(); + public function addHeaders(array $fields) { + return; + } + abstract public function addObject($object, array $fields, array $map); abstract public function newFileData(); diff --git a/src/infrastructure/export/PhabricatorTextExportFormat.php b/src/infrastructure/export/PhabricatorTextExportFormat.php index ec308f2eb5..d51e199f91 100644 --- a/src/infrastructure/export/PhabricatorTextExportFormat.php +++ b/src/infrastructure/export/PhabricatorTextExportFormat.php @@ -23,17 +23,29 @@ final class PhabricatorTextExportFormat return 'text/plain'; } + public function addHeaders(array $fields) { + $headers = mpull($fields, 'getLabel'); + $this->addRow($headers); + } + public function addObject($object, array $fields, array $map) { $values = array(); foreach ($fields as $key => $field) { $value = $map[$key]; $value = $field->getTextValue($value); - $value = addcslashes($value, "\0..\37\\\177..\377"); - $values[] = $value; } - $this->rows[] = implode("\t", $values); + $this->addRow($values); + } + + private function addRow(array $values) { + $row = array(); + foreach ($values as $value) { + $row[] = addcslashes($value, "\0..\37\\\177..\377"); + } + + $this->rows[] = implode("\t", $row); } public function newFileData() { From 8b8a3142b3102fdf8199ee549ff245d90483f50b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 28 Jan 2018 18:47:42 -0800 Subject: [PATCH 10/33] Support export of data in files larger than 8MB Summary: Depends on D18952. Ref T13049. For files larger than 8MB, we need to engage the chunk storage engine. `PhabricatorFile::newFromFileData()` always writes a single chunk, and can't handle files larger than the mandatory chunk threshold (8MB). Use `IteratorUploadSource`, which can, and "stream" the data into it. This should raise the limit from 8MB to 2GB (maximum size of a string in PHP). If we need to go above 2GB we could stream CSV and text pretty easily, and JSON without too much trouble, but Excel might be trickier. Hopefully no one is trying to export 2GB+ datafiles, though. Test Plan: - Changed the JSON exporter to just export 8MB of the letter "q": `return str_repeat('q', 1024 * 1024 * 9);`. - Before change: fatal, "no storage engine can store this file". - After change: export works cleanly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18953 --- .../files/storage/PhabricatorFile.php | 8 +++-- .../PhabricatorFileUploadSource.php | 30 +++++++++++++++++++ ...PhabricatorApplicationSearchController.php | 23 ++++++++------ 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index d3dc5208d2..2b8624b9c0 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -272,8 +272,12 @@ final class PhabricatorFile extends PhabricatorFileDAO $file->setByteSize($length); // NOTE: Once we receive the first chunk, we'll detect its MIME type and - // update the parent file. This matters for large media files like video. - $file->setMimeType('application/octet-stream'); + // update the parent file if a MIME type hasn't been provided. This matters + // for large media files like video. + $mime_type = idx($params, 'mime-type'); + if (!strlen($mime_type)) { + $file->setMimeType('application/octet-stream'); + } $chunked_hash = idx($params, 'chunkedHash'); diff --git a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php index bf213a417e..87a4486869 100644 --- a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php +++ b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php @@ -6,6 +6,8 @@ abstract class PhabricatorFileUploadSource private $name; private $relativeTTL; private $viewPolicy; + private $mimeType; + private $authorPHID; private $rope; private $data; @@ -51,6 +53,24 @@ abstract class PhabricatorFileUploadSource return $this->byteLimit; } + public function setMIMEType($mime_type) { + $this->mimeType = $mime_type; + return $this; + } + + public function getMIMEType() { + return $this->mimeType; + } + + public function setAuthorPHID($author_phid) { + $this->authorPHID = $author_phid; + return $this; + } + + public function getAuthorPHID() { + return $this->authorPHID; + } + public function uploadFile() { if (!$this->shouldChunkFile()) { return $this->writeSingleFile(); @@ -245,6 +265,16 @@ abstract class PhabricatorFileUploadSource $parameters['ttl.relative'] = $ttl; } + $mime_type = $this->getMimeType(); + if ($mime_type !== null) { + $parameters['mime-type'] = $mime_type; + } + + $author_phid = $this->getAuthorPHID(); + if ($author_phid !== null) { + $parameters['authorPHID'] = $author_phid; + } + return $parameters; } diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 0402102e2b..e980498c42 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -461,15 +461,20 @@ final class PhabricatorApplicationSearchController $export_result = $format->newFileData(); - $file = PhabricatorFile::newFromFileData( - $export_result, - array( - 'name' => $filename, - 'authorPHID' => $viewer->getPHID(), - 'ttl.relative' => phutil_units('15 minutes in seconds'), - 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, - 'mime-type' => $mime_type, - )); + // We have all the data in one big string and aren't actually + // streaming it, but pretending that we are allows us to actviate + // the chunk engine and store large files. + $iterator = new ArrayIterator(array($export_result)); + + $source = id(new PhabricatorIteratorFileUploadSource()) + ->setName($filename) + ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) + ->setMIMEType($mime_type) + ->setRelativeTTL(phutil_units('60 minutes in seconds')) + ->setAuthorPHID($viewer->getPHID()) + ->setIterator($iterator); + + $file = $source->uploadFile(); return $this->newDialog() ->setTitle(pht('Download Results')) From a067f64ebb326f65078322aabb0790a7ddb34d66 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 28 Jan 2018 19:30:41 -0800 Subject: [PATCH 11/33] Support export engine extensions and implement an extension for custom fields Summary: Depends on D18953. Ref T13049. Allow applications and infrastructure to supplement exportable fields for objects. Then, implement an extension for custom fields. Only a couple field types (int, string) are supported for now. Test Plan: Added some custom fields to Users, populated them, exported users. Saw custom fields in the export. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18954 --- src/__phutil_library_map__.php | 4 + .../PhabricatorApplicationSearchEngine.php | 58 +++++++++++++ .../field/PhabricatorCustomField.php | 43 ++++++++++ .../PhabricatorStandardCustomField.php | 3 + .../PhabricatorStandardCustomFieldInt.php | 4 + .../PhabricatorStandardCustomFieldText.php | 4 + ...icatorCustomFieldExportEngineExtension.php | 86 +++++++++++++++++++ .../PhabricatorExportEngineExtension.php | 31 +++++++ 8 files changed, 233 insertions(+) create mode 100644 src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php create mode 100644 src/infrastructure/export/PhabricatorExportEngineExtension.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1ad3565809..03ac41fe7f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2583,6 +2583,7 @@ phutil_register_library_map(array( 'PhabricatorCustomFieldEditEngineExtension' => 'infrastructure/customfield/engineextension/PhabricatorCustomFieldEditEngineExtension.php', 'PhabricatorCustomFieldEditField' => 'infrastructure/customfield/editor/PhabricatorCustomFieldEditField.php', 'PhabricatorCustomFieldEditType' => 'infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php', + 'PhabricatorCustomFieldExportEngineExtension' => 'infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php', 'PhabricatorCustomFieldFulltextEngineExtension' => 'infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php', 'PhabricatorCustomFieldHeraldAction' => 'infrastructure/customfield/herald/PhabricatorCustomFieldHeraldAction.php', 'PhabricatorCustomFieldHeraldActionGroup' => 'infrastructure/customfield/herald/PhabricatorCustomFieldHeraldActionGroup.php', @@ -2847,6 +2848,7 @@ phutil_register_library_map(array( 'PhabricatorEventType' => 'infrastructure/events/constant/PhabricatorEventType.php', 'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php', 'PhabricatorExecFutureFileUploadSource' => 'applications/files/uploadsource/PhabricatorExecFutureFileUploadSource.php', + 'PhabricatorExportEngineExtension' => 'infrastructure/export/PhabricatorExportEngineExtension.php', 'PhabricatorExportField' => 'infrastructure/export/PhabricatorExportField.php', 'PhabricatorExportFormat' => 'infrastructure/export/PhabricatorExportFormat.php', 'PhabricatorExtendedPolicyInterface' => 'applications/policy/interface/PhabricatorExtendedPolicyInterface.php', @@ -7991,6 +7993,7 @@ phutil_register_library_map(array( 'PhabricatorCustomFieldEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorCustomFieldEditField' => 'PhabricatorEditField', 'PhabricatorCustomFieldEditType' => 'PhabricatorEditType', + 'PhabricatorCustomFieldExportEngineExtension' => 'PhabricatorExportEngineExtension', 'PhabricatorCustomFieldFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', 'PhabricatorCustomFieldHeraldAction' => 'HeraldAction', 'PhabricatorCustomFieldHeraldActionGroup' => 'HeraldActionGroup', @@ -8280,6 +8283,7 @@ phutil_register_library_map(array( 'PhabricatorEventType' => 'PhutilEventType', 'PhabricatorExampleEventListener' => 'PhabricatorEventListener', 'PhabricatorExecFutureFileUploadSource' => 'PhabricatorFileUploadSource', + 'PhabricatorExportEngineExtension' => 'Phobject', 'PhabricatorExportField' => 'Phobject', 'PhabricatorExportFormat' => 'Phobject', 'PhabricatorExtendingPhabricatorConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index e77c06a62b..3de7b9c4b9 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -1483,6 +1483,26 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { $fields[$key] = $export_field; } + $extensions = $this->newExportExtensions(); + foreach ($extensions as $extension) { + $extension_fields = $extension->newExportFields(); + foreach ($extension_fields as $extension_field) { + $key = $extension_field->getKey(); + + if (isset($fields[$key])) { + throw new Exception( + pht( + 'Export engine extension ("%s") defines an export field with '. + 'a key ("%s") that collides with another field. Each field '. + 'must have a unique key.', + get_class($extension_field), + $key)); + } + + $fields[$key] = $extension_field; + } + } + return $fields; } @@ -1514,6 +1534,25 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { $maps[$ii] += $export_data[$ii]; } + $extensions = $this->newExportExtensions(); + foreach ($extensions as $extension) { + $extension_data = $extension->newExportData($objects); + $extension_data = array_values($extension_data); + if (count($export_data) !== count($objects)) { + throw new Exception( + pht( + 'Export engine extension ("%s") exported the wrong number of '. + 'objects, expected %s but got %s.', + get_class($extension), + phutil_count($objects), + phutil_count($export_data))); + } + + for ($ii = 0; $ii < $n; $ii++) { + $maps[$ii] += $extension_data[$ii]; + } + } + return $maps; } @@ -1525,4 +1564,23 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { throw new PhutilMethodNotImplementedException(); } + private function newExportExtensions() { + $object = $this->newResultObject(); + $viewer = $this->requireViewer(); + + $extensions = PhabricatorExportEngineExtension::getAllExtensions(); + + $supported = array(); + foreach ($extensions as $extension) { + $extension = clone $extension; + $extension->setViewer($viewer); + + if ($extension->supportsObject($object)) { + $supported[] = $extension; + } + } + + return $supported; + } + } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index c96f09f369..818bf119ff 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -35,6 +35,7 @@ abstract class PhabricatorCustomField extends Phobject { const ROLE_HERALD = 'herald'; const ROLE_EDITENGINE = 'EditEngine'; const ROLE_HERALDACTION = 'herald.action'; + const ROLE_EXPORT = 'export'; /* -( Building Applications with Custom Fields )--------------------------- */ @@ -299,6 +300,8 @@ abstract class PhabricatorCustomField extends Phobject { case self::ROLE_EDITENGINE: return $this->shouldAppearInEditView() || $this->shouldAppearInEditEngine(); + case self::ROLE_EXPORT: + return $this->shouldAppearInDataExport(); case self::ROLE_DEFAULT: return true; default: @@ -1362,6 +1365,46 @@ abstract class PhabricatorCustomField extends Phobject { } +/* -( Data Export )-------------------------------------------------------- */ + + + public function shouldAppearInDataExport() { + if ($this->proxy) { + return $this->proxy->shouldAppearInDataExport(); + } + + try { + $this->newExportFieldType(); + return true; + } catch (PhabricatorCustomFieldImplementationIncompleteException $ex) { + return false; + } + } + + public function newExportField() { + if ($this->proxy) { + return $this->proxy->newExportField(); + } + + return $this->newExportFieldType() + ->setLabel($this->getFieldName()); + } + + public function newExportData() { + if ($this->proxy) { + return $this->proxy->newExportData(); + } + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + protected function newExportFieldType() { + if ($this->proxy) { + return $this->proxy->newExportFieldType(); + } + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + + /* -( Conduit )------------------------------------------------------------ */ diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index f617d3a849..5bd6256b73 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -496,5 +496,8 @@ abstract class PhabricatorStandardCustomField return $this->getFieldValue(); } + public function newExportData() { + return $this->getFieldValue(); + } } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php index 4a3e45d757..f06c30d482 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php @@ -124,4 +124,8 @@ final class PhabricatorStandardCustomFieldInt return new ConduitIntParameterType(); } + protected function newExportFieldType() { + return new PhabricatorIntExportField(); + } + } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php index 8242c477fd..56164bb7b5 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldText.php @@ -76,4 +76,8 @@ final class PhabricatorStandardCustomFieldText return new ConduitStringParameterType(); } + protected function newExportFieldType() { + return new PhabricatorStringExportField(); + } + } diff --git a/src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php b/src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php new file mode 100644 index 0000000000..c2a9eddae2 --- /dev/null +++ b/src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php @@ -0,0 +1,86 @@ +object = $object; + return ($object instanceof PhabricatorCustomFieldInterface); + } + + public function newExportFields() { + $prototype = $this->object; + + $fields = $this->newCustomFields($prototype); + + $results = array(); + foreach ($fields as $field) { + $field_key = $field->getModernFieldKey(); + + $results[] = $field->newExportField() + ->setKey($field_key); + } + + return $results; + } + + public function newExportData(array $objects) { + $viewer = $this->getViewer(); + + $field_map = array(); + foreach ($objects as $object) { + $object_phid = $object->getPHID(); + + $fields = PhabricatorCustomField::getObjectFields( + $object, + PhabricatorCustomField::ROLE_EXPORT); + + $fields + ->setViewer($viewer) + ->readFieldsFromObject($object); + + $field_map[$object_phid] = $fields; + } + + $all_fields = array(); + foreach ($field_map as $field_list) { + foreach ($field_list->getFields() as $field) { + $all_fields[] = $field; + } + } + + id(new PhabricatorCustomFieldStorageQuery()) + ->addFields($all_fields) + ->execute(); + + $results = array(); + foreach ($objects as $object) { + $object_phid = $object->getPHID(); + $object_fields = $field_map[$object_phid]; + + $map = array(); + foreach ($object_fields->getFields() as $field) { + $key = $field->getModernFieldKey(); + $map[$key] = $field->newExportData(); + } + + $results[] = $map; + } + + return $results; + } + + private function newCustomFields($object) { + $fields = PhabricatorCustomField::getObjectFields( + $object, + PhabricatorCustomField::ROLE_EXPORT); + $fields->setViewer($this->getViewer()); + + return $fields->getFields(); + } + +} diff --git a/src/infrastructure/export/PhabricatorExportEngineExtension.php b/src/infrastructure/export/PhabricatorExportEngineExtension.php new file mode 100644 index 0000000000..01d4471ef2 --- /dev/null +++ b/src/infrastructure/export/PhabricatorExportEngineExtension.php @@ -0,0 +1,31 @@ +getPhobjectClassConstant('EXTENSIONKEY'); + } + + final public function setViewer($viewer) { + $this->viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + abstract public function supportsObject($object); + abstract public function newExportFields(); + abstract public function newExportData(array $objects); + + final public static function getAllExtensions() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getExtensionKey') + ->execute(); + } + +} From 0409279595464924d1dc728a8c6830228d75dc5f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 06:50:11 -0800 Subject: [PATCH 12/33] Support Excel as a data export format Summary: Depends on D18954. Ref T13049. This brings over the existing Maniphest Excel export pipeline in a generic way. The `ExportField` classes know directly that `PHPExcel` exists, which is a little sketchy, but writing an Excel indirection layer sounds like a lot of work and I don't anticipate us changing Excel backends anytime soon, so trying to abstract this feels YAGNI. This doesn't bring over the install instructions for PHPExcel or the detection of whether or not it exists. I'll bring that over in a future change. Test Plan: Exported users as Excel, opened them up, got a sensible-looking Excel sheet. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18955 --- src/__phutil_library_map__.php | 2 + ...PhabricatorApplicationSearchController.php | 7 +- .../export/PhabricatorEpochExportField.php | 20 +++ .../export/PhabricatorExcelExportFormat.php | 145 ++++++++++++++++++ .../export/PhabricatorExportField.php | 15 ++ .../export/PhabricatorExportFormat.php | 10 ++ .../export/PhabricatorIDExportField.php | 4 + .../export/PhabricatorIntExportField.php | 15 ++ .../export/PhabricatorPHIDExportField.php | 8 +- 9 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 src/infrastructure/export/PhabricatorExcelExportFormat.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 03ac41fe7f..3ef9f4ec3b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2847,6 +2847,7 @@ phutil_register_library_map(array( 'PhabricatorEventListener' => 'infrastructure/events/PhabricatorEventListener.php', 'PhabricatorEventType' => 'infrastructure/events/constant/PhabricatorEventType.php', 'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php', + 'PhabricatorExcelExportFormat' => 'infrastructure/export/PhabricatorExcelExportFormat.php', 'PhabricatorExecFutureFileUploadSource' => 'applications/files/uploadsource/PhabricatorExecFutureFileUploadSource.php', 'PhabricatorExportEngineExtension' => 'infrastructure/export/PhabricatorExportEngineExtension.php', 'PhabricatorExportField' => 'infrastructure/export/PhabricatorExportField.php', @@ -8282,6 +8283,7 @@ phutil_register_library_map(array( 'PhabricatorEventListener' => 'PhutilEventListener', 'PhabricatorEventType' => 'PhutilEventType', 'PhabricatorExampleEventListener' => 'PhabricatorEventListener', + 'PhabricatorExcelExportFormat' => 'PhabricatorExportFormat', 'PhabricatorExecFutureFileUploadSource' => 'PhabricatorFileUploadSource', 'PhabricatorExportEngineExtension' => 'Phobject', 'PhabricatorExportField' => 'Phobject', diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index e980498c42..88cfd3c137 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -410,8 +410,10 @@ final class PhabricatorApplicationSearchController if ($named_query) { $filename = $named_query->getQueryName(); + $sheet_title = $named_query->getQueryName(); } else { $filename = $engine->getResultTypeDescription(); + $sheet_title = $engine->getResultTypeDescription(); } $filename = phutil_utf8_strtolower($filename); $filename = PhabricatorFile::normalizeFileName($filename); @@ -445,8 +447,9 @@ final class PhabricatorApplicationSearchController $mime_type = $format->getMIMEContentType(); $filename = $filename.'.'.$extension; - $format = clone $format; - $format->setViewer($viewer); + $format = id(clone $format) + ->setViewer($viewer) + ->setTitle($sheet_title); $export_data = $engine->newExport($objects); $objects = array_values($objects); diff --git a/src/infrastructure/export/PhabricatorEpochExportField.php b/src/infrastructure/export/PhabricatorEpochExportField.php index a19e60b50e..4dffde5aa8 100644 --- a/src/infrastructure/export/PhabricatorEpochExportField.php +++ b/src/infrastructure/export/PhabricatorEpochExportField.php @@ -24,4 +24,24 @@ final class PhabricatorEpochExportField return (int)$value; } + public function getPHPExcelValue($value) { + $epoch = $this->getNaturalValue($value); + + $seconds_per_day = phutil_units('1 day in seconds'); + $offset = ($seconds_per_day * 25569); + + return ($epoch + $offset) / $seconds_per_day; + } + + /** + * @phutil-external-symbol class PHPExcel_Style_NumberFormat + */ + public function formatPHPExcelCell($cell, $style) { + $code = PHPExcel_Style_NumberFormat::FORMAT_DATE_YYYYMMDD2; + + $style + ->getNumberFormat() + ->setFormatCode($code); + } + } diff --git a/src/infrastructure/export/PhabricatorExcelExportFormat.php b/src/infrastructure/export/PhabricatorExcelExportFormat.php new file mode 100644 index 0000000000..633d98fa53 --- /dev/null +++ b/src/infrastructure/export/PhabricatorExcelExportFormat.php @@ -0,0 +1,145 @@ +getSheet(); + + $header_format = array( + 'font' => array( + 'bold' => true, + ), + ); + + $row = 1; + $col = 0; + foreach ($fields as $field) { + $cell_value = $field->getLabel(); + + $cell_name = $this->getCellName($col, $row); + + $cell = $sheet->setCellValue( + $cell_name, + $cell_value, + $return_cell = true); + + $sheet->getStyle($cell_name)->applyFromArray($header_format); + $cell->setDataType(PHPExcel_Cell_DataType::TYPE_STRING); + + $width = $field->getCharacterWidth(); + if ($width !== null) { + $col_name = $this->getCellName($col); + $sheet->getColumnDimension($col_name) + ->setWidth($width); + } + + $col++; + } + } + + public function addObject($object, array $fields, array $map) { + $sheet = $this->getSheet(); + + $col = 0; + foreach ($fields as $key => $field) { + $cell_value = $map[$key]; + $cell_value = $field->getPHPExcelValue($cell_value); + + $cell_name = $this->getCellName($col, $this->rowCursor); + + $cell = $sheet->setCellValue( + $cell_name, + $cell_value, + $return_cell = true); + + $style = $sheet->getStyle($cell_name); + $field->formatPHPExcelCell($cell, $style); + + $col++; + } + + $this->rowCursor++; + } + + /** + * @phutil-external-symbol class PHPExcel_IOFactory + */ + public function newFileData() { + $workbook = $this->getWorkbook(); + $writer = PHPExcel_IOFactory::createWriter($workbook, 'Excel2007'); + + ob_start(); + $writer->save('php://output'); + $data = ob_get_clean(); + + return $data; + } + + private function getWorkbook() { + if (!$this->workbook) { + $this->workbook = $this->newWorkbook(); + } + return $this->workbook; + } + + /** + * @phutil-external-symbol class PHPExcel + */ + private function newWorkbook() { + include_once 'PHPExcel.php'; + return new PHPExcel(); + } + + private function getSheet() { + if (!$this->sheet) { + $workbook = $this->getWorkbook(); + + $sheet = $workbook->setActiveSheetIndex(0); + $sheet->setTitle($this->getTitle()); + + $this->sheet = $sheet; + + // The row cursor starts on the second row, after the header row. + $this->rowCursor = 2; + } + + return $this->sheet; + } + + private function getCellName($col, $row = null) { + $col_name = chr(ord('A') + $col); + + if ($row === null) { + return $col_name; + } + + return $col_name.$row; + } + +} diff --git a/src/infrastructure/export/PhabricatorExportField.php b/src/infrastructure/export/PhabricatorExportField.php index 3efb7a8b9a..85e21b3e37 100644 --- a/src/infrastructure/export/PhabricatorExportField.php +++ b/src/infrastructure/export/PhabricatorExportField.php @@ -32,4 +32,19 @@ abstract class PhabricatorExportField return $value; } + public function getPHPExcelValue($value) { + return $this->getTextValue($value); + } + + /** + * @phutil-external-symbol class PHPExcel_Cell_DataType + */ + public function formatPHPExcelCell($cell, $style) { + $cell->setDataType(PHPExcel_Cell_DataType::TYPE_STRING); + } + + public function getCharacterWidth() { + return 24; + } + } diff --git a/src/infrastructure/export/PhabricatorExportFormat.php b/src/infrastructure/export/PhabricatorExportFormat.php index 9a8e035c58..7e174f5197 100644 --- a/src/infrastructure/export/PhabricatorExportFormat.php +++ b/src/infrastructure/export/PhabricatorExportFormat.php @@ -4,6 +4,7 @@ abstract class PhabricatorExportFormat extends Phobject { private $viewer; + private $title; final public function getExportFormatKey() { return $this->getPhobjectClassConstant('EXPORTKEY'); @@ -18,6 +19,15 @@ abstract class PhabricatorExportFormat return $this->viewer; } + final public function setTitle($title) { + $this->title = $title; + return $this; + } + + final public function getTitle() { + return $this->title; + } + abstract public function getExportFormatName(); abstract public function getMIMEContentType(); abstract public function getFileExtension(); diff --git a/src/infrastructure/export/PhabricatorIDExportField.php b/src/infrastructure/export/PhabricatorIDExportField.php index 5b29fdb21d..1ef3d53370 100644 --- a/src/infrastructure/export/PhabricatorIDExportField.php +++ b/src/infrastructure/export/PhabricatorIDExportField.php @@ -7,4 +7,8 @@ final class PhabricatorIDExportField return (int)$value; } + public function getCharacterWidth() { + return 12; + } + } diff --git a/src/infrastructure/export/PhabricatorIntExportField.php b/src/infrastructure/export/PhabricatorIntExportField.php index 3363f9b5d5..57f7e0ab29 100644 --- a/src/infrastructure/export/PhabricatorIntExportField.php +++ b/src/infrastructure/export/PhabricatorIntExportField.php @@ -4,7 +4,22 @@ final class PhabricatorIntExportField extends PhabricatorExportField { public function getNaturalValue($value) { + if ($value === null) { + return $value; + } + return (int)$value; } + /** + * @phutil-external-symbol class PHPExcel_Cell_DataType + */ + public function formatPHPExcelCell($cell, $style) { + $cell->setDataType(PHPExcel_Cell_DataType::TYPE_NUMERIC); + } + + public function getCharacterWidth() { + return 8; + } + } diff --git a/src/infrastructure/export/PhabricatorPHIDExportField.php b/src/infrastructure/export/PhabricatorPHIDExportField.php index 7c08ae0226..052c73fbd6 100644 --- a/src/infrastructure/export/PhabricatorPHIDExportField.php +++ b/src/infrastructure/export/PhabricatorPHIDExportField.php @@ -1,4 +1,10 @@ Date: Mon, 29 Jan 2018 07:24:32 -0800 Subject: [PATCH 13/33] Organize the export code into subdirectories Summary: Depends on D18955. Ref T13049. This directory was getting a little cluttered with different kinds of code. Put the formats (csv, json, ...), the field types (int, string, epoch, ...) and the engine-related stuff in subdirectories. Test Plan: wow so aesthetic Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18956 --- src/__phutil_library_map__.php | 26 +++++++++---------- ...icatorCustomFieldExportEngineExtension.php | 0 .../PhabricatorExportEngineExtension.php | 0 .../PhabricatorEpochExportField.php | 0 .../{ => field}/PhabricatorExportField.php | 0 .../{ => field}/PhabricatorIDExportField.php | 0 .../{ => field}/PhabricatorIntExportField.php | 0 .../PhabricatorPHIDExportField.php | 0 .../PhabricatorStringExportField.php | 0 .../PhabricatorCSVExportFormat.php | 0 .../PhabricatorExcelExportFormat.php | 0 .../{ => format}/PhabricatorExportFormat.php | 0 .../PhabricatorJSONExportFormat.php | 0 .../PhabricatorTextExportFormat.php | 0 14 files changed, 13 insertions(+), 13 deletions(-) rename src/infrastructure/export/{ => engine}/PhabricatorCustomFieldExportEngineExtension.php (100%) rename src/infrastructure/export/{ => engine}/PhabricatorExportEngineExtension.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorEpochExportField.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorExportField.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorIDExportField.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorIntExportField.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorPHIDExportField.php (100%) rename src/infrastructure/export/{ => field}/PhabricatorStringExportField.php (100%) rename src/infrastructure/export/{ => format}/PhabricatorCSVExportFormat.php (100%) rename src/infrastructure/export/{ => format}/PhabricatorExcelExportFormat.php (100%) rename src/infrastructure/export/{ => format}/PhabricatorExportFormat.php (100%) rename src/infrastructure/export/{ => format}/PhabricatorJSONExportFormat.php (100%) rename src/infrastructure/export/{ => format}/PhabricatorTextExportFormat.php (100%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3ef9f4ec3b..8ada6932ae 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2232,7 +2232,7 @@ phutil_register_library_map(array( 'PhabricatorBulkEngine' => 'applications/transactions/bulk/PhabricatorBulkEngine.php', 'PhabricatorBulkManagementMakeSilentWorkflow' => 'applications/transactions/bulk/management/PhabricatorBulkManagementMakeSilentWorkflow.php', 'PhabricatorBulkManagementWorkflow' => 'applications/transactions/bulk/management/PhabricatorBulkManagementWorkflow.php', - 'PhabricatorCSVExportFormat' => 'infrastructure/export/PhabricatorCSVExportFormat.php', + 'PhabricatorCSVExportFormat' => 'infrastructure/export/format/PhabricatorCSVExportFormat.php', 'PhabricatorCacheDAO' => 'applications/cache/storage/PhabricatorCacheDAO.php', 'PhabricatorCacheEngine' => 'applications/system/engine/PhabricatorCacheEngine.php', 'PhabricatorCacheEngineExtension' => 'applications/system/engine/PhabricatorCacheEngineExtension.php', @@ -2583,7 +2583,7 @@ phutil_register_library_map(array( 'PhabricatorCustomFieldEditEngineExtension' => 'infrastructure/customfield/engineextension/PhabricatorCustomFieldEditEngineExtension.php', 'PhabricatorCustomFieldEditField' => 'infrastructure/customfield/editor/PhabricatorCustomFieldEditField.php', 'PhabricatorCustomFieldEditType' => 'infrastructure/customfield/editor/PhabricatorCustomFieldEditType.php', - 'PhabricatorCustomFieldExportEngineExtension' => 'infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php', + 'PhabricatorCustomFieldExportEngineExtension' => 'infrastructure/export/engine/PhabricatorCustomFieldExportEngineExtension.php', 'PhabricatorCustomFieldFulltextEngineExtension' => 'infrastructure/customfield/engineextension/PhabricatorCustomFieldFulltextEngineExtension.php', 'PhabricatorCustomFieldHeraldAction' => 'infrastructure/customfield/herald/PhabricatorCustomFieldHeraldAction.php', 'PhabricatorCustomFieldHeraldActionGroup' => 'infrastructure/customfield/herald/PhabricatorCustomFieldHeraldActionGroup.php', @@ -2841,17 +2841,17 @@ phutil_register_library_map(array( 'PhabricatorEnv' => 'infrastructure/env/PhabricatorEnv.php', 'PhabricatorEnvTestCase' => 'infrastructure/env/__tests__/PhabricatorEnvTestCase.php', 'PhabricatorEpochEditField' => 'applications/transactions/editfield/PhabricatorEpochEditField.php', - 'PhabricatorEpochExportField' => 'infrastructure/export/PhabricatorEpochExportField.php', + 'PhabricatorEpochExportField' => 'infrastructure/export/field/PhabricatorEpochExportField.php', 'PhabricatorEvent' => 'infrastructure/events/PhabricatorEvent.php', 'PhabricatorEventEngine' => 'infrastructure/events/PhabricatorEventEngine.php', 'PhabricatorEventListener' => 'infrastructure/events/PhabricatorEventListener.php', 'PhabricatorEventType' => 'infrastructure/events/constant/PhabricatorEventType.php', 'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php', - 'PhabricatorExcelExportFormat' => 'infrastructure/export/PhabricatorExcelExportFormat.php', + 'PhabricatorExcelExportFormat' => 'infrastructure/export/format/PhabricatorExcelExportFormat.php', 'PhabricatorExecFutureFileUploadSource' => 'applications/files/uploadsource/PhabricatorExecFutureFileUploadSource.php', - 'PhabricatorExportEngineExtension' => 'infrastructure/export/PhabricatorExportEngineExtension.php', - 'PhabricatorExportField' => 'infrastructure/export/PhabricatorExportField.php', - 'PhabricatorExportFormat' => 'infrastructure/export/PhabricatorExportFormat.php', + 'PhabricatorExportEngineExtension' => 'infrastructure/export/engine/PhabricatorExportEngineExtension.php', + 'PhabricatorExportField' => 'infrastructure/export/field/PhabricatorExportField.php', + 'PhabricatorExportFormat' => 'infrastructure/export/format/PhabricatorExportFormat.php', 'PhabricatorExtendedPolicyInterface' => 'applications/policy/interface/PhabricatorExtendedPolicyInterface.php', 'PhabricatorExtendingPhabricatorConfigOptions' => 'applications/config/option/PhabricatorExtendingPhabricatorConfigOptions.php', 'PhabricatorExtensionsSetupCheck' => 'applications/config/check/PhabricatorExtensionsSetupCheck.php', @@ -3072,7 +3072,7 @@ phutil_register_library_map(array( 'PhabricatorHomeProfileMenuItem' => 'applications/home/menuitem/PhabricatorHomeProfileMenuItem.php', 'PhabricatorHovercardEngineExtension' => 'applications/search/engineextension/PhabricatorHovercardEngineExtension.php', 'PhabricatorHovercardEngineExtensionModule' => 'applications/search/engineextension/PhabricatorHovercardEngineExtensionModule.php', - 'PhabricatorIDExportField' => 'infrastructure/export/PhabricatorIDExportField.php', + 'PhabricatorIDExportField' => 'infrastructure/export/field/PhabricatorIDExportField.php', 'PhabricatorIDsSearchEngineExtension' => 'applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php', 'PhabricatorIDsSearchField' => 'applications/search/field/PhabricatorIDsSearchField.php', 'PhabricatorIconDatasource' => 'applications/files/typeahead/PhabricatorIconDatasource.php', @@ -3096,7 +3096,7 @@ phutil_register_library_map(array( 'PhabricatorInlineSummaryView' => 'infrastructure/diff/view/PhabricatorInlineSummaryView.php', 'PhabricatorInstructionsEditField' => 'applications/transactions/editfield/PhabricatorInstructionsEditField.php', 'PhabricatorIntConfigType' => 'applications/config/type/PhabricatorIntConfigType.php', - 'PhabricatorIntExportField' => 'infrastructure/export/PhabricatorIntExportField.php', + 'PhabricatorIntExportField' => 'infrastructure/export/field/PhabricatorIntExportField.php', 'PhabricatorInternalSetting' => 'applications/settings/setting/PhabricatorInternalSetting.php', 'PhabricatorInternationalizationManagementExtractWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementExtractWorkflow.php', 'PhabricatorInternationalizationManagementWorkflow' => 'infrastructure/internationalization/management/PhabricatorInternationalizationManagementWorkflow.php', @@ -3106,7 +3106,7 @@ phutil_register_library_map(array( 'PhabricatorIteratorFileUploadSource' => 'applications/files/uploadsource/PhabricatorIteratorFileUploadSource.php', 'PhabricatorJIRAAuthProvider' => 'applications/auth/provider/PhabricatorJIRAAuthProvider.php', 'PhabricatorJSONConfigType' => 'applications/config/type/PhabricatorJSONConfigType.php', - 'PhabricatorJSONExportFormat' => 'infrastructure/export/PhabricatorJSONExportFormat.php', + 'PhabricatorJSONExportFormat' => 'infrastructure/export/format/PhabricatorJSONExportFormat.php', 'PhabricatorJavelinLinter' => 'infrastructure/lint/linter/PhabricatorJavelinLinter.php', 'PhabricatorJiraIssueHasObjectEdgeType' => 'applications/doorkeeper/edge/PhabricatorJiraIssueHasObjectEdgeType.php', 'PhabricatorJumpNavHandler' => 'applications/search/engine/PhabricatorJumpNavHandler.php', @@ -3426,7 +3426,7 @@ phutil_register_library_map(array( 'PhabricatorPHDConfigOptions' => 'applications/config/option/PhabricatorPHDConfigOptions.php', 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', - 'PhabricatorPHIDExportField' => 'infrastructure/export/PhabricatorPHIDExportField.php', + 'PhabricatorPHIDExportField' => 'infrastructure/export/field/PhabricatorPHIDExportField.php', 'PhabricatorPHIDInterface' => 'applications/phid/interface/PhabricatorPHIDInterface.php', 'PhabricatorPHIDListEditField' => 'applications/transactions/editfield/PhabricatorPHIDListEditField.php', 'PhabricatorPHIDListEditType' => 'applications/transactions/edittype/PhabricatorPHIDListEditType.php', @@ -4192,7 +4192,7 @@ phutil_register_library_map(array( 'PhabricatorStorageSchemaSpec' => 'infrastructure/storage/schema/PhabricatorStorageSchemaSpec.php', 'PhabricatorStorageSetupCheck' => 'applications/config/check/PhabricatorStorageSetupCheck.php', 'PhabricatorStringConfigType' => 'applications/config/type/PhabricatorStringConfigType.php', - 'PhabricatorStringExportField' => 'infrastructure/export/PhabricatorStringExportField.php', + 'PhabricatorStringExportField' => 'infrastructure/export/field/PhabricatorStringExportField.php', 'PhabricatorStringListConfigType' => 'applications/config/type/PhabricatorStringListConfigType.php', 'PhabricatorStringListEditField' => 'applications/transactions/editfield/PhabricatorStringListEditField.php', 'PhabricatorStringSetting' => 'applications/settings/setting/PhabricatorStringSetting.php', @@ -4256,7 +4256,7 @@ phutil_register_library_map(array( 'PhabricatorTextAreaEditField' => 'applications/transactions/editfield/PhabricatorTextAreaEditField.php', 'PhabricatorTextConfigType' => 'applications/config/type/PhabricatorTextConfigType.php', 'PhabricatorTextEditField' => 'applications/transactions/editfield/PhabricatorTextEditField.php', - 'PhabricatorTextExportFormat' => 'infrastructure/export/PhabricatorTextExportFormat.php', + 'PhabricatorTextExportFormat' => 'infrastructure/export/format/PhabricatorTextExportFormat.php', 'PhabricatorTextListConfigType' => 'applications/config/type/PhabricatorTextListConfigType.php', 'PhabricatorTime' => 'infrastructure/time/PhabricatorTime.php', 'PhabricatorTimeFormatSetting' => 'applications/settings/setting/PhabricatorTimeFormatSetting.php', diff --git a/src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorCustomFieldExportEngineExtension.php similarity index 100% rename from src/infrastructure/export/PhabricatorCustomFieldExportEngineExtension.php rename to src/infrastructure/export/engine/PhabricatorCustomFieldExportEngineExtension.php diff --git a/src/infrastructure/export/PhabricatorExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorExportEngineExtension.php similarity index 100% rename from src/infrastructure/export/PhabricatorExportEngineExtension.php rename to src/infrastructure/export/engine/PhabricatorExportEngineExtension.php diff --git a/src/infrastructure/export/PhabricatorEpochExportField.php b/src/infrastructure/export/field/PhabricatorEpochExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorEpochExportField.php rename to src/infrastructure/export/field/PhabricatorEpochExportField.php diff --git a/src/infrastructure/export/PhabricatorExportField.php b/src/infrastructure/export/field/PhabricatorExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorExportField.php rename to src/infrastructure/export/field/PhabricatorExportField.php diff --git a/src/infrastructure/export/PhabricatorIDExportField.php b/src/infrastructure/export/field/PhabricatorIDExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorIDExportField.php rename to src/infrastructure/export/field/PhabricatorIDExportField.php diff --git a/src/infrastructure/export/PhabricatorIntExportField.php b/src/infrastructure/export/field/PhabricatorIntExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorIntExportField.php rename to src/infrastructure/export/field/PhabricatorIntExportField.php diff --git a/src/infrastructure/export/PhabricatorPHIDExportField.php b/src/infrastructure/export/field/PhabricatorPHIDExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorPHIDExportField.php rename to src/infrastructure/export/field/PhabricatorPHIDExportField.php diff --git a/src/infrastructure/export/PhabricatorStringExportField.php b/src/infrastructure/export/field/PhabricatorStringExportField.php similarity index 100% rename from src/infrastructure/export/PhabricatorStringExportField.php rename to src/infrastructure/export/field/PhabricatorStringExportField.php diff --git a/src/infrastructure/export/PhabricatorCSVExportFormat.php b/src/infrastructure/export/format/PhabricatorCSVExportFormat.php similarity index 100% rename from src/infrastructure/export/PhabricatorCSVExportFormat.php rename to src/infrastructure/export/format/PhabricatorCSVExportFormat.php diff --git a/src/infrastructure/export/PhabricatorExcelExportFormat.php b/src/infrastructure/export/format/PhabricatorExcelExportFormat.php similarity index 100% rename from src/infrastructure/export/PhabricatorExcelExportFormat.php rename to src/infrastructure/export/format/PhabricatorExcelExportFormat.php diff --git a/src/infrastructure/export/PhabricatorExportFormat.php b/src/infrastructure/export/format/PhabricatorExportFormat.php similarity index 100% rename from src/infrastructure/export/PhabricatorExportFormat.php rename to src/infrastructure/export/format/PhabricatorExportFormat.php diff --git a/src/infrastructure/export/PhabricatorJSONExportFormat.php b/src/infrastructure/export/format/PhabricatorJSONExportFormat.php similarity index 100% rename from src/infrastructure/export/PhabricatorJSONExportFormat.php rename to src/infrastructure/export/format/PhabricatorJSONExportFormat.php diff --git a/src/infrastructure/export/PhabricatorTextExportFormat.php b/src/infrastructure/export/format/PhabricatorTextExportFormat.php similarity index 100% rename from src/infrastructure/export/PhabricatorTextExportFormat.php rename to src/infrastructure/export/format/PhabricatorTextExportFormat.php From 61b8c12970be2280b38a2842313c4ca9a6d76a42 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 07:33:04 -0800 Subject: [PATCH 14/33] Make the data export format selector remember your last setting Summary: Depends on D18956. Ref T13049. Make the "Export Format" selector sticky. This is partly selfish, since it makes testing format changes a bit easier. It also seems like it's probably a good behavior in general: if you export to Excel once, that's probably what you're going to pick next time. Test Plan: Exported to excel. Exported again, got excel as the default option. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18957 --- src/__phutil_library_map__.php | 2 + ...PhabricatorApplicationSearchController.php | 38 +++++++++++++++++++ .../engine/PhabricatorExportFormatSetting.php | 16 ++++++++ 3 files changed, 56 insertions(+) create mode 100644 src/infrastructure/export/engine/PhabricatorExportFormatSetting.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8ada6932ae..4d4aeda7aa 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2852,6 +2852,7 @@ phutil_register_library_map(array( 'PhabricatorExportEngineExtension' => 'infrastructure/export/engine/PhabricatorExportEngineExtension.php', 'PhabricatorExportField' => 'infrastructure/export/field/PhabricatorExportField.php', 'PhabricatorExportFormat' => 'infrastructure/export/format/PhabricatorExportFormat.php', + 'PhabricatorExportFormatSetting' => 'infrastructure/export/engine/PhabricatorExportFormatSetting.php', 'PhabricatorExtendedPolicyInterface' => 'applications/policy/interface/PhabricatorExtendedPolicyInterface.php', 'PhabricatorExtendingPhabricatorConfigOptions' => 'applications/config/option/PhabricatorExtendingPhabricatorConfigOptions.php', 'PhabricatorExtensionsSetupCheck' => 'applications/config/check/PhabricatorExtensionsSetupCheck.php', @@ -8288,6 +8289,7 @@ phutil_register_library_map(array( 'PhabricatorExportEngineExtension' => 'Phobject', 'PhabricatorExportField' => 'Phobject', 'PhabricatorExportFormat' => 'Phobject', + 'PhabricatorExportFormatSetting' => 'PhabricatorInternalSetting', 'PhabricatorExtendingPhabricatorConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorExtensionsSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorExternalAccount' => array( diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 88cfd3c137..a5c58aac7f 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -421,6 +421,13 @@ final class PhabricatorApplicationSearchController $formats = PhabricatorExportFormat::getAllEnabledExportFormats(); $format_options = mpull($formats, 'getExportFormatName'); + // Try to default to the format the user used last time. If you just + // exported to Excel, you probably want to export to Excel again. + $format_key = $this->readExportFormatPreference(); + if (!isset($formats[$format_key])) { + $format_key = head_key($format_options); + } + $errors = array(); $e_format = null; @@ -434,6 +441,8 @@ final class PhabricatorApplicationSearchController } if (!$errors) { + $this->writeExportFormatPreference($format_key); + $query = $engine->buildQueryFromSavedQuery($saved_query); // NOTE: We aren't reading the pager from the request. Exports always @@ -497,6 +506,7 @@ final class PhabricatorApplicationSearchController ->setName('format') ->setLabel(pht('Format')) ->setError($e_format) + ->setValue($format_key) ->setOptions($format_options)); return $this->newDialog() @@ -912,4 +922,32 @@ final class PhabricatorApplicationSearchController return true; } + private function readExportFormatPreference() { + $viewer = $this->getViewer(); + $export_key = PhabricatorPolicyFavoritesSetting::SETTINGKEY; + return $viewer->getUserSetting($export_key); + } + + private function writeExportFormatPreference($value) { + $viewer = $this->getViewer(); + $request = $this->getRequest(); + + if (!$viewer->isLoggedIn()) { + return; + } + + $export_key = PhabricatorPolicyFavoritesSetting::SETTINGKEY; + $preferences = PhabricatorUserPreferences::loadUserPreferences($viewer); + + $editor = id(new PhabricatorUserPreferencesEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $xactions = array(); + $xactions[] = $preferences->newTransaction($export_key, $value); + $editor->applyTransactions($preferences, $xactions); + } + } diff --git a/src/infrastructure/export/engine/PhabricatorExportFormatSetting.php b/src/infrastructure/export/engine/PhabricatorExportFormatSetting.php new file mode 100644 index 0000000000..625a90719b --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorExportFormatSetting.php @@ -0,0 +1,16 @@ + Date: Mon, 29 Jan 2018 07:53:25 -0800 Subject: [PATCH 15/33] When PHPExcel is not installed, detect it and provide install instructions Summary: Depends on D18957. Ref T13049. To do Excel exports, PHPExcel needs to be installed on the system somewhere. This library is enormous (1K files, ~100K SLOC), which is why we don't just include it in `externals/`. This install process is a little weird and we could improve it, but users don't seem to have too much difficulty with it. This shouldn't be worse than the existing workflow in Maniphest, and I tried to make it at least slightly more clear. Test Plan: Uninstalled PHPExcel, got it marked "Unavailable" and got reasonably-helpful-ish guidance on how to get it to work. Reinstalled, exported, got a sheet. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18958 --- ...PhabricatorApplicationSearchController.php | 36 +++++++++++++++++-- .../format/PhabricatorExcelExportFormat.php | 25 ++++++++++++- .../export/format/PhabricatorExportFormat.php | 12 ------- 3 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index a5c58aac7f..f0bf9bb365 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -418,8 +418,24 @@ final class PhabricatorApplicationSearchController $filename = phutil_utf8_strtolower($filename); $filename = PhabricatorFile::normalizeFileName($filename); - $formats = PhabricatorExportFormat::getAllEnabledExportFormats(); - $format_options = mpull($formats, 'getExportFormatName'); + $all_formats = PhabricatorExportFormat::getAllExportFormats(); + + $available_options = array(); + $unavailable_options = array(); + $formats = array(); + $unavailable_formats = array(); + foreach ($all_formats as $key => $format) { + if ($format->isExportFormatEnabled()) { + $available_options[$key] = $format->getExportFormatName(); + $formats[$key] = $format; + } else { + $unavailable_options[$key] = pht( + '%s (Not Available)', + $format->getExportFormatName()); + $unavailable_formats[$key] = $format; + } + } + $format_options = $available_options + $unavailable_options; // Try to default to the format the user used last time. If you just // exported to Excel, you probably want to export to Excel again. @@ -433,6 +449,22 @@ final class PhabricatorApplicationSearchController $e_format = null; if ($request->isFormPost()) { $format_key = $request->getStr('format'); + + if (isset($unavailable_formats[$format_key])) { + $unavailable = $unavailable_formats[$format_key]; + $instructions = $unavailable->getInstallInstructions(); + + $markup = id(new PHUIRemarkupView($viewer, $instructions)) + ->setRemarkupOption( + PHUIRemarkupView::OPTION_PRESERVE_LINEBREAKS, + false); + + return $this->newDialog() + ->setTitle(pht('Export Format Not Available')) + ->appendChild($markup) + ->addCancelButton($cancel_uri, pht('Done')); + } + $format = idx($formats, $format_key); if (!$format) { diff --git a/src/infrastructure/export/format/PhabricatorExcelExportFormat.php b/src/infrastructure/export/format/PhabricatorExcelExportFormat.php index 633d98fa53..2b0c787884 100644 --- a/src/infrastructure/export/format/PhabricatorExcelExportFormat.php +++ b/src/infrastructure/export/format/PhabricatorExcelExportFormat.php @@ -14,7 +14,30 @@ final class PhabricatorExcelExportFormat } public function isExportFormatEnabled() { - return true; + // TODO: PHPExcel has a dependency on the PHP zip extension. We should test + // for that here, since it fatals if we don't have the ZipArchive class. + return @include_once 'PHPExcel.php'; + } + + public function getInstallInstructions() { + return pht(<< https://github.com/PHPOffice/PHPExcel + +Briefly: + + - Clone that repository somewhere on the sever + (like `/path/to/example/PHPExcel`). + - Update your PHP `%s` setting (in `php.ini`) to include the PHPExcel + `Classes` directory (like `/path/to/example/PHPExcel/Classes`). +EOHELP + , + 'include_path'); } public function getFileExtension() { diff --git a/src/infrastructure/export/format/PhabricatorExportFormat.php b/src/infrastructure/export/format/PhabricatorExportFormat.php index 7e174f5197..4566814b9d 100644 --- a/src/infrastructure/export/format/PhabricatorExportFormat.php +++ b/src/infrastructure/export/format/PhabricatorExportFormat.php @@ -50,16 +50,4 @@ abstract class PhabricatorExportFormat ->execute(); } - final public static function getAllEnabledExportFormats() { - $formats = self::getAllExportFormats(); - - foreach ($formats as $key => $format) { - if (!$format->isExportFormatEnabled()) { - unset($formats[$key]); - } - } - - return $formats; - } - } From 2ac4e1991b4d64972366c5a1e7c7e129eeea0f02 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 08:12:25 -0800 Subject: [PATCH 16/33] Support new data export infrastructure in Maniphest Summary: Depends on D18958. Ref T13049. Support the new stuff. There are a couple more fields this needs to strictly improve on the old export, but I'll add them as extensions shortly. Test Plan: Exported tasks to Excel, saw reasonble-looking data in the export. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18959 --- src/__phutil_library_map__.php | 2 + .../PhabricatorManiphestApplication.php | 2 +- .../query/ManiphestTaskSearchEngine.php | 107 ++++++++++++++++++ .../field/PhabricatorURIExportField.php | 4 + 4 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 src/infrastructure/export/field/PhabricatorURIExportField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4d4aeda7aa..8e7b09a959 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4322,6 +4322,7 @@ phutil_register_library_map(array( 'PhabricatorUIExample' => 'applications/uiexample/examples/PhabricatorUIExample.php', 'PhabricatorUIExampleRenderController' => 'applications/uiexample/controller/PhabricatorUIExampleRenderController.php', 'PhabricatorUIExamplesApplication' => 'applications/uiexample/application/PhabricatorUIExamplesApplication.php', + 'PhabricatorURIExportField' => 'infrastructure/export/field/PhabricatorURIExportField.php', 'PhabricatorUSEnglishTranslation' => 'infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php', 'PhabricatorUnifiedDiffsSetting' => 'applications/settings/setting/PhabricatorUnifiedDiffsSetting.php', 'PhabricatorUnitTestContentSource' => 'infrastructure/contentsource/PhabricatorUnitTestContentSource.php', @@ -10023,6 +10024,7 @@ phutil_register_library_map(array( 'PhabricatorUIExample' => 'Phobject', 'PhabricatorUIExampleRenderController' => 'PhabricatorController', 'PhabricatorUIExamplesApplication' => 'PhabricatorApplication', + 'PhabricatorURIExportField' => 'PhabricatorExportField', 'PhabricatorUSEnglishTranslation' => 'PhutilTranslation', 'PhabricatorUnifiedDiffsSetting' => 'PhabricatorSelectSetting', 'PhabricatorUnitTestContentSource' => 'PhabricatorContentSource', diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index 6e4ac0a8f6..4376eb3f32 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -50,7 +50,7 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { return array( '/T(?P[1-9]\d*)' => 'ManiphestTaskDetailController', '/maniphest/' => array( - '(?:query/(?P[^/]+)/)?' => 'ManiphestTaskListController', + $this->getQueryRoutePattern() => 'ManiphestTaskListController', 'report/(?:(?P\w+)/)?' => 'ManiphestReportController', $this->getBulkRoutePattern('bulk/') => 'ManiphestBulkEditController', 'task/' => array( diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index ec1956bd8e..caf6eb30f5 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -432,4 +432,111 @@ final class ManiphestTaskSearchEngine return $view; } + + protected function newExportFields() { + $fields = array( + id(new PhabricatorStringExportField()) + ->setKey('monogram') + ->setLabel(pht('Monogram')), + id(new PhabricatorPHIDExportField()) + ->setKey('authorPHID') + ->setLabel(pht('Author PHID')), + id(new PhabricatorStringExportField()) + ->setKey('author') + ->setLabel(pht('Author')), + id(new PhabricatorPHIDExportField()) + ->setKey('ownerPHID') + ->setLabel(pht('Owner PHID')), + id(new PhabricatorStringExportField()) + ->setKey('owner') + ->setLabel(pht('Owner')), + id(new PhabricatorStringExportField()) + ->setKey('status') + ->setLabel(pht('Status')), + id(new PhabricatorStringExportField()) + ->setKey('statusName') + ->setLabel(pht('Status Name')), + id(new PhabricatorStringExportField()) + ->setKey('priority') + ->setLabel(pht('Priority')), + id(new PhabricatorStringExportField()) + ->setKey('priorityName') + ->setLabel(pht('Priority Name')), + id(new PhabricatorStringExportField()) + ->setKey('subtype') + ->setLabel('string'), + id(new PhabricatorURIExportField()) + ->setKey('uri') + ->setLabel(pht('URI')), + id(new PhabricatorStringExportField()) + ->setKey('title') + ->setLabel(pht('Title')), + id(new PhabricatorStringExportField()) + ->setKey('description') + ->setLabel(pht('Description')), + ); + + if (ManiphestTaskPoints::getIsEnabled()) { + $fields[] = id(new PhabricatorIntExportField()) + ->setKey('points') + ->setLabel('Points'); + } + + return $fields; + } + + protected function newExportData(array $tasks) { + $viewer = $this->requireViewer(); + + $phids = array(); + foreach ($tasks as $task) { + $phids[] = $task->getAuthorPHID(); + $phids[] = $task->getOwnerPHID(); + } + $handles = $viewer->loadHandles($phids); + + $export = array(); + foreach ($tasks as $task) { + + $author_phid = $task->getAuthorPHID(); + if ($author_phid) { + $author_name = $handles[$author_phid]->getName(); + } else { + $author_name = null; + } + + $owner_phid = $task->getOwnerPHID(); + if ($owner_phid) { + $owner_name = $handles[$owner_phid]->getName(); + } else { + $owner_name = null; + } + + $status_value = $task->getStatus(); + $status_name = ManiphestTaskStatus::getTaskStatusName($status_value); + + $priority_value = $task->getPriority(); + $priority_name = ManiphestTaskPriority::getTaskPriorityName( + $priority_value); + + $export[] = array( + 'monogram' => $task->getMonogram(), + 'authorPHID' => $author_phid, + 'author' => $author_name, + 'ownerPHID' => $owner_phid, + 'owner' => $owner_name, + 'status' => $status_value, + 'statusName' => $status_name, + 'priority' => $priority_value, + 'priorityName' => $priority_name, + 'points' => $task->getPoints(), + 'subtype' => $task->getSubtype(), + 'title' => $task->getTitle(), + 'uri' => PhabricatorEnv::getProductionURI($task->getURI()), + 'description' => $task->getDescription(), + ); + } + + return $export; + } } diff --git a/src/infrastructure/export/field/PhabricatorURIExportField.php b/src/infrastructure/export/field/PhabricatorURIExportField.php new file mode 100644 index 0000000000..9ba2cd4b75 --- /dev/null +++ b/src/infrastructure/export/field/PhabricatorURIExportField.php @@ -0,0 +1,4 @@ + Date: Mon, 29 Jan 2018 08:37:38 -0800 Subject: [PATCH 17/33] Implement common infrastructure fields as export extensions Summary: Depends on D18959. Ref T13049. Provide tags, subscribers, spaces, and created/modified as automatic extensions for all objects which support them. (Also, for JSON export, be a little more consistent about exporting `null` instead of empty string when there's no value in a text field.) Test Plan: Exported users and tasks, saw relevant fields in the export. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18960 --- src/__phutil_library_map__.php | 14 +++++ .../query/ManiphestTaskSearchEngine.php | 2 +- .../query/PhabricatorPeopleSearchEngine.php | 4 -- .../PhabricatorLiskExportEngineExtension.php | 42 +++++++++++++ ...abricatorProjectsExportEngineExtension.php | 60 +++++++++++++++++++ ...PhabricatorSpacesExportEngineExtension.php | 53 ++++++++++++++++ ...atorSubscriptionsExportEngineExtension.php | 60 +++++++++++++++++++ .../export/field/PhabricatorExportField.php | 8 ++- .../field/PhabricatorListExportField.php | 10 ++++ .../field/PhabricatorPHIDListExportField.php | 10 ++++ .../field/PhabricatorStringExportField.php | 16 ++++- .../PhabricatorStringListExportField.php | 4 ++ 12 files changed, 276 insertions(+), 7 deletions(-) create mode 100644 src/infrastructure/export/engine/PhabricatorLiskExportEngineExtension.php create mode 100644 src/infrastructure/export/engine/PhabricatorProjectsExportEngineExtension.php create mode 100644 src/infrastructure/export/engine/PhabricatorSpacesExportEngineExtension.php create mode 100644 src/infrastructure/export/engine/PhabricatorSubscriptionsExportEngineExtension.php create mode 100644 src/infrastructure/export/field/PhabricatorListExportField.php create mode 100644 src/infrastructure/export/field/PhabricatorPHIDListExportField.php create mode 100644 src/infrastructure/export/field/PhabricatorStringListExportField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8e7b09a959..132c112879 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3130,9 +3130,11 @@ phutil_register_library_map(array( 'PhabricatorLipsumManagementWorkflow' => 'applications/lipsum/management/PhabricatorLipsumManagementWorkflow.php', 'PhabricatorLipsumMondrianArtist' => 'applications/lipsum/image/PhabricatorLipsumMondrianArtist.php', 'PhabricatorLiskDAO' => 'infrastructure/storage/lisk/PhabricatorLiskDAO.php', + 'PhabricatorLiskExportEngineExtension' => 'infrastructure/export/engine/PhabricatorLiskExportEngineExtension.php', 'PhabricatorLiskFulltextEngineExtension' => 'applications/search/engineextension/PhabricatorLiskFulltextEngineExtension.php', 'PhabricatorLiskSearchEngineExtension' => 'applications/search/engineextension/PhabricatorLiskSearchEngineExtension.php', 'PhabricatorLiskSerializer' => 'infrastructure/storage/lisk/PhabricatorLiskSerializer.php', + 'PhabricatorListExportField' => 'infrastructure/export/field/PhabricatorListExportField.php', 'PhabricatorLocalDiskFileStorageEngine' => 'applications/files/engine/PhabricatorLocalDiskFileStorageEngine.php', 'PhabricatorLocalTimeTestCase' => 'view/__tests__/PhabricatorLocalTimeTestCase.php', 'PhabricatorLocaleScopeGuard' => 'infrastructure/internationalization/scope/PhabricatorLocaleScopeGuard.php', @@ -3431,6 +3433,7 @@ phutil_register_library_map(array( 'PhabricatorPHIDInterface' => 'applications/phid/interface/PhabricatorPHIDInterface.php', 'PhabricatorPHIDListEditField' => 'applications/transactions/editfield/PhabricatorPHIDListEditField.php', 'PhabricatorPHIDListEditType' => 'applications/transactions/edittype/PhabricatorPHIDListEditType.php', + 'PhabricatorPHIDListExportField' => 'infrastructure/export/field/PhabricatorPHIDListExportField.php', 'PhabricatorPHIDResolver' => 'applications/phid/resolver/PhabricatorPHIDResolver.php', 'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php', 'PhabricatorPHIDTypeTestCase' => 'applications/phid/type/__tests__/PhabricatorPHIDTypeTestCase.php', @@ -3839,6 +3842,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsCurtainExtension' => 'applications/project/engineextension/PhabricatorProjectsCurtainExtension.php', 'PhabricatorProjectsEditEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php', 'PhabricatorProjectsEditField' => 'applications/transactions/editfield/PhabricatorProjectsEditField.php', + 'PhabricatorProjectsExportEngineExtension' => 'infrastructure/export/engine/PhabricatorProjectsExportEngineExtension.php', 'PhabricatorProjectsFulltextEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php', 'PhabricatorProjectsMembersSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsMembersSearchEngineAttachment.php', 'PhabricatorProjectsMembershipIndexEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php', @@ -4130,6 +4134,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesController' => 'applications/spaces/controller/PhabricatorSpacesController.php', 'PhabricatorSpacesDAO' => 'applications/spaces/storage/PhabricatorSpacesDAO.php', 'PhabricatorSpacesEditController' => 'applications/spaces/controller/PhabricatorSpacesEditController.php', + 'PhabricatorSpacesExportEngineExtension' => 'infrastructure/export/engine/PhabricatorSpacesExportEngineExtension.php', 'PhabricatorSpacesInterface' => 'applications/spaces/interface/PhabricatorSpacesInterface.php', 'PhabricatorSpacesListController' => 'applications/spaces/controller/PhabricatorSpacesListController.php', 'PhabricatorSpacesNamespace' => 'applications/spaces/storage/PhabricatorSpacesNamespace.php', @@ -4196,6 +4201,7 @@ phutil_register_library_map(array( 'PhabricatorStringExportField' => 'infrastructure/export/field/PhabricatorStringExportField.php', 'PhabricatorStringListConfigType' => 'applications/config/type/PhabricatorStringListConfigType.php', 'PhabricatorStringListEditField' => 'applications/transactions/editfield/PhabricatorStringListEditField.php', + 'PhabricatorStringListExportField' => 'infrastructure/export/field/PhabricatorStringListExportField.php', 'PhabricatorStringSetting' => 'applications/settings/setting/PhabricatorStringSetting.php', 'PhabricatorSubmitEditField' => 'applications/transactions/editfield/PhabricatorSubmitEditField.php', 'PhabricatorSubscribableInterface' => 'applications/subscriptions/interface/PhabricatorSubscribableInterface.php', @@ -4210,6 +4216,7 @@ phutil_register_library_map(array( 'PhabricatorSubscriptionsEditController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php', 'PhabricatorSubscriptionsEditEngineExtension' => 'applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php', 'PhabricatorSubscriptionsEditor' => 'applications/subscriptions/editor/PhabricatorSubscriptionsEditor.php', + 'PhabricatorSubscriptionsExportEngineExtension' => 'infrastructure/export/engine/PhabricatorSubscriptionsExportEngineExtension.php', 'PhabricatorSubscriptionsFulltextEngineExtension' => 'applications/subscriptions/engineextension/PhabricatorSubscriptionsFulltextEngineExtension.php', 'PhabricatorSubscriptionsHeraldAction' => 'applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php', 'PhabricatorSubscriptionsListController' => 'applications/subscriptions/controller/PhabricatorSubscriptionsListController.php', @@ -8608,9 +8615,11 @@ phutil_register_library_map(array( 'PhabricatorLipsumManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorLipsumMondrianArtist' => 'PhabricatorLipsumArtist', 'PhabricatorLiskDAO' => 'LiskDAO', + 'PhabricatorLiskExportEngineExtension' => 'PhabricatorExportEngineExtension', 'PhabricatorLiskFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', 'PhabricatorLiskSearchEngineExtension' => 'PhabricatorSearchEngineExtension', 'PhabricatorLiskSerializer' => 'Phobject', + 'PhabricatorListExportField' => 'PhabricatorExportField', 'PhabricatorLocalDiskFileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorLocalTimeTestCase' => 'PhabricatorTestCase', 'PhabricatorLocaleScopeGuard' => 'Phobject', @@ -8948,6 +8957,7 @@ phutil_register_library_map(array( 'PhabricatorPHIDExportField' => 'PhabricatorExportField', 'PhabricatorPHIDListEditField' => 'PhabricatorEditField', 'PhabricatorPHIDListEditType' => 'PhabricatorEditType', + 'PhabricatorPHIDListExportField' => 'PhabricatorListExportField', 'PhabricatorPHIDResolver' => 'Phobject', 'PhabricatorPHIDType' => 'Phobject', 'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase', @@ -9448,6 +9458,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsCurtainExtension' => 'PHUICurtainExtension', 'PhabricatorProjectsEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorProjectsEditField' => 'PhabricatorTokenizerEditField', + 'PhabricatorProjectsExportEngineExtension' => 'PhabricatorExportEngineExtension', 'PhabricatorProjectsFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', 'PhabricatorProjectsMembersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorProjectsMembershipIndexEngineExtension' => 'PhabricatorIndexEngineExtension', @@ -9815,6 +9826,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesController' => 'PhabricatorController', 'PhabricatorSpacesDAO' => 'PhabricatorLiskDAO', 'PhabricatorSpacesEditController' => 'PhabricatorSpacesController', + 'PhabricatorSpacesExportEngineExtension' => 'PhabricatorExportEngineExtension', 'PhabricatorSpacesInterface' => 'PhabricatorPHIDInterface', 'PhabricatorSpacesListController' => 'PhabricatorSpacesController', 'PhabricatorSpacesNamespace' => array( @@ -9888,6 +9900,7 @@ phutil_register_library_map(array( 'PhabricatorStringExportField' => 'PhabricatorExportField', 'PhabricatorStringListConfigType' => 'PhabricatorTextListConfigType', 'PhabricatorStringListEditField' => 'PhabricatorEditField', + 'PhabricatorStringListExportField' => 'PhabricatorListExportField', 'PhabricatorStringSetting' => 'PhabricatorSetting', 'PhabricatorSubmitEditField' => 'PhabricatorEditField', 'PhabricatorSubscribedToObjectEdgeType' => 'PhabricatorEdgeType', @@ -9901,6 +9914,7 @@ phutil_register_library_map(array( 'PhabricatorSubscriptionsEditController' => 'PhabricatorController', 'PhabricatorSubscriptionsEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorSubscriptionsEditor' => 'PhabricatorEditor', + 'PhabricatorSubscriptionsExportEngineExtension' => 'PhabricatorExportEngineExtension', 'PhabricatorSubscriptionsFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', 'PhabricatorSubscriptionsHeraldAction' => 'HeraldAction', 'PhabricatorSubscriptionsListController' => 'PhabricatorController', diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index caf6eb30f5..ad668db376 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -464,7 +464,7 @@ final class ManiphestTaskSearchEngine ->setLabel(pht('Priority Name')), id(new PhabricatorStringExportField()) ->setKey('subtype') - ->setLabel('string'), + ->setLabel('Subtype'), id(new PhabricatorURIExportField()) ->setKey('uri') ->setLabel(pht('URI')), diff --git a/src/applications/people/query/PhabricatorPeopleSearchEngine.php b/src/applications/people/query/PhabricatorPeopleSearchEngine.php index e0e1b5070e..57ed133df4 100644 --- a/src/applications/people/query/PhabricatorPeopleSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleSearchEngine.php @@ -328,9 +328,6 @@ final class PhabricatorPeopleSearchEngine id(new PhabricatorStringExportField()) ->setKey('realName') ->setLabel(pht('Real Name')), - id(new PhabricatorEpochExportField()) - ->setKey('created') - ->setLabel(pht('Date Created')), ); } @@ -342,7 +339,6 @@ final class PhabricatorPeopleSearchEngine $export[] = array( 'username' => $user->getUsername(), 'realName' => $user->getRealName(), - 'created' => $user->getDateCreated(), ); } diff --git a/src/infrastructure/export/engine/PhabricatorLiskExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorLiskExportEngineExtension.php new file mode 100644 index 0000000000..5162986057 --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorLiskExportEngineExtension.php @@ -0,0 +1,42 @@ +getConfigOption(LiskDAO::CONFIG_TIMESTAMPS)) { + return false; + } + + return true; + } + + public function newExportFields() { + return array( + id(new PhabricatorEpochExportField()) + ->setKey('dateCreated') + ->setLabel(pht('Created')), + id(new PhabricatorEpochExportField()) + ->setKey('dateModified') + ->setLabel(pht('Modified')), + ); + } + + public function newExportData(array $objects) { + $map = array(); + foreach ($objects as $object) { + $map[] = array( + 'dateCreated' => $object->getDateCreated(), + 'dateModified' => $object->getDateModified(), + ); + } + return $map; + } + +} diff --git a/src/infrastructure/export/engine/PhabricatorProjectsExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorProjectsExportEngineExtension.php new file mode 100644 index 0000000000..eb3bca3a49 --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorProjectsExportEngineExtension.php @@ -0,0 +1,60 @@ +setKey('tagPHIDs') + ->setLabel(pht('Tag PHIDs')), + id(new PhabricatorStringListExportField()) + ->setKey('tags') + ->setLabel(pht('Tags')), + ); + } + + public function newExportData(array $objects) { + $viewer = $this->getViewer(); + + $object_phids = mpull($objects, 'getPHID'); + + $projects_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($object_phids) + ->withEdgeTypes( + array( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + )); + $projects_query->execute(); + + $handles = $viewer->loadHandles($projects_query->getDestinationPHIDs()); + + $map = array(); + foreach ($objects as $object) { + $object_phid = $object->getPHID(); + + $project_phids = $projects_query->getDestinationPHIDs( + array($object_phid), + array(PhabricatorProjectObjectHasProjectEdgeType::EDGECONST)); + + $handle_list = $handles->newSublist($project_phids); + $handle_list = iterator_to_array($handle_list); + $handle_names = mpull($handle_list, 'getName'); + $handle_names = array_values($handle_names); + + $map[] = array( + 'tagPHIDs' => $project_phids, + 'tags' => $handle_names, + ); + } + + return $map; + } + +} diff --git a/src/infrastructure/export/engine/PhabricatorSpacesExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorSpacesExportEngineExtension.php new file mode 100644 index 0000000000..3e187bc8cd --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorSpacesExportEngineExtension.php @@ -0,0 +1,53 @@ +getViewer(); + + if (!PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) { + return false; + } + + return ($object instanceof PhabricatorSpacesInterface); + } + + public function newExportFields() { + return array( + id(new PhabricatorPHIDExportField()) + ->setKey('spacePHID') + ->setLabel(pht('Space PHID')), + id(new PhabricatorStringExportField()) + ->setKey('space') + ->setLabel(pht('Space')), + ); + } + + public function newExportData(array $objects) { + $viewer = $this->getViewer(); + + $space_phids = array(); + foreach ($objects as $object) { + $space_phids[] = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + } + $handles = $viewer->loadHandles($space_phids); + + $map = array(); + foreach ($objects as $object) { + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + + $map[] = array( + 'spacePHID' => $space_phid, + 'space' => $handles[$space_phid]->getName(), + ); + } + + return $map; + } + +} diff --git a/src/infrastructure/export/engine/PhabricatorSubscriptionsExportEngineExtension.php b/src/infrastructure/export/engine/PhabricatorSubscriptionsExportEngineExtension.php new file mode 100644 index 0000000000..8aedb38fa8 --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorSubscriptionsExportEngineExtension.php @@ -0,0 +1,60 @@ +setKey('subscriberPHIDs') + ->setLabel(pht('Subscriber PHIDs')), + id(new PhabricatorStringListExportField()) + ->setKey('subscribers') + ->setLabel(pht('Subscribers')), + ); + } + + public function newExportData(array $objects) { + $viewer = $this->getViewer(); + + $object_phids = mpull($objects, 'getPHID'); + + $projects_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($object_phids) + ->withEdgeTypes( + array( + PhabricatorObjectHasSubscriberEdgeType::EDGECONST, + )); + $projects_query->execute(); + + $handles = $viewer->loadHandles($projects_query->getDestinationPHIDs()); + + $map = array(); + foreach ($objects as $object) { + $object_phid = $object->getPHID(); + + $project_phids = $projects_query->getDestinationPHIDs( + array($object_phid), + array(PhabricatorObjectHasSubscriberEdgeType::EDGECONST)); + + $handle_list = $handles->newSublist($project_phids); + $handle_list = iterator_to_array($handle_list); + $handle_names = mpull($handle_list, 'getName'); + $handle_names = array_values($handle_names); + + $map[] = array( + 'subscriberPHIDs' => $project_phids, + 'subscribers' => $handle_names, + ); + } + + return $map; + } + +} diff --git a/src/infrastructure/export/field/PhabricatorExportField.php b/src/infrastructure/export/field/PhabricatorExportField.php index 85e21b3e37..7ee0918595 100644 --- a/src/infrastructure/export/field/PhabricatorExportField.php +++ b/src/infrastructure/export/field/PhabricatorExportField.php @@ -25,7 +25,13 @@ abstract class PhabricatorExportField } public function getTextValue($value) { - return (string)$this->getNaturalValue($value); + $natural_value = $this->getNaturalValue($value); + + if ($natural_value === null) { + return null; + } + + return (string)$natural_value; } public function getNaturalValue($value) { diff --git a/src/infrastructure/export/field/PhabricatorListExportField.php b/src/infrastructure/export/field/PhabricatorListExportField.php new file mode 100644 index 0000000000..67c3e06ff5 --- /dev/null +++ b/src/infrastructure/export/field/PhabricatorListExportField.php @@ -0,0 +1,10 @@ + Date: Mon, 29 Jan 2018 08:56:49 -0800 Subject: [PATCH 18/33] Remove the old, non-modular Excel export workflow from Maniphest Summary: Depends on D18960. Ref T13049. Now that Maniphest fully supports "Export Data", remove the old hard-coded version. This is a backward compatibility break with the handful of installs that might have defined a custom export by subclassing `ManiphestExcelFormat`. I suspect this is almost zero installs, and that the additional data in the new format may serve most of the needs of this tiny number of installs. They can upgrade to `ExportEngineExtensions` fairly easily if this isn't true. Test Plan: - Viewed Maniphest, no longer saw the old export workflow. - Grepped for `export` and similar strings to try to hunt everything down. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18961 --- src/__phutil_library_map__.php | 8 - .../PhabricatorManiphestApplication.php | 1 - .../controller/ManiphestExportController.php | 135 ----------------- .../export/ManiphestExcelDefaultFormat.php | 140 ------------------ .../maniphest/export/ManiphestExcelFormat.php | 35 ----- .../ManiphestExcelFormatTestCase.php | 10 -- .../view/ManiphestTaskResultListView.php | 13 +- ...PhabricatorApplicationSearchController.php | 2 +- 8 files changed, 2 insertions(+), 342 deletions(-) delete mode 100644 src/applications/maniphest/controller/ManiphestExportController.php delete mode 100644 src/applications/maniphest/export/ManiphestExcelDefaultFormat.php delete mode 100644 src/applications/maniphest/export/ManiphestExcelFormat.php delete mode 100644 src/applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 132c112879..1f71d97460 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1525,10 +1525,6 @@ phutil_register_library_map(array( 'ManiphestEditProjectsCapability' => 'applications/maniphest/capability/ManiphestEditProjectsCapability.php', 'ManiphestEditStatusCapability' => 'applications/maniphest/capability/ManiphestEditStatusCapability.php', 'ManiphestEmailCommand' => 'applications/maniphest/command/ManiphestEmailCommand.php', - 'ManiphestExcelDefaultFormat' => 'applications/maniphest/export/ManiphestExcelDefaultFormat.php', - 'ManiphestExcelFormat' => 'applications/maniphest/export/ManiphestExcelFormat.php', - 'ManiphestExcelFormatTestCase' => 'applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php', - 'ManiphestExportController' => 'applications/maniphest/controller/ManiphestExportController.php', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php', 'ManiphestHovercardEngineExtension' => 'applications/maniphest/engineextension/ManiphestHovercardEngineExtension.php', 'ManiphestInfoConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestInfoConduitAPIMethod.php', @@ -6780,10 +6776,6 @@ phutil_register_library_map(array( 'ManiphestEditProjectsCapability' => 'PhabricatorPolicyCapability', 'ManiphestEditStatusCapability' => 'PhabricatorPolicyCapability', 'ManiphestEmailCommand' => 'MetaMTAEmailTransactionCommand', - 'ManiphestExcelDefaultFormat' => 'ManiphestExcelFormat', - 'ManiphestExcelFormat' => 'Phobject', - 'ManiphestExcelFormatTestCase' => 'PhabricatorTestCase', - 'ManiphestExportController' => 'ManiphestController', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'ManiphestInfoConduitAPIMethod' => 'ManiphestConduitAPIMethod', diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index 4376eb3f32..0075770863 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -57,7 +57,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { $this->getEditRoutePattern('edit/') => 'ManiphestTaskEditController', ), - 'export/(?P[^/]+)/' => 'ManiphestExportController', 'subpriority/' => 'ManiphestSubpriorityController', ), ); diff --git a/src/applications/maniphest/controller/ManiphestExportController.php b/src/applications/maniphest/controller/ManiphestExportController.php deleted file mode 100644 index 1201c50478..0000000000 --- a/src/applications/maniphest/controller/ManiphestExportController.php +++ /dev/null @@ -1,135 +0,0 @@ -getViewer(); - $key = $request->getURIData('key'); - - $ok = @include_once 'PHPExcel.php'; - if (!$ok) { - $dialog = $this->newDialog(); - - $inst1 = pht( - 'This system does not have PHPExcel installed. This software '. - 'component is required to export tasks to Excel. Have your system '. - 'administrator install it from:'); - - $inst2 = pht( - 'Your PHP "%s" needs to be updated to include the '. - 'PHPExcel Classes directory.', - 'include_path'); - - $dialog->setTitle(pht('Excel Export Not Configured')); - $dialog->appendChild(hsprintf( - '

%s

'. - '
'. - '

'. - ''. - 'https://github.com/PHPOffice/PHPExcel'. - ''. - '

'. - '
'. - '

%s

', - $inst1, - $inst2)); - - $dialog->addCancelButton('/maniphest/'); - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - // TODO: PHPExcel has a dependency on the PHP zip extension. We should test - // for that here, since it fatals if we don't have the ZipArchive class. - - $saved = id(new PhabricatorSavedQueryQuery()) - ->setViewer($viewer) - ->withQueryKeys(array($key)) - ->executeOne(); - if (!$saved) { - $engine = id(new ManiphestTaskSearchEngine()) - ->setViewer($viewer); - if ($engine->isBuiltinQuery($key)) { - $saved = $engine->buildSavedQueryFromBuiltin($key); - } - if (!$saved) { - return new Aphront404Response(); - } - } - - $formats = ManiphestExcelFormat::loadAllFormats(); - $export_formats = array(); - foreach ($formats as $format_class => $format_object) { - $export_formats[$format_class] = $format_object->getName(); - } - - if (!$request->isDialogFormPost()) { - $dialog = new AphrontDialogView(); - $dialog->setUser($viewer); - - $dialog->setTitle(pht('Export Tasks to Excel')); - $dialog->appendChild( - phutil_tag( - 'p', - array(), - pht('Do you want to export the query results to Excel?'))); - - $form = id(new PHUIFormLayoutView()) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel(pht('Format:')) - ->setName('excel-format') - ->setOptions($export_formats)); - - $dialog->appendChild($form); - - $dialog->addCancelButton('/maniphest/'); - $dialog->addSubmitButton(pht('Export to Excel')); - return id(new AphrontDialogResponse())->setDialog($dialog); - } - - $format = idx($formats, $request->getStr('excel-format')); - if ($format === null) { - throw new Exception(pht('Excel format object not found.')); - } - - $saved->makeEphemeral(); - $saved->setParameter('limit', PHP_INT_MAX); - - $engine = id(new ManiphestTaskSearchEngine()) - ->setViewer($viewer); - - $query = $engine->buildQueryFromSavedQuery($saved); - $query->setViewer($viewer); - $tasks = $query->execute(); - - $all_projects = array_mergev(mpull($tasks, 'getProjectPHIDs')); - $all_assigned = mpull($tasks, 'getOwnerPHID'); - - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($viewer) - ->withPHIDs(array_merge($all_projects, $all_assigned)) - ->execute(); - - $workbook = new PHPExcel(); - $format->buildWorkbook($workbook, $tasks, $handles, $viewer); - $writer = PHPExcel_IOFactory::createWriter($workbook, 'Excel2007'); - - ob_start(); - $writer->save('php://output'); - $data = ob_get_clean(); - - $mime = 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'; - - return id(new AphrontFileResponse()) - ->setMimeType($mime) - ->setDownload($format->getFileName().'.xlsx') - ->setContent($data); - } - -} diff --git a/src/applications/maniphest/export/ManiphestExcelDefaultFormat.php b/src/applications/maniphest/export/ManiphestExcelDefaultFormat.php deleted file mode 100644 index 1451ae504e..0000000000 --- a/src/applications/maniphest/export/ManiphestExcelDefaultFormat.php +++ /dev/null @@ -1,140 +0,0 @@ -setActiveSheetIndex(0); - $sheet->setTitle(pht('Tasks')); - - $widths = array( - null, - 15, - null, - 10, - 15, - 15, - 60, - 30, - 20, - 100, - ); - - foreach ($widths as $col => $width) { - if ($width !== null) { - $sheet->getColumnDimension($this->col($col))->setWidth($width); - } - } - - $status_map = ManiphestTaskStatus::getTaskStatusMap(); - $pri_map = ManiphestTaskPriority::getTaskPriorityMap(); - - $date_format = null; - - $rows = array(); - $rows[] = array( - pht('ID'), - pht('Owner'), - pht('Status'), - pht('Priority'), - pht('Date Created'), - pht('Date Updated'), - pht('Title'), - pht('Tags'), - pht('URI'), - pht('Description'), - ); - - $is_date = array( - false, - false, - false, - false, - true, - true, - false, - false, - false, - false, - ); - - $header_format = array( - 'font' => array( - 'bold' => true, - ), - ); - - foreach ($tasks as $task) { - $task_owner = null; - if ($task->getOwnerPHID()) { - $task_owner = $handles[$task->getOwnerPHID()]->getName(); - } - - $projects = array(); - foreach ($task->getProjectPHIDs() as $phid) { - $projects[] = $handles[$phid]->getName(); - } - $projects = implode(', ', $projects); - - $rows[] = array( - 'T'.$task->getID(), - $task_owner, - idx($status_map, $task->getStatus(), '?'), - idx($pri_map, $task->getPriority(), '?'), - $this->computeExcelDate($task->getDateCreated()), - $this->computeExcelDate($task->getDateModified()), - $task->getTitle(), - $projects, - PhabricatorEnv::getProductionURI('/T'.$task->getID()), - id(new PhutilUTF8StringTruncator()) - ->setMaximumBytes(512) - ->truncateString($task->getDescription()), - ); - } - - foreach ($rows as $row => $cols) { - foreach ($cols as $col => $spec) { - $cell_name = $this->col($col).($row + 1); - $cell = $sheet - ->setCellValue($cell_name, $spec, $return_cell = true); - - if ($row == 0) { - $sheet->getStyle($cell_name)->applyFromArray($header_format); - } - - if ($is_date[$col]) { - $code = PHPExcel_Style_NumberFormat::FORMAT_DATE_YYYYMMDD2; - $sheet - ->getStyle($cell_name) - ->getNumberFormat() - ->setFormatCode($code); - } else { - $cell->setDataType(PHPExcel_Cell_DataType::TYPE_STRING); - } - } - } - } - - private function col($n) { - return chr(ord('A') + $n); - } - -} diff --git a/src/applications/maniphest/export/ManiphestExcelFormat.php b/src/applications/maniphest/export/ManiphestExcelFormat.php deleted file mode 100644 index e455a63656..0000000000 --- a/src/applications/maniphest/export/ManiphestExcelFormat.php +++ /dev/null @@ -1,35 +0,0 @@ -setAncestorClass(__CLASS__) - ->setSortMethod('getOrder') - ->execute(); - } - - abstract public function getName(); - abstract public function getFileName(); - - public function getOrder() { - return 0; - } - - protected function computeExcelDate($epoch) { - $seconds_per_day = (60 * 60 * 24); - $offset = ($seconds_per_day * 25569); - - return ($epoch + $offset) / $seconds_per_day; - } - - /** - * @phutil-external-symbol class PHPExcel - */ - abstract public function buildWorkbook( - PHPExcel $workbook, - array $tasks, - array $handles, - PhabricatorUser $user); - -} diff --git a/src/applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php b/src/applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php deleted file mode 100644 index a3c312fcd2..0000000000 --- a/src/applications/maniphest/export/__tests__/ManiphestExcelFormatTestCase.php +++ /dev/null @@ -1,10 +0,0 @@ -assertTrue(true); - } - -} diff --git a/src/applications/maniphest/view/ManiphestTaskResultListView.php b/src/applications/maniphest/view/ManiphestTaskResultListView.php index b4cf9a544c..6aafcbdccb 100644 --- a/src/applications/maniphest/view/ManiphestTaskResultListView.php +++ b/src/applications/maniphest/view/ManiphestTaskResultListView.php @@ -175,8 +175,7 @@ final class ManiphestTaskResultListView extends ManiphestView { } if (!$user->isLoggedIn()) { - // Don't show the batch editor or excel export for logged-out users. - // Technically we //could// let them export, but ehh. + // Don't show the batch editor for logged-out users. return null; } @@ -220,14 +219,6 @@ final class ManiphestTaskResultListView extends ManiphestView { ), pht("Bulk Edit Selected \xC2\xBB")); - $export = javelin_tag( - 'a', - array( - 'href' => '/maniphest/export/'.$saved_query->getQueryKey().'/', - 'class' => 'button button-grey', - ), - pht('Export to Excel')); - $hidden = phutil_tag( 'div', array( @@ -239,14 +230,12 @@ final class ManiphestTaskResultListView extends ManiphestView { ''. ''. ''. - ''. ''. ''. ''. '
%s%s%s%s%s%s
', $select_all, $select_none, - $export, '', $submit, $hidden); diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index f0bf9bb365..ea46886a15 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -154,7 +154,7 @@ final class PhabricatorApplicationSearchController $saved_query = $engine->buildSavedQueryFromRequest($request); // Save the query to generate a query key, so "Save Custom Query..." and - // other features like Maniphest's "Export..." work correctly. + // other features like "Bulk Edit" and "Export Data" work correctly. $engine->saveQuery($saved_query); } From 84df1220858f61f13ee33506edfc8ec61af920c7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 29 Jan 2018 10:29:49 -0800 Subject: [PATCH 19/33] When exporting more than 1,000 records, export in the background Summary: Depends on D18961. Ref T13049. Currently, longer exports don't give the user any feedback, and exports that take longer than 30 seconds are likely to timeout. For small exports (up to 1,000 rows) continue doing the export in the web process. For large exports, queue a bulk job and do them in the workers instead. This sends the user through the bulk operation UI and is similar to bulk edits. It's a little clunky for now, but you get your data at the end, which is far better than hanging for 30 seconds and then fataling. Test Plan: Exported small result sets, got the same workflow as before. Exported very large result sets, went through the bulk flow, got reasonable results out. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18962 --- src/__phutil_library_map__.php | 6 + ...PhabricatorDaemonBulkJobViewController.php | 12 +- ...PhabricatorApplicationSearchController.php | 79 ++++---- .../bulk/PhabricatorWorkerBulkJobType.php | 20 +++ .../bulk/PhabricatorWorkerBulkJobWorker.php | 4 + .../PhabricatorWorkerSingleBulkJobType.php | 27 +++ .../storage/PhabricatorWorkerBulkJob.php | 4 + .../export/engine/PhabricatorExportEngine.php | 168 ++++++++++++++++++ .../PhabricatorExportEngineBulkJobType.php | 118 ++++++++++++ 9 files changed, 380 insertions(+), 58 deletions(-) create mode 100644 src/infrastructure/daemon/workers/bulk/PhabricatorWorkerSingleBulkJobType.php create mode 100644 src/infrastructure/export/engine/PhabricatorExportEngine.php create mode 100644 src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1f71d97460..06f619d74e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2845,6 +2845,8 @@ phutil_register_library_map(array( 'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php', 'PhabricatorExcelExportFormat' => 'infrastructure/export/format/PhabricatorExcelExportFormat.php', 'PhabricatorExecFutureFileUploadSource' => 'applications/files/uploadsource/PhabricatorExecFutureFileUploadSource.php', + 'PhabricatorExportEngine' => 'infrastructure/export/engine/PhabricatorExportEngine.php', + 'PhabricatorExportEngineBulkJobType' => 'infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php', 'PhabricatorExportEngineExtension' => 'infrastructure/export/engine/PhabricatorExportEngineExtension.php', 'PhabricatorExportField' => 'infrastructure/export/field/PhabricatorExportField.php', 'PhabricatorExportFormat' => 'infrastructure/export/format/PhabricatorExportFormat.php', @@ -4419,6 +4421,7 @@ phutil_register_library_map(array( 'PhabricatorWorkerManagementWorkflow' => 'infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php', 'PhabricatorWorkerPermanentFailureException' => 'infrastructure/daemon/workers/exception/PhabricatorWorkerPermanentFailureException.php', 'PhabricatorWorkerSchemaSpec' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerSchemaSpec.php', + 'PhabricatorWorkerSingleBulkJobType' => 'infrastructure/daemon/workers/bulk/PhabricatorWorkerSingleBulkJobType.php', 'PhabricatorWorkerTask' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerTask.php', 'PhabricatorWorkerTaskData' => 'infrastructure/daemon/workers/storage/PhabricatorWorkerTaskData.php', 'PhabricatorWorkerTaskDetailController' => 'applications/daemon/controller/PhabricatorWorkerTaskDetailController.php', @@ -8286,6 +8289,8 @@ phutil_register_library_map(array( 'PhabricatorExampleEventListener' => 'PhabricatorEventListener', 'PhabricatorExcelExportFormat' => 'PhabricatorExportFormat', 'PhabricatorExecFutureFileUploadSource' => 'PhabricatorFileUploadSource', + 'PhabricatorExportEngine' => 'Phobject', + 'PhabricatorExportEngineBulkJobType' => 'PhabricatorWorkerSingleBulkJobType', 'PhabricatorExportEngineExtension' => 'Phobject', 'PhabricatorExportField' => 'Phobject', 'PhabricatorExportFormat' => 'Phobject', @@ -10154,6 +10159,7 @@ phutil_register_library_map(array( 'PhabricatorWorkerManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorWorkerPermanentFailureException' => 'Exception', 'PhabricatorWorkerSchemaSpec' => 'PhabricatorConfigSchemaSpec', + 'PhabricatorWorkerSingleBulkJobType' => 'PhabricatorWorkerBulkJobType', 'PhabricatorWorkerTask' => 'PhabricatorWorkerDAO', 'PhabricatorWorkerTaskData' => 'PhabricatorWorkerDAO', 'PhabricatorWorkerTaskDetailController' => 'PhabricatorDaemonController', diff --git a/src/applications/daemon/controller/PhabricatorDaemonBulkJobViewController.php b/src/applications/daemon/controller/PhabricatorDaemonBulkJobViewController.php index f7aa396e94..00fb297fe0 100644 --- a/src/applications/daemon/controller/PhabricatorDaemonBulkJobViewController.php +++ b/src/applications/daemon/controller/PhabricatorDaemonBulkJobViewController.php @@ -71,18 +71,10 @@ final class PhabricatorDaemonBulkJobViewController $viewer = $this->getViewer(); $curtain = $this->newCurtainView($job); - if ($job->isConfirming()) { - $continue_uri = $job->getMonitorURI(); - } else { - $continue_uri = $job->getDoneURI(); + foreach ($job->getCurtainActions($viewer) as $action) { + $curtain->addAction($action); } - $curtain->addAction( - id(new PhabricatorActionView()) - ->setHref($continue_uri) - ->setIcon('fa-arrow-circle-o-right') - ->setName(pht('Continue'))); - return $curtain; } diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index ea46886a15..6c7d80eafe 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -444,6 +444,17 @@ final class PhabricatorApplicationSearchController $format_key = head_key($format_options); } + // Check if this is a large result set or not. If we're exporting a + // large amount of data, we'll build the actual export file in the daemons. + + $threshold = 1000; + $query = $engine->buildQueryFromSavedQuery($saved_query); + $pager = $engine->newPagerForSavedQuery($saved_query); + $pager->setPageSize($threshold + 1); + $objects = $engine->executeQuery($query, $pager); + $object_count = count($objects); + $is_large_export = ($object_count > $threshold); + $errors = array(); $e_format = null; @@ -475,59 +486,31 @@ final class PhabricatorApplicationSearchController if (!$errors) { $this->writeExportFormatPreference($format_key); - $query = $engine->buildQueryFromSavedQuery($saved_query); - - // NOTE: We aren't reading the pager from the request. Exports always - // affect the entire result set. - $pager = $engine->newPagerForSavedQuery($saved_query); - $pager->setPageSize(0x7FFFFFFF); - - $objects = $engine->executeQuery($query, $pager); - - $extension = $format->getFileExtension(); - $mime_type = $format->getMIMEContentType(); - $filename = $filename.'.'.$extension; - - $format = id(clone $format) + $export_engine = id(new PhabricatorExportEngine()) ->setViewer($viewer) - ->setTitle($sheet_title); + ->setSearchEngine($engine) + ->setSavedQuery($saved_query) + ->setTitle($sheet_title) + ->setFilename($filename) + ->setExportFormat($format); - $export_data = $engine->newExport($objects); - $objects = array_values($objects); + if ($is_large_export) { + $job = $export_engine->newBulkJob($request); - $field_list = $engine->newExportFieldList(); - $field_list = mpull($field_list, null, 'getKey'); + return id(new AphrontRedirectResponse()) + ->setURI($job->getMonitorURI()); + } else { + $file = $export_engine->exportFile(); - $format->addHeaders($field_list); - for ($ii = 0; $ii < count($objects); $ii++) { - $format->addObject($objects[$ii], $field_list, $export_data[$ii]); + return $this->newDialog() + ->setTitle(pht('Download Results')) + ->appendParagraph( + pht('Click the download button to download the exported data.')) + ->addCancelButton($cancel_uri, pht('Done')) + ->setSubmitURI($file->getDownloadURI()) + ->setDisableWorkflowOnSubmit(true) + ->addSubmitButton(pht('Download Data')); } - - $export_result = $format->newFileData(); - - // We have all the data in one big string and aren't actually - // streaming it, but pretending that we are allows us to actviate - // the chunk engine and store large files. - $iterator = new ArrayIterator(array($export_result)); - - $source = id(new PhabricatorIteratorFileUploadSource()) - ->setName($filename) - ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) - ->setMIMEType($mime_type) - ->setRelativeTTL(phutil_units('60 minutes in seconds')) - ->setAuthorPHID($viewer->getPHID()) - ->setIterator($iterator); - - $file = $source->uploadFile(); - - return $this->newDialog() - ->setTitle(pht('Download Results')) - ->appendParagraph( - pht('Click the download button to download the exported data.')) - ->addCancelButton($cancel_uri, pht('Done')) - ->setSubmitURI($file->getDownloadURI()) - ->setDisableWorkflowOnSubmit(true) - ->addSubmitButton(pht('Download Data')); } } diff --git a/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobType.php b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobType.php index a5e29bc101..7854730bea 100644 --- a/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobType.php +++ b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobType.php @@ -25,4 +25,24 @@ abstract class PhabricatorWorkerBulkJobType extends Phobject { ->execute(); } + public function getCurtainActions( + PhabricatorUser $viewer, + PhabricatorWorkerBulkJob $job) { + + if ($job->isConfirming()) { + $continue_uri = $job->getMonitorURI(); + } else { + $continue_uri = $job->getDoneURI(); + } + + $continue = id(new PhabricatorActionView()) + ->setHref($continue_uri) + ->setIcon('fa-arrow-circle-o-right') + ->setName(pht('Continue')); + + return array( + $continue, + ); + } + } diff --git a/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobWorker.php b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobWorker.php index 9117060da6..d11f7e81af 100644 --- a/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobWorker.php +++ b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerBulkJobWorker.php @@ -77,6 +77,10 @@ abstract class PhabricatorWorkerBulkJobWorker pht('Job actor does not have permission to edit job.')); } + // Allow the worker to fill user caches inline; bulk jobs occasionally + // need to access user preferences. + $actor->setAllowInlineCacheGeneration(true); + return $actor; } diff --git a/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerSingleBulkJobType.php b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerSingleBulkJobType.php new file mode 100644 index 0000000000..f80411348c --- /dev/null +++ b/src/infrastructure/daemon/workers/bulk/PhabricatorWorkerSingleBulkJobType.php @@ -0,0 +1,27 @@ +getPHID()); + + return $tasks; + } + +} diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerBulkJob.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerBulkJob.php index 40e43c2ec7..312281617d 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerBulkJob.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerBulkJob.php @@ -180,6 +180,10 @@ final class PhabricatorWorkerBulkJob return $this->getJobImplementation()->getJobName($this); } + public function getCurtainActions(PhabricatorUser $viewer) { + return $this->getJobImplementation()->getCurtainActions($viewer, $this); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/infrastructure/export/engine/PhabricatorExportEngine.php b/src/infrastructure/export/engine/PhabricatorExportEngine.php new file mode 100644 index 0000000000..f2c6a1270b --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorExportEngine.php @@ -0,0 +1,168 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setSearchEngine( + PhabricatorApplicationSearchEngine $search_engine) { + $this->searchEngine = $search_engine; + return $this; + } + + public function getSearchEngine() { + return $this->searchEngine; + } + + public function setSavedQuery(PhabricatorSavedQuery $saved_query) { + $this->savedQuery = $saved_query; + return $this; + } + + public function getSavedQuery() { + return $this->savedQuery; + } + + public function setExportFormat( + PhabricatorExportFormat $export_format) { + $this->exportFormat = $export_format; + return $this; + } + + public function getExportFormat() { + return $this->exportFormat; + } + + public function setFilename($filename) { + $this->filename = $filename; + return $this; + } + + public function getFilename() { + return $this->filename; + } + + public function setTitle($title) { + $this->title = $title; + return $this; + } + + public function getTitle() { + return $this->title; + } + + public function newBulkJob(AphrontRequest $request) { + $viewer = $this->getViewer(); + $engine = $this->getSearchEngine(); + $saved_query = $this->getSavedQuery(); + $format = $this->getExportFormat(); + + $params = array( + 'engineClass' => get_class($engine), + 'queryKey' => $saved_query->getQueryKey(), + 'formatKey' => $format->getExportFormatKey(), + 'title' => $this->getTitle(), + 'filename' => $this->getFilename(), + ); + + $job = PhabricatorWorkerBulkJob::initializeNewJob( + $viewer, + new PhabricatorExportEngineBulkJobType(), + $params); + + // We queue these jobs directly into STATUS_WAITING without requiring + // a confirmation from the user. + + $xactions = array(); + + $xactions[] = id(new PhabricatorWorkerBulkJobTransaction()) + ->setTransactionType(PhabricatorWorkerBulkJobTransaction::TYPE_STATUS) + ->setNewValue(PhabricatorWorkerBulkJob::STATUS_WAITING); + + $editor = id(new PhabricatorWorkerBulkJobEditor()) + ->setActor($viewer) + ->setContentSourceFromRequest($request) + ->setContinueOnMissingFields(true) + ->applyTransactions($job, $xactions); + + return $job; + } + + public function exportFile() { + $viewer = $this->getViewer(); + $engine = $this->getSearchEngine(); + $saved_query = $this->getSavedQuery(); + $format = $this->getExportFormat(); + $title = $this->getTitle(); + $filename = $this->getFilename(); + + $query = $engine->buildQueryFromSavedQuery($saved_query); + + $extension = $format->getFileExtension(); + $mime_type = $format->getMIMEContentType(); + $filename = $filename.'.'.$extension; + + $format = id(clone $format) + ->setViewer($viewer) + ->setTitle($title); + + $field_list = $engine->newExportFieldList(); + $field_list = mpull($field_list, null, 'getKey'); + $format->addHeaders($field_list); + + // Iterate over the query results in large page so we don't have to hold + // too much stuff in memory. + $page_size = 1000; + $page_cursor = null; + do { + $pager = $engine->newPagerForSavedQuery($saved_query); + $pager->setPageSize($page_size); + + if ($page_cursor !== null) { + $pager->setAfterID($page_cursor); + } + + $objects = $engine->executeQuery($query, $pager); + $objects = array_values($objects); + $page_cursor = $pager->getNextPageID(); + + $export_data = $engine->newExport($objects); + for ($ii = 0; $ii < count($objects); $ii++) { + $format->addObject($objects[$ii], $field_list, $export_data[$ii]); + } + } while ($pager->getHasMoreResults()); + + $export_result = $format->newFileData(); + + // We have all the data in one big string and aren't actually + // streaming it, but pretending that we are allows us to actviate + // the chunk engine and store large files. + $iterator = new ArrayIterator(array($export_result)); + + $source = id(new PhabricatorIteratorFileUploadSource()) + ->setName($filename) + ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) + ->setMIMEType($mime_type) + ->setRelativeTTL(phutil_units('60 minutes in seconds')) + ->setAuthorPHID($viewer->getPHID()) + ->setIterator($iterator); + + return $source->uploadFile(); + } + +} diff --git a/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php b/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php new file mode 100644 index 0000000000..712127f479 --- /dev/null +++ b/src/infrastructure/export/engine/PhabricatorExportEngineBulkJobType.php @@ -0,0 +1,118 @@ +getParameter('filePHID'); + if (!$file_phid) { + $actions[] = id(new PhabricatorActionView()) + ->setHref('#') + ->setIcon('fa-download') + ->setDisabled(true) + ->setName(pht('Exporting Data...')); + } else { + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + $actions[] = id(new PhabricatorActionView()) + ->setHref('#') + ->setIcon('fa-download') + ->setDisabled(true) + ->setName(pht('Temporary File Expired')); + } else { + $actions[] = id(new PhabricatorActionView()) + ->setRenderAsForm(true) + ->setHref($file->getDownloadURI()) + ->setIcon('fa-download') + ->setName(pht('Download Data Export')); + } + } + + return $actions; + } + + + public function runTask( + PhabricatorUser $actor, + PhabricatorWorkerBulkJob $job, + PhabricatorWorkerBulkTask $task) { + + $engine_class = $job->getParameter('engineClass'); + if (!is_subclass_of($engine_class, 'PhabricatorApplicationSearchEngine')) { + throw new Exception( + pht( + 'Unknown search engine class "%s".', + $engine_class)); + } + + $engine = newv($engine_class, array()) + ->setViewer($actor); + + $query_key = $job->getParameter('queryKey'); + if ($engine->isBuiltinQuery($query_key)) { + $saved_query = $engine->buildSavedQueryFromBuiltin($query_key); + } else if ($query_key) { + $saved_query = id(new PhabricatorSavedQueryQuery()) + ->setViewer($actor) + ->withQueryKeys(array($query_key)) + ->executeOne(); + } else { + $saved_query = null; + } + + if (!$saved_query) { + throw new Exception( + pht( + 'Failed to load saved query ("%s").', + $query_key)); + } + + $format_key = $job->getParameter('formatKey'); + + $all_formats = PhabricatorExportFormat::getAllExportFormats(); + $format = idx($all_formats, $format_key); + if (!$format) { + throw new Exception( + pht( + 'Unknown export format ("%s").', + $format_key)); + } + + if (!$format->isExportFormatEnabled()) { + throw new Exception( + pht( + 'Export format ("%s") is not enabled.', + $format_key)); + } + + $export_engine = id(new PhabricatorExportEngine()) + ->setViewer($actor) + ->setTitle($job->getParameter('title')) + ->setFilename($job->getParameter('filename')) + ->setSearchEngine($engine) + ->setSavedQuery($saved_query) + ->setExportFormat($format); + + $file = $export_engine->exportFile(); + + $job + ->setParameter('filePHID', $file->getPHID()) + ->save(); + } + +} From b27fd05eef0a20176574c7ff433ab59911738a77 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 05:39:09 -0800 Subject: [PATCH 20/33] Add a `bin/bulk export` CLI tool to make debugging and profiling large exports easier Summary: Ref T13049. When stuff executes asynchronously on the bulk workflow it can be hard to inspect directly, and/or a pain to test because you have to go through a bunch of steps to run it again. Make future work here easier by making export triggerable from the CLI. This makes it easy to repeat, inspect with `--trace`, profile with `--xprofile`, etc. Test Plan: - Ran several invalid commands, got sensible error messages. - Ran some valid commands, got exported data. - Used `--xprofile` to look at the profile for a 300MB dump of 100K tasks which took about 40 seconds to export. Nothing jumped out as sketchy to me -- CustomField wrangling is a little slow but most of the time looked like it was being spent legitimately. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18965 --- src/__phutil_library_map__.php | 2 + ...habricatorBulkManagementExportWorkflow.php | 168 ++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 06f619d74e..32985c76c0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2226,6 +2226,7 @@ phutil_register_library_map(array( 'PhabricatorBulkContentSource' => 'infrastructure/daemon/contentsource/PhabricatorBulkContentSource.php', 'PhabricatorBulkEditGroup' => 'applications/transactions/bulk/PhabricatorBulkEditGroup.php', 'PhabricatorBulkEngine' => 'applications/transactions/bulk/PhabricatorBulkEngine.php', + 'PhabricatorBulkManagementExportWorkflow' => 'applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php', 'PhabricatorBulkManagementMakeSilentWorkflow' => 'applications/transactions/bulk/management/PhabricatorBulkManagementMakeSilentWorkflow.php', 'PhabricatorBulkManagementWorkflow' => 'applications/transactions/bulk/management/PhabricatorBulkManagementWorkflow.php', 'PhabricatorCSVExportFormat' => 'infrastructure/export/format/PhabricatorCSVExportFormat.php', @@ -7579,6 +7580,7 @@ phutil_register_library_map(array( 'PhabricatorBulkContentSource' => 'PhabricatorContentSource', 'PhabricatorBulkEditGroup' => 'Phobject', 'PhabricatorBulkEngine' => 'Phobject', + 'PhabricatorBulkManagementExportWorkflow' => 'PhabricatorBulkManagementWorkflow', 'PhabricatorBulkManagementMakeSilentWorkflow' => 'PhabricatorBulkManagementWorkflow', 'PhabricatorBulkManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorCSVExportFormat' => 'PhabricatorExportFormat', diff --git a/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php new file mode 100644 index 0000000000..cd0ed346fc --- /dev/null +++ b/src/applications/transactions/bulk/management/PhabricatorBulkManagementExportWorkflow.php @@ -0,0 +1,168 @@ +setName('export') + ->setExamples('**export** [options]') + ->setSynopsis( + pht('Export data to a flat file (JSON, CSV, Excel, etc).')) + ->setArguments( + array( + array( + 'name' => 'class', + 'param' => 'class', + 'help' => pht( + 'SearchEngine class to export data from.'), + ), + array( + 'name' => 'format', + 'param' => 'format', + 'help' => pht('Export format.'), + ), + array( + 'name' => 'query', + 'param' => 'key', + 'help' => pht( + 'Export the data selected by this query.'), + ), + array( + 'name' => 'output', + 'param' => 'path', + 'help' => pht( + 'Write output to a file. If omitted, output will be sent to '. + 'stdout.'), + ), + array( + 'name' => 'overwrite', + 'help' => pht( + 'If the output file already exists, overwrite it instead of '. + 'raising an error.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $class = $args->getArg('class'); + + if (!strlen($class)) { + throw new PhutilArgumentUsageException( + pht( + 'Specify a search engine class to export data from with '. + '"--class".')); + } + + if (!is_subclass_of($class, 'PhabricatorApplicationSearchEngine')) { + throw new PhutilArgumentUsageException( + pht( + 'SearchEngine class ("%s") is unknown.', + $class)); + } + + $engine = newv($class, array()) + ->setViewer($viewer); + + if (!$engine->canExport()) { + throw new PhutilArgumentUsageException( + pht( + 'SearchEngine class ("%s") does not support data export.', + $class)); + } + + $query_key = $args->getArg('query'); + if (!strlen($query_key)) { + throw new PhutilArgumentUsageException( + pht( + 'Specify a query to export with "--query".')); + } + + if ($engine->isBuiltinQuery($query_key)) { + $saved_query = $engine->buildSavedQueryFromBuiltin($query_key); + } else if ($query_key) { + $saved_query = id(new PhabricatorSavedQueryQuery()) + ->setViewer($viewer) + ->withQueryKeys(array($query_key)) + ->executeOne(); + } else { + $saved_query = null; + } + + if (!$saved_query) { + throw new PhutilArgumentUsageException( + pht( + 'Failed to load saved query ("%s").', + $query_key)); + } + + $format_key = $args->getArg('format'); + if (!strlen($format_key)) { + throw new PhutilArgumentUsageException( + pht( + 'Specify an export format with "--format".')); + } + + $all_formats = PhabricatorExportFormat::getAllExportFormats(); + $format = idx($all_formats, $format_key); + if (!$format) { + throw new PhutilArgumentUsageException( + pht( + 'Unknown export format ("%s"). Known formats are: %s.', + $format_key, + implode(', ', array_keys($all_formats)))); + } + + if (!$format->isExportFormatEnabled()) { + throw new PhutilArgumentUsageException( + pht( + 'Export format ("%s") is not enabled.', + $format_key)); + } + + $is_overwrite = $args->getArg('overwrite'); + $output_path = $args->getArg('output'); + + if (!strlen($output_path) && $is_overwrite) { + throw new PhutilArgumentUsageException( + pht( + 'Flag "--overwrite" has no effect without "--output".')); + } + + if (!$is_overwrite) { + if (Filesystem::pathExists($output_path)) { + throw new PhutilArgumentUsageException( + pht( + 'Output path already exists. Use "--overwrite" to overwrite '. + 'it.')); + } + } + + $export_engine = id(new PhabricatorExportEngine()) + ->setViewer($viewer) + ->setTitle(pht('Export')) + ->setFilename(pht('export')) + ->setSearchEngine($engine) + ->setSavedQuery($saved_query) + ->setExportFormat($format); + + $file = $export_engine->exportFile(); + + $iterator = $file->getFileDataIterator(); + + if (strlen($output_path)) { + foreach ($iterator as $chunk) { + Filesystem::appendFile($output_path, $chunk); + } + } else { + foreach ($iterator as $chunk) { + echo $chunk; + } + } + + return 0; + } + +} From 91108cf83826c4587adcad73008b309669b49dee Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 06:09:35 -0800 Subject: [PATCH 21/33] Upgrade user account activity logs to modern construction Summary: Depends on D18965. Ref T13049. Move this Query and SearchEngine to be a little more modern, to prepare for Export support. Test Plan: - Used all the query fields, viewed activity logs via People and Settings. - I'm not sure the "Session" query is used/useful and may remove it before I'm done here, but I just left it in place for now. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18966 --- .../query/PhabricatorPeopleLogQuery.php | 41 ++--- .../PhabricatorPeopleLogSearchEngine.php | 157 ++++++------------ .../people/view/PhabricatorUserLogView.php | 15 +- .../PhabricatorActivitySettingsPanel.php | 3 +- 4 files changed, 71 insertions(+), 145 deletions(-) diff --git a/src/applications/people/query/PhabricatorPeopleLogQuery.php b/src/applications/people/query/PhabricatorPeopleLogQuery.php index 9bcdc53f49..e6e2506bee 100644 --- a/src/applications/people/query/PhabricatorPeopleLogQuery.php +++ b/src/applications/people/query/PhabricatorPeopleLogQuery.php @@ -40,70 +40,61 @@ final class PhabricatorPeopleLogQuery return $this; } - protected function loadPage() { - $table = new PhabricatorUserLog(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + public function newResultObject() { + return new PhabricatorUserLog(); } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); if ($this->actorPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'actorPHID IN (%Ls)', $this->actorPHIDs); } if ($this->userPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'userPHID IN (%Ls)', $this->userPHIDs); } if ($this->relatedPHIDs !== null) { $where[] = qsprintf( - $conn_r, - 'actorPHID IN (%Ls) OR userPHID IN (%Ls)', + $conn, + '(actorPHID IN (%Ls) OR userPHID IN (%Ls))', $this->relatedPHIDs, $this->relatedPHIDs); } if ($this->sessionKeys !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'session IN (%Ls)', $this->sessionKeys); } if ($this->actions !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'action IN (%Ls)', $this->actions); } if ($this->remoteAddressPrefix !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'remoteAddr LIKE %>', $this->remoteAddressPrefix); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php index 851ef113d0..aea576693f 100644 --- a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php @@ -15,34 +15,8 @@ final class PhabricatorPeopleLogSearchEngine return 500; } - public function buildSavedQueryFromRequest(AphrontRequest $request) { - $saved = new PhabricatorSavedQuery(); - - $saved->setParameter( - 'userPHIDs', - $this->readUsersFromRequest($request, 'users')); - - $saved->setParameter( - 'actorPHIDs', - $this->readUsersFromRequest($request, 'actors')); - - $saved->setParameter( - 'actions', - $this->readListFromRequest($request, 'actions')); - - $saved->setParameter( - 'ip', - $request->getStr('ip')); - - $saved->setParameter( - 'sessions', - $this->readListFromRequest($request, 'sessions')); - - return $saved; - } - - public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new PhabricatorPeopleLogQuery()); + public function newQuery() { + $query = new PhabricatorPeopleLogQuery(); // NOTE: If the viewer isn't an administrator, always restrict the query to // related records. This echoes the policy logic of these logs. This is @@ -54,82 +28,61 @@ final class PhabricatorPeopleLogSearchEngine $query->withRelatedPHIDs(array($viewer->getPHID())); } - $actor_phids = $saved->getParameter('actorPHIDs', array()); - if ($actor_phids) { - $query->withActorPHIDs($actor_phids); + return $query; + } + + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + + if ($map['userPHIDs']) { + $query->withUserPHIDs($map['userPHIDs']); } - $user_phids = $saved->getParameter('userPHIDs', array()); - if ($user_phids) { - $query->withUserPHIDs($user_phids); + if ($map['actorPHIDs']) { + $query->withActorPHIDs($map['actorPHIDs']); } - $actions = $saved->getParameter('actions', array()); - if ($actions) { - $query->withActions($actions); + if ($map['actions']) { + $query->withActions($map['actions']); } - $remote_prefix = $saved->getParameter('ip'); - if (strlen($remote_prefix)) { - $query->withRemoteAddressprefix($remote_prefix); + if (strlen($map['ip'])) { + $query->withRemoteAddressPrefix($map['ip']); } - $sessions = $saved->getParameter('sessions', array()); - if ($sessions) { - $query->withSessionKeys($sessions); + if ($map['sessions']) { + $query->withSessionKeys($map['sessions']); } return $query; } - public function buildSearchForm( - AphrontFormView $form, - PhabricatorSavedQuery $saved) { - - $actor_phids = $saved->getParameter('actorPHIDs', array()); - $user_phids = $saved->getParameter('userPHIDs', array()); - - $actions = $saved->getParameter('actions', array()); - $remote_prefix = $saved->getParameter('ip'); - $sessions = $saved->getParameter('sessions', array()); - - $actions = array_fuse($actions); - $action_control = id(new AphrontFormCheckboxControl()) - ->setLabel(pht('Actions')); - $action_types = PhabricatorUserLog::getActionTypeMap(); - foreach ($action_types as $type => $label) { - $action_control->addCheckbox( - 'actions[]', - $type, - $label, - isset($actions[$label])); - } - - $form - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setDatasource(new PhabricatorPeopleDatasource()) - ->setName('actors') - ->setLabel(pht('Actors')) - ->setValue($actor_phids)) - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setDatasource(new PhabricatorPeopleDatasource()) - ->setName('users') - ->setLabel(pht('Users')) - ->setValue($user_phids)) - ->appendChild($action_control) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Filter IP')) - ->setName('ip') - ->setValue($remote_prefix)) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Sessions')) - ->setName('sessions') - ->setValue(implode(', ', $sessions))); - + protected function buildCustomSearchFields() { + return array( + id(new PhabricatorUsersSearchField()) + ->setKey('userPHIDs') + ->setAliases(array('users', 'user', 'userPHID')) + ->setLabel(pht('Users')) + ->setDescription(pht('Search for activity affecting specific users.')), + id(new PhabricatorUsersSearchField()) + ->setKey('actorPHIDs') + ->setAliases(array('actors', 'actor', 'actorPHID')) + ->setLabel(pht('Actors')) + ->setDescription(pht('Search for activity by specific users.')), + id(new PhabricatorSearchCheckboxesField()) + ->setKey('actions') + ->setLabel(pht('Actions')) + ->setDescription(pht('Search for particular types of activity.')) + ->setOptions(PhabricatorUserLog::getActionTypeMap()), + id(new PhabricatorSearchTextField()) + ->setKey('ip') + ->setLabel(pht('Filter IP')) + ->setDescription(pht('Search for actions by remote address.')), + id(new PhabricatorSearchStringListField()) + ->setKey('sessions') + ->setLabel(pht('Sessions')) + ->setDescription(pht('Search for activity in particular sessions.')), + ); } protected function getURI($path) { @@ -156,19 +109,6 @@ final class PhabricatorPeopleLogSearchEngine return parent::buildSavedQueryFromBuiltin($query_key); } - protected function getRequiredHandlePHIDsForResultList( - array $logs, - PhabricatorSavedQuery $query) { - - $phids = array(); - foreach ($logs as $log) { - $phids[$log->getActorPHID()] = true; - $phids[$log->getUserPHID()] = true; - } - - return array_keys($phids); - } - protected function renderResultList( array $logs, PhabricatorSavedQuery $query, @@ -179,16 +119,13 @@ final class PhabricatorPeopleLogSearchEngine $table = id(new PhabricatorUserLogView()) ->setUser($viewer) - ->setLogs($logs) - ->setHandles($handles); + ->setLogs($logs); if ($viewer->getIsAdmin()) { $table->setSearchBaseURI($this->getApplicationURI('logs/')); } - $result = new PhabricatorApplicationSearchResultView(); - $result->setTable($table); - - return $result; + return id(new PhabricatorApplicationSearchResultView()) + ->setTable($table); } } diff --git a/src/applications/people/view/PhabricatorUserLogView.php b/src/applications/people/view/PhabricatorUserLogView.php index d648b248f7..309540c611 100644 --- a/src/applications/people/view/PhabricatorUserLogView.php +++ b/src/applications/people/view/PhabricatorUserLogView.php @@ -3,7 +3,6 @@ final class PhabricatorUserLogView extends AphrontView { private $logs; - private $handles; private $searchBaseURI; public function setSearchBaseURI($search_base_uri) { @@ -17,17 +16,17 @@ final class PhabricatorUserLogView extends AphrontView { return $this; } - public function setHandles(array $handles) { - assert_instances_of($handles, 'PhabricatorObjectHandle'); - $this->handles = $handles; - return $this; - } - public function render() { $logs = $this->logs; - $handles = $this->handles; $viewer = $this->getUser(); + $phids = array(); + foreach ($logs as $log) { + $phids[] = $log->getActorPHID(); + $phids[] = $log->getUserPHID(); + } + $handles = $viewer->loadHandles($phids); + $action_map = PhabricatorUserLog::getActionTypeMap(); $base_uri = $this->searchBaseURI; diff --git a/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php b/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php index 50f951d661..eeeca27663 100644 --- a/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php @@ -43,8 +43,7 @@ final class PhabricatorActivitySettingsPanel extends PhabricatorSettingsPanel { $table = id(new PhabricatorUserLogView()) ->setUser($viewer) - ->setLogs($logs) - ->setHandles($handles); + ->setLogs($logs); $panel = $this->newBox(pht('Account Activity Logs'), $table); From a5b8be0316ce617208072a3fa402cf61e90b0402 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 06:27:36 -0800 Subject: [PATCH 22/33] Support export of user activity logs Summary: Depends on D18966. Ref T13049. Adds export support to user activity logs. These don't have PHIDs. We could add them, but just make the "phid" column test if the objects have PHIDs or not for now. Test Plan: - Exported user activity logs, got sensible output (with no PHIDs). - Exported some users to make sure I didn't break PHIDs, got an export with PHIDs. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18967 --- .../PhabricatorPeopleApplication.php | 5 +- .../PhabricatorPeopleLogSearchEngine.php | 98 +++++++++++++++++++ .../PhabricatorApplicationSearchEngine.php | 23 ++++- 3 files changed, 119 insertions(+), 7 deletions(-) diff --git a/src/applications/people/application/PhabricatorPeopleApplication.php b/src/applications/people/application/PhabricatorPeopleApplication.php index 28405ca92c..6322b29b24 100644 --- a/src/applications/people/application/PhabricatorPeopleApplication.php +++ b/src/applications/people/application/PhabricatorPeopleApplication.php @@ -42,8 +42,9 @@ final class PhabricatorPeopleApplication extends PhabricatorApplication { return array( '/people/' => array( $this->getQueryRoutePattern() => 'PhabricatorPeopleListController', - 'logs/(?:query/(?P[^/]+)/)?' - => 'PhabricatorPeopleLogsController', + 'logs/' => array( + $this->getQueryRoutePattern() => 'PhabricatorPeopleLogsController', + ), 'invite/' => array( '(?:query/(?P[^/]+)/)?' => 'PhabricatorPeopleInviteListController', diff --git a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php index aea576693f..1a3ae4dea1 100644 --- a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php @@ -128,4 +128,102 @@ final class PhabricatorPeopleLogSearchEngine return id(new PhabricatorApplicationSearchResultView()) ->setTable($table); } + + protected function newExportFields() { + $viewer = $this->requireViewer(); + + $fields = array( + $fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('actorPHID') + ->setLabel(pht('Actor PHID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('actor') + ->setLabel(pht('Actor')), + $fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('userPHID') + ->setLabel(pht('User PHID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('user') + ->setLabel(pht('User')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('action') + ->setLabel(pht('Action')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('actionName') + ->setLabel(pht('Action Name')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('session') + ->setLabel(pht('Session')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('old') + ->setLabel(pht('Old Value')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('new') + ->setLabel(pht('New Value')), + ); + + if ($viewer->getIsAdmin()) { + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('remoteAddress') + ->setLabel(pht('Remote Address')); + } + + return $fields; + } + + protected function newExportData(array $logs) { + $viewer = $this->requireViewer(); + + + $phids = array(); + foreach ($logs as $log) { + $phids[] = $log->getUserPHID(); + $phids[] = $log->getActorPHID(); + } + $handles = $viewer->loadHandles($phids); + + $action_map = PhabricatorUserLog::getActionTypeMap(); + + $export = array(); + foreach ($logs as $log) { + + $user_phid = $log->getUserPHID(); + if ($user_phid) { + $user_name = $handles[$user_phid]->getName(); + } else { + $user_name = null; + } + + $actor_phid = $log->getActorPHID(); + if ($actor_phid) { + $actor_name = $handles[$actor_phid]->getName(); + } else { + $actor_name = null; + } + + $action = $log->getAction(); + $action_name = idx($action_map, $action, pht('Unknown ("%s")', $action)); + + $map = array( + 'actorPHID' => $actor_phid, + 'actor' => $actor_name, + 'userPHID' => $user_phid, + 'user' => $user_name, + 'action' => $action, + 'actionName' => $action_name, + 'session' => substr($log->getSession(), 0, 6), + 'old' => $log->getOldValue(), + 'new' => $log->getNewValue(), + ); + + if ($viewer->getIsAdmin()) { + $map['remoteAddress'] = $log->getRemoteAddr(); + } + + $export[] = $map; + } + + return $export; + } + } diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 3de7b9c4b9..b808291a52 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -1455,15 +1455,20 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } final public function newExportFieldList() { + $object = $this->newResultObject(); + $builtin_fields = array( id(new PhabricatorIDExportField()) ->setKey('id') ->setLabel(pht('ID')), - id(new PhabricatorPHIDExportField()) - ->setKey('phid') - ->setLabel(pht('PHID')), ); + if ($object->getConfigOption(LiskDAO::CONFIG_AUX_PHID)) { + $builtin_fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('phid') + ->setLabel(pht('PHID')); + } + $fields = mpull($builtin_fields, null, 'getKey'); $export_fields = $this->newExportFields(); @@ -1507,15 +1512,23 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { } final public function newExport(array $objects) { + $object = $this->newResultObject(); + $has_phid = $object->getConfigOption(LiskDAO::CONFIG_AUX_PHID); + $objects = array_values($objects); $n = count($objects); $maps = array(); foreach ($objects as $object) { - $maps[] = array( + $map = array( 'id' => $object->getID(), - 'phid' => $object->getPHID(), ); + + if ($has_phid) { + $map['phid'] = $object->getPHID(); + } + + $maps[] = $map; } $export_data = $this->newExportData($objects); From 5b22412f246e74abf0d95d497d416466be488a8f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 07:47:34 -0800 Subject: [PATCH 23/33] Support data export on push logs Summary: Depends on D18967. Ref T13049. Nothing too fancy going on here. Test Plan: Exported push logs, looked at the export, seemed sensible. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18968 --- .../PhabricatorDiffusionApplication.php | 2 +- .../PhabricatorRepositoryPushLogQuery.php | 53 +++---- ...abricatorRepositoryPushLogSearchEngine.php | 142 ++++++++++++++++++ .../storage/PhabricatorRepositoryPushLog.php | 22 +++ 4 files changed, 187 insertions(+), 32 deletions(-) diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index d932149a75..e619ecb1ad 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -121,7 +121,7 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { $this->getEditRoutePattern('edit/') => 'DiffusionRepositoryEditController', 'pushlog/' => array( - '(?:query/(?P[^/]+)/)?' => 'DiffusionPushLogListController', + $this->getQueryRoutePattern() => 'DiffusionPushLogListController', 'view/(?P\d+)/' => 'DiffusionPushEventViewController', ), 'pulllog/' => array( diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php index 94a0b6922e..2c5f7b0d0d 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php @@ -46,19 +46,12 @@ final class PhabricatorRepositoryPushLogQuery return $this; } + public function newResultObject() { + return new PhabricatorRepositoryPushLog(); + } + protected function loadPage() { - $table = new PhabricatorRepositoryPushLog(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + return $this->loadStandardPage($this->newResultObject()); } protected function willFilterPage(array $logs) { @@ -82,61 +75,59 @@ final class PhabricatorRepositoryPushLogQuery return $logs; } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); - if ($this->ids) { + if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'phid IN (%Ls)', $this->phids); } - if ($this->repositoryPHIDs) { + if ($this->repositoryPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'repositoryPHID IN (%Ls)', $this->repositoryPHIDs); } - if ($this->pusherPHIDs) { + if ($this->pusherPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'pusherPHID in (%Ls)', $this->pusherPHIDs); } - if ($this->pushEventPHIDs) { + if ($this->pushEventPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'pushEventPHID in (%Ls)', $this->pushEventPHIDs); } - if ($this->refTypes) { + if ($this->refTypes !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'refType IN (%Ls)', $this->refTypes); } - if ($this->newRefs) { + if ($this->newRefs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'refNew IN (%Ls)', $this->newRefs); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } public function getQueryApplicationClass() { diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php index d171b80999..b2234ab6c7 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php @@ -82,4 +82,146 @@ final class PhabricatorRepositoryPushLogSearchEngine ->setTable($table); } + protected function newExportFields() { + $viewer = $this->requireViewer(); + + $fields = array( + $fields[] = id(new PhabricatorIDExportField()) + ->setKey('pushID') + ->setLabel(pht('Push ID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('protocol') + ->setLabel(pht('Protocol')), + $fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('repositoryPHID') + ->setLabel(pht('Repository PHID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('repository') + ->setLabel(pht('Repository')), + $fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('pusherPHID') + ->setLabel(pht('Pusher PHID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('pusher') + ->setLabel(pht('Pusher')), + $fields[] = id(new PhabricatorPHIDExportField()) + ->setKey('devicePHID') + ->setLabel(pht('Device PHID')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('device') + ->setLabel(pht('Device')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('type') + ->setLabel(pht('Ref Type')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('name') + ->setLabel(pht('Ref Name')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('old') + ->setLabel(pht('Ref Old')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('new') + ->setLabel(pht('Ref New')), + $fields[] = id(new PhabricatorIntExportField()) + ->setKey('flags') + ->setLabel(pht('Flags')), + $fields[] = id(new PhabricatorStringListExportField()) + ->setKey('flagNames') + ->setLabel(pht('Flag Names')), + $fields[] = id(new PhabricatorIntExportField()) + ->setKey('result') + ->setLabel(pht('Result')), + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('resultName') + ->setLabel(pht('Result Name')), + ); + + if ($viewer->getIsAdmin()) { + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('remoteAddress') + ->setLabel(pht('Remote Address')); + } + + return $fields; + } + + protected function newExportData(array $logs) { + $viewer = $this->requireViewer(); + + $phids = array(); + foreach ($logs as $log) { + $phids[] = $log->getPusherPHID(); + $phids[] = $log->getDevicePHID(); + $phids[] = $log->getPushEvent()->getRepositoryPHID(); + } + $handles = $viewer->loadHandles($phids); + + $flag_map = PhabricatorRepositoryPushLog::getFlagDisplayNames(); + $reject_map = PhabricatorRepositoryPushLog::getRejectCodeDisplayNames(); + + $export = array(); + foreach ($logs as $log) { + $event = $log->getPushEvent(); + + $repository_phid = $event->getRepositoryPHID(); + if ($repository_phid) { + $repository_name = $handles[$repository_phid]->getName(); + } else { + $repository_name = null; + } + + $pusher_phid = $log->getPusherPHID(); + if ($pusher_phid) { + $pusher_name = $handles[$pusher_phid]->getName(); + } else { + $pusher_name = null; + } + + $device_phid = $log->getDevicePHID(); + if ($device_phid) { + $device_name = $handles[$device_phid]->getName(); + } else { + $device_name = null; + } + + $flags = $log->getChangeFlags(); + $flag_names = array(); + foreach ($flag_map as $flag_key => $flag_name) { + if (($flags & $flag_key) === $flag_key) { + $flag_names[] = $flag_name; + } + } + + $result = $event->getRejectCode(); + $result_name = idx($reject_map, $result, pht('Unknown ("%s")', $result)); + + $map = array( + 'pushID' => $event->getID(), + 'protocol' => $event->getRemoteProtocol(), + 'repositoryPHID' => $repository_phid, + 'repository' => $repository_name, + 'pusherPHID' => $pusher_phid, + 'pusher' => $pusher_name, + 'devicePHID' => $device_phid, + 'device' => $device_name, + 'type' => $log->getRefType(), + 'name' => $log->getRefName(), + 'old' => $log->getRefOld(), + 'new' => $log->getRefNew(), + 'flags' => $flags, + 'flagNames' => $flag_names, + 'result' => $result, + 'resultName' => $result_name, + ); + + if ($viewer->getIsAdmin()) { + $map['remoteAddress'] = $event->getRemoteAddress(); + } + + $export[] = $map; + } + + return $export; + } + } diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index 4e099209c6..81a564a91f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php @@ -55,6 +55,28 @@ final class PhabricatorRepositoryPushLog ->setPusherPHID($viewer->getPHID()); } + public static function getFlagDisplayNames() { + return array( + self::CHANGEFLAG_ADD => pht('Create'), + self::CHANGEFLAG_DELETE => pht('Delete'), + self::CHANGEFLAG_APPEND => pht('Append'), + self::CHANGEFLAG_REWRITE => pht('Rewrite'), + self::CHANGEFLAG_DANGEROUS => pht('Dangerous'), + self::CHANGEFLAG_ENORMOUS => pht('Enormous'), + ); + } + + public static function getRejectCodeDisplayNames() { + return array( + self::REJECT_ACCEPT => pht('Accepted'), + self::REJECT_DANGEROUS => pht('Rejected: Dangerous'), + self::REJECT_HERALD => pht('Rejected: Herald'), + self::REJECT_EXTERNAL => pht('Rejected: External Hook'), + self::REJECT_BROKEN => pht('Rejected: Broken'), + self::REJECT_ENORMOUS => pht('Rejected: Enormous'), + ); + } + public static function getHeraldChangeFlagConditionOptions() { return array( self::CHANGEFLAG_ADD => From 0d5379ee1778fd049216c171382b1dbb0173e900 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 08:03:29 -0800 Subject: [PATCH 24/33] Fix an export bug where queries specified in the URI ("?param=value") were ignored when filtering the result set Summary: Depends on D18968. Ref T13049. Currently, if you visit `/query/?param=value`, there is no `queryKey` for the page but we build a query later on. Right now, we incorrectly link to `/query/all/export/` in this case (and export too many results), but we should actually link to `/query//export/` to export only the desired/previewed results. Swap the logic around a little bit so we look at the query we're actually executing, not the original URI, to figure out the query key we should use when building the link. Test Plan: Visited a `/?param=value` page, exported data, got a subset of the full data instead of everything. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18969 --- .../PhabricatorApplicationSearchController.php | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 6c7d80eafe..cc8bcefff1 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -7,6 +7,7 @@ final class PhabricatorApplicationSearchController private $navigation; private $queryKey; private $preface; + private $activeQuery; public function setPreface($preface) { $this->preface = $preface; @@ -45,6 +46,14 @@ final class PhabricatorApplicationSearchController return $this->searchEngine; } + protected function getActiveQuery() { + if (!$this->activeQuery) { + throw new Exception(pht('There is no active query yet.')); + } + + return $this->activeQuery; + } + protected function validateDelegatingController() { $parent = $this->getDelegatingController(); @@ -158,6 +167,8 @@ final class PhabricatorApplicationSearchController $engine->saveQuery($saved_query); } + $this->activeQuery = $saved_query; + $nav->selectFilter( 'query/'.$saved_query->getQueryKey(), 'query/advanced'); @@ -867,10 +878,8 @@ final class PhabricatorApplicationSearchController $engine = $this->getSearchEngine(); $engine_class = get_class($engine); - $query_key = $this->getQueryKey(); - if (!$query_key) { - $query_key = $engine->getDefaultQueryKey(); - } + + $query_key = $this->getActiveQuery()->getQueryKey(); $can_use = $engine->canUseInPanelContext(); $is_installed = PhabricatorApplication::isClassInstalledForViewer( From 75bc86589f10dd520bba41dc52bd3458e22b1f6a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 11:47:55 -0800 Subject: [PATCH 25/33] Add date range filtering for activity, push, and pull logs Summary: Ref T13049. This is just a general nice-to-have so you don't have to export a 300MB file if you want to check the last month of data or whatever. Test Plan: Applied filters to all three logs, got appropriate date-range result sets. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18970 --- .../query/DiffusionPullLogSearchEngine.php | 12 ++++++++++ .../query/PhabricatorPeopleLogQuery.php | 22 +++++++++++++++++++ .../PhabricatorPeopleLogSearchEngine.php | 12 ++++++++++ .../PhabricatorRepositoryPullEventQuery.php | 22 +++++++++++++++++++ .../PhabricatorRepositoryPushLogQuery.php | 22 +++++++++++++++++++ ...abricatorRepositoryPushLogSearchEngine.php | 12 ++++++++++ .../storage/PhabricatorRepositoryPushLog.php | 3 +++ 7 files changed, 105 insertions(+) diff --git a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php index fc7cee52ce..db5d4b9fcb 100644 --- a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php +++ b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php @@ -26,6 +26,12 @@ final class DiffusionPullLogSearchEngine $query->withPullerPHIDs($map['pullerPHIDs']); } + if ($map['createdStart'] || $map['createdEnd']) { + $query->withEpochBetween( + $map['createdStart'], + $map['createdEnd']); + } + return $query; } @@ -44,6 +50,12 @@ final class DiffusionPullLogSearchEngine ->setLabel(pht('Pullers')) ->setDescription( pht('Search for pull logs by specific users.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created After')) + ->setKey('createdStart'), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created Before')) + ->setKey('createdEnd'), ); } diff --git a/src/applications/people/query/PhabricatorPeopleLogQuery.php b/src/applications/people/query/PhabricatorPeopleLogQuery.php index e6e2506bee..fc6a87b335 100644 --- a/src/applications/people/query/PhabricatorPeopleLogQuery.php +++ b/src/applications/people/query/PhabricatorPeopleLogQuery.php @@ -9,6 +9,8 @@ final class PhabricatorPeopleLogQuery private $sessionKeys; private $actions; private $remoteAddressPrefix; + private $dateCreatedMin; + private $dateCreatedMax; public function withActorPHIDs(array $actor_phids) { $this->actorPHIDs = $actor_phids; @@ -40,6 +42,12 @@ final class PhabricatorPeopleLogQuery return $this; } + public function withDateCreatedBetween($min, $max) { + $this->dateCreatedMin = $min; + $this->dateCreatedMax = $max; + return $this; + } + public function newResultObject() { return new PhabricatorUserLog(); } @@ -94,6 +102,20 @@ final class PhabricatorPeopleLogQuery $this->remoteAddressPrefix); } + if ($this->dateCreatedMin !== null) { + $where[] = qsprintf( + $conn, + 'dateCreated >= %d', + $this->dateCreatedMin); + } + + if ($this->dateCreatedMax !== null) { + $where[] = qsprintf( + $conn, + 'dateCreated <= %d', + $this->dateCreatedMax); + } + return $where; } diff --git a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php index 1a3ae4dea1..b052456cd3 100644 --- a/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php +++ b/src/applications/people/query/PhabricatorPeopleLogSearchEngine.php @@ -54,6 +54,12 @@ final class PhabricatorPeopleLogSearchEngine $query->withSessionKeys($map['sessions']); } + if ($map['createdStart'] || $map['createdEnd']) { + $query->withDateCreatedBetween( + $map['createdStart'], + $map['createdEnd']); + } + return $query; } @@ -82,6 +88,12 @@ final class PhabricatorPeopleLogSearchEngine ->setKey('sessions') ->setLabel(pht('Sessions')) ->setDescription(pht('Search for activity in particular sessions.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created After')) + ->setKey('createdStart'), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created Before')) + ->setKey('createdEnd'), ); } diff --git a/src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php b/src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php index af60ee0383..ce14a6f831 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryPullEventQuery.php @@ -7,6 +7,8 @@ final class PhabricatorRepositoryPullEventQuery private $phids; private $repositoryPHIDs; private $pullerPHIDs; + private $epochMin; + private $epochMax; public function withIDs(array $ids) { $this->ids = $ids; @@ -28,6 +30,12 @@ final class PhabricatorRepositoryPullEventQuery return $this; } + public function withEpochBetween($min, $max) { + $this->epochMin = $min; + $this->epochMax = $max; + return $this; + } + public function newResultObject() { return new PhabricatorRepositoryPullEvent(); } @@ -103,6 +111,20 @@ final class PhabricatorRepositoryPullEventQuery $this->pullerPHIDs); } + if ($this->epochMin !== null) { + $where[] = qsprintf( + $conn, + 'epoch >= %d', + $this->epochMin); + } + + if ($this->epochMax !== null) { + $where[] = qsprintf( + $conn, + 'epoch <= %d', + $this->epochMax); + } + return $where; } diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php index 2c5f7b0d0d..cb097fae2f 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php @@ -10,6 +10,8 @@ final class PhabricatorRepositoryPushLogQuery private $refTypes; private $newRefs; private $pushEventPHIDs; + private $epochMin; + private $epochMax; public function withIDs(array $ids) { $this->ids = $ids; @@ -46,6 +48,12 @@ final class PhabricatorRepositoryPushLogQuery return $this; } + public function withEpochBetween($min, $max) { + $this->epochMin = $min; + $this->epochMax = $max; + return $this; + } + public function newResultObject() { return new PhabricatorRepositoryPushLog(); } @@ -127,6 +135,20 @@ final class PhabricatorRepositoryPushLogQuery $this->newRefs); } + if ($this->epochMin !== null) { + $where[] = qsprintf( + $conn, + 'epoch >= %d', + $this->epochMin); + } + + if ($this->epochMax !== null) { + $where[] = qsprintf( + $conn, + 'epoch <= %d', + $this->epochMax); + } + return $where; } diff --git a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php index b2234ab6c7..8ad76987f9 100644 --- a/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php +++ b/src/applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php @@ -26,6 +26,12 @@ final class PhabricatorRepositoryPushLogSearchEngine $query->withPusherPHIDs($map['pusherPHIDs']); } + if ($map['createdStart'] || $map['createdEnd']) { + $query->withEpochBetween( + $map['createdStart'], + $map['createdEnd']); + } + return $query; } @@ -44,6 +50,12 @@ final class PhabricatorRepositoryPushLogSearchEngine ->setLabel(pht('Pushers')) ->setDescription( pht('Search for pull logs by specific users.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created After')) + ->setKey('createdStart'), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created Before')) + ->setKey('createdEnd'), ); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index 81a564a91f..c2d3456da6 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php @@ -124,6 +124,9 @@ final class PhabricatorRepositoryPushLog 'key_pusher' => array( 'columns' => array('pusherPHID'), ), + 'key_epoch' => array( + 'columns' => array('epoch'), + ), ), ) + parent::getConfiguration(); } From 8a2863e3f7f8de79fbdd25eb3dc05605b0be2784 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 12:01:26 -0800 Subject: [PATCH 26/33] Change the "can see remote address?" policy to "is administrator?" everywhere Summary: Depends on D18970. Ref T13049. Currently, the policy for viewing remote addresses is: - In activity logs: administrators. - In push and pull logs: users who can edit the corresponding repository. This sort of makes sense, but is also sort of weird. Particularly, I think it's kind of hard to understand and predict, and hard to guess that this is the behavior we implement. The actual implementation is complex, too. Instead, just use the rule "administrators can see remote addresses" consistently across all applications. This should generally be more strict than the old rule, because administrators could usually have seen everyone's address in the activity logs anyway. It's also simpler and more expected, and I don't really know of any legit use cases for the "repository editor" rule. Test Plan: Viewed pull/push/activity logs as non-admin. Saw remote addresses as an admin, and none as a non-admin. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18971 --- .../query/DiffusionPullLogSearchEngine.php | 20 +++++++++-- .../view/DiffusionPullLogListView.php | 36 +++++++------------ .../view/DiffusionPushLogListView.php | 25 ++++--------- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php index db5d4b9fcb..dfdfceb519 100644 --- a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php +++ b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php @@ -60,7 +60,9 @@ final class DiffusionPullLogSearchEngine } protected function newExportFields() { - return array( + $viewer = $this->requireViewer(); + + $fields = array( id(new PhabricatorPHIDExportField()) ->setKey('repositoryPHID') ->setLabel(pht('Repository PHID')), @@ -86,6 +88,14 @@ final class DiffusionPullLogSearchEngine ->setKey('date') ->setLabel(pht('Date')), ); + + if ($viewer->getIsAdmin()) { + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('remoteAddress') + ->setLabel(pht('Remote Address')); + } + + return $fields; } protected function newExportData(array $events) { @@ -117,7 +127,7 @@ final class DiffusionPullLogSearchEngine $puller_name = null; } - $export[] = array( + $map = array( 'repositoryPHID' => $repository_phid, 'repository' => $repository_name, 'pullerPHID' => $puller_phid, @@ -127,6 +137,12 @@ final class DiffusionPullLogSearchEngine 'code' => $event->getResultCode(), 'date' => $event->getEpoch(), ); + + if ($viewer->getIsAdmin()) { + $map['remoteAddress'] = $event->getRemoteAddress(); + } + + $export[] = $map; } return $export; diff --git a/src/applications/diffusion/view/DiffusionPullLogListView.php b/src/applications/diffusion/view/DiffusionPullLogListView.php index f2e3280eba..8df35e2922 100644 --- a/src/applications/diffusion/view/DiffusionPullLogListView.php +++ b/src/applications/diffusion/view/DiffusionPullLogListView.php @@ -22,24 +22,10 @@ final class DiffusionPullLogListView extends AphrontView { } $handles = $viewer->loadHandles($handle_phids); - // Figure out which repositories are editable. We only let you see remote - // IPs if you have edit capability on a repository. - $editable_repos = array(); - if ($events) { - $editable_repos = id(new PhabricatorRepositoryQuery()) - ->setViewer($viewer) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->withPHIDs(mpull($events, 'getRepositoryPHID')) - ->execute(); - $editable_repos = mpull($editable_repos, null, 'getPHID'); - } + // Only administrators can view remote addresses. + $remotes_visible = $viewer->getIsAdmin(); $rows = array(); - $any_host = false; foreach ($events as $event) { if ($event->getRepositoryPHID()) { $repository = $event->getRepository(); @@ -47,13 +33,10 @@ final class DiffusionPullLogListView extends AphrontView { $repository = null; } - // Reveal this if it's valid and the user can edit the repository. For - // invalid requests you currently have to go fishing in the database. - $remote_address = '-'; - if ($repository) { - if (isset($editable_repos[$event->getRepositoryPHID()])) { - $remote_address = $event->getRemoteAddress(); - } + if ($remotes_visible) { + $remote_address = $event->getRemoteAddress(); + } else { + $remote_address = null; } $event_id = $event->getID(); @@ -107,6 +90,13 @@ final class DiffusionPullLogListView extends AphrontView { '', 'n', 'right', + )) + ->setColumnVisibility( + array( + true, + true, + true, + $remotes_visible, )); return $table; diff --git a/src/applications/diffusion/view/DiffusionPushLogListView.php b/src/applications/diffusion/view/DiffusionPushLogListView.php index 303f0519f6..77f28671c6 100644 --- a/src/applications/diffusion/view/DiffusionPushLogListView.php +++ b/src/applications/diffusion/view/DiffusionPushLogListView.php @@ -25,31 +25,18 @@ final class DiffusionPushLogListView extends AphrontView { $handles = $viewer->loadHandles($handle_phids); - // Figure out which repositories are editable. We only let you see remote - // IPs if you have edit capability on a repository. - $editable_repos = array(); - if ($logs) { - $editable_repos = id(new PhabricatorRepositoryQuery()) - ->setViewer($viewer) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->withPHIDs(mpull($logs, 'getRepositoryPHID')) - ->execute(); - $editable_repos = mpull($editable_repos, null, 'getPHID'); - } + // Only administrators can view remote addresses. + $remotes_visible = $viewer->getIsAdmin(); $rows = array(); $any_host = false; foreach ($logs as $log) { $repository = $log->getRepository(); - // Reveal this if it's valid and the user can edit the repository. - $remote_address = '-'; - if (isset($editable_repos[$log->getRepositoryPHID()])) { + if ($remotes_visible) { $remote_address = $log->getPushEvent()->getRemoteAddress(); + } else { + $remote_address = null; } $event_id = $log->getPushEvent()->getID(); @@ -142,7 +129,7 @@ final class DiffusionPushLogListView extends AphrontView { true, true, true, - true, + $remotes_visible, true, $any_host, )); From ff98f6f522b171bcdabf45933c775e1a4089792e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 12:13:28 -0800 Subject: [PATCH 27/33] Make the remote address rules for Settings > Activity Logs more consistent Summary: Depends on D18971. Ref T13049. The rule is currently "you can see IP addresses for actions which affect your account". There's some legitimate motivation for this, since it's good if you can see that someone you don't recognize has been trying to log into your account. However, this includes cases where an administrator disables/enables your account, or promotes/demotes you to administrator. In these cases, //their// IP is shown! Make the rule: - Administrators can see it (consistent with everything else). - You can see your own actions. - You can see actions which affected you that have no actor (these are things like login attempts). - You can't see other stuff: usually, administrators changing your account settings. Test Plan: Viewed activity log as a non-admin, no longer saw administrator's IP address disclosed in "Demote from Admin" log. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18972 --- .../people/view/PhabricatorUserLogView.php | 58 +++++++++++++++---- .../PhabricatorActivitySettingsPanel.php | 15 ----- 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/applications/people/view/PhabricatorUserLogView.php b/src/applications/people/view/PhabricatorUserLogView.php index 309540c611..72a9378a1a 100644 --- a/src/applications/people/view/PhabricatorUserLogView.php +++ b/src/applications/people/view/PhabricatorUserLogView.php @@ -30,31 +30,65 @@ final class PhabricatorUserLogView extends AphrontView { $action_map = PhabricatorUserLog::getActionTypeMap(); $base_uri = $this->searchBaseURI; + $viewer_phid = $viewer->getPHID(); + $rows = array(); foreach ($logs as $log) { - $ip = $log->getRemoteAddr(); $session = substr($log->getSession(), 0, 6); - if ($base_uri) { - $ip = phutil_tag( - 'a', - array( - 'href' => $base_uri.'?ip='.$ip.'#R', - ), - $ip); + $actor_phid = $log->getActorPHID(); + $user_phid = $log->getUserPHID(); + + if ($viewer->getIsAdmin()) { + $can_see_ip = true; + } else if ($viewer_phid == $actor_phid) { + // You can see the address if you took the action. + $can_see_ip = true; + } else if (!$actor_phid && ($viewer_phid == $user_phid)) { + // You can see the address if it wasn't authenticated and applied + // to you (partial login). + $can_see_ip = true; + } else { + // You can't see the address when an administrator disables your + // account, since it's their address. + $can_see_ip = false; + } + + if ($can_see_ip) { + $ip = $log->getRemoteAddr(); + if ($base_uri) { + $ip = phutil_tag( + 'a', + array( + 'href' => $base_uri.'?ip='.$ip.'#R', + ), + $ip); + } + } else { + $ip = null; } $action = $log->getAction(); $action_name = idx($action_map, $action, $action); + if ($actor_phid) { + $actor_name = $handles[$actor_phid]->renderLink(); + } else { + $actor_name = null; + } + + if ($user_phid) { + $user_name = $handles[$user_phid]->renderLink(); + } else { + $user_name = null; + } + $rows[] = array( phabricator_date($log->getDateCreated(), $viewer), phabricator_time($log->getDateCreated(), $viewer), $action_name, - $log->getActorPHID() - ? $handles[$log->getActorPHID()]->getName() - : null, - $username = $handles[$log->getUserPHID()]->renderLink(), + $actor_name, + $user_name, $ip, $session, ); diff --git a/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php b/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php index eeeca27663..2759f3a26c 100644 --- a/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorActivitySettingsPanel.php @@ -26,21 +26,6 @@ final class PhabricatorActivitySettingsPanel extends PhabricatorSettingsPanel { ->withRelatedPHIDs(array($user->getPHID())) ->executeWithCursorPager($pager); - $phids = array(); - foreach ($logs as $log) { - $phids[] = $log->getUserPHID(); - $phids[] = $log->getActorPHID(); - } - - if ($phids) { - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($viewer) - ->withPHIDs($phids) - ->execute(); - } else { - $handles = array(); - } - $table = id(new PhabricatorUserLogView()) ->setUser($viewer) ->setLogs($logs); From 1e3d1271ada02ee80693901286152422d6e9d731 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 13:01:09 -0800 Subject: [PATCH 28/33] Make push log "flags", "reject code" human readable; add crumbs to pull/push logs Summary: Depends on D18972. Ref T13049. Currently, the "flags" columns renders an inscrutible bitmask which you have to go hunt down in the code. Show a list of flags in human-readable text instead. The "code" column renders a meaningless integer code. Show a text description instead. The pull logs and push logs pages don't have a crumb to go back up out of the current query. Add one. Test Plan: Viewed push logs, no more arcane numbers. Saw and clicked crumbs on each log page. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18973 --- .../DiffusionPullLogListController.php | 5 ++++ .../DiffusionPushLogListController.php | 5 ++++ .../view/DiffusionPushLogListView.php | 30 +++++++++++++++---- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionPullLogListController.php b/src/applications/diffusion/controller/DiffusionPullLogListController.php index 29712be2a4..f3b570c815 100644 --- a/src/applications/diffusion/controller/DiffusionPullLogListController.php +++ b/src/applications/diffusion/controller/DiffusionPullLogListController.php @@ -9,4 +9,9 @@ final class DiffusionPullLogListController ->buildResponse(); } + protected function buildApplicationCrumbs() { + return parent::buildApplicationCrumbs() + ->addTextCrumb(pht('Pull Logs'), $this->getApplicationURI('pulllog/')); + } + } diff --git a/src/applications/diffusion/controller/DiffusionPushLogListController.php b/src/applications/diffusion/controller/DiffusionPushLogListController.php index 658e21674d..a6c612eebe 100644 --- a/src/applications/diffusion/controller/DiffusionPushLogListController.php +++ b/src/applications/diffusion/controller/DiffusionPushLogListController.php @@ -9,4 +9,9 @@ final class DiffusionPushLogListController ->buildResponse(); } + protected function buildApplicationCrumbs() { + return parent::buildApplicationCrumbs() + ->addTextCrumb(pht('Push Logs'), $this->getApplicationURI('pushlog/')); + } + } diff --git a/src/applications/diffusion/view/DiffusionPushLogListView.php b/src/applications/diffusion/view/DiffusionPushLogListView.php index 77f28671c6..e101625752 100644 --- a/src/applications/diffusion/view/DiffusionPushLogListView.php +++ b/src/applications/diffusion/view/DiffusionPushLogListView.php @@ -28,6 +28,9 @@ final class DiffusionPushLogListView extends AphrontView { // Only administrators can view remote addresses. $remotes_visible = $viewer->getIsAdmin(); + $flag_map = PhabricatorRepositoryPushLog::getFlagDisplayNames(); + $reject_map = PhabricatorRepositoryPushLog::getRejectCodeDisplayNames(); + $rows = array(); $any_host = false; foreach ($logs as $log) { @@ -59,6 +62,23 @@ final class DiffusionPushLogListView extends AphrontView { $device = null; } + $flags = $log->getChangeFlags(); + $flag_names = array(); + foreach ($flag_map as $flag_key => $flag_name) { + if (($flags & $flag_key) === $flag_key) { + $flag_names[] = $flag_name; + } + } + $flag_names = phutil_implode_html( + phutil_tag('br'), + $flag_names); + + $reject_code = $log->getPushEvent()->getRejectCode(); + $reject_label = idx( + $reject_map, + $reject_code, + pht('Unknown ("%s")', $reject_code)); + $rows[] = array( phutil_tag( 'a', @@ -85,10 +105,8 @@ final class DiffusionPushLogListView extends AphrontView { 'href' => $repository->getCommitURI($log->getRefNew()), ), $log->getRefNewShort()), - - // TODO: Make these human-readable. - $log->getChangeFlags(), - $log->getPushEvent()->getRejectCode(), + $flag_names, + $reject_label, $viewer->formatShortDateTime($log->getEpoch()), ); } @@ -107,7 +125,7 @@ final class DiffusionPushLogListView extends AphrontView { pht('Old'), pht('New'), pht('Flags'), - pht('Code'), + pht('Result'), pht('Date'), )) ->setColumnClasses( @@ -122,6 +140,8 @@ final class DiffusionPushLogListView extends AphrontView { 'wide', 'n', 'n', + '', + '', 'right', )) ->setColumnVisibility( From c9df8f77c8b5cabb5fe2a3c8874c82d12d1cf431 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Jan 2018 10:37:38 -0800 Subject: [PATCH 29/33] Fix transcription of single-value bulk edit fields ("Assign to") Summary: See PHI333. Some of the cleanup at the tail end of the bulk edit changes made "Assign To" stop working properly, since we don't strip the `array(...)` off the `array(PHID)` value we receive. Test Plan: - Used bulk editor to assign and unassign tasks (single value datasource). - Used bulk editor to change projects (multi-value datasource). Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18975 --- resources/celerity/map.php | 12 ++++++------ .../edittype/PhabricatorPHIDListEditType.php | 12 ++++++++++++ webroot/rsrc/js/phuix/PHUIXFormControl.js | 2 +- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 661ac58c15..d9aebf32bc 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -532,7 +532,7 @@ return array( 'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '04b2ae03', 'rsrc/js/phuix/PHUIXExample.js' => '68af71ca', - 'rsrc/js/phuix/PHUIXFormControl.js' => '1dd0870c', + 'rsrc/js/phuix/PHUIXFormControl.js' => '16ad6224', 'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b', ), 'symbols' => array( @@ -884,7 +884,7 @@ return array( 'phuix-autocomplete' => 'e0731603', 'phuix-button-view' => '8a91e1ac', 'phuix-dropdown-menu' => '04b2ae03', - 'phuix-form-control-view' => '1dd0870c', + 'phuix-form-control-view' => '16ad6224', 'phuix-icon-view' => 'bff6884b', 'policy-css' => '957ea14c', 'policy-edit-css' => '815c66f7', @@ -995,6 +995,10 @@ return array( 'aphront-typeahead-control-css', 'phui-tag-view-css', ), + '16ad6224' => array( + 'javelin-install', + 'javelin-dom', + ), '17bb8539' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1029,10 +1033,6 @@ return array( 'javelin-request', 'javelin-uri', ), - '1dd0870c' => array( - 'javelin-install', - 'javelin-dom', - ), '1e911d0f' => array( 'javelin-stratcom', 'javelin-request', diff --git a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php index eb728a6808..07489521b6 100644 --- a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php +++ b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php @@ -55,4 +55,16 @@ abstract class PhabricatorPHIDListEditType } } + public function getTransactionValueFromBulkEdit($value) { + if (!$this->getIsSingleValue()) { + return $value; + } + + if ($value) { + return head($value); + } + + return null; + } + } diff --git a/webroot/rsrc/js/phuix/PHUIXFormControl.js b/webroot/rsrc/js/phuix/PHUIXFormControl.js index cd67e89691..eb5f32963d 100644 --- a/webroot/rsrc/js/phuix/PHUIXFormControl.js +++ b/webroot/rsrc/js/phuix/PHUIXFormControl.js @@ -293,7 +293,7 @@ JX.install('PHUIXFormControl', { }, _newPoints: function(spec) { - return this._newText(); + return this._newText(spec); }, _newText: function(spec) { From f9336e56940fb9ae00ac7fd967eaf0ef974463c9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 15:58:05 -0800 Subject: [PATCH 30/33] Mangle cells that look a little bit like formulas in CSV files Summary: Fixes T12800. See that task for discussion. When a cell in a CSV begins with "=", "+", "-", or "@", mangle the content to discourage Excel from executing it. This is clumsy, but we support other formats (e.g., JSON) which preserve the data faithfully and you should probably be using JSON if you're going to do anything programmatic with it. We could add two formats or a checkbox or a warning or something but cells with these symbols are fairly rare anyway. Some possible exceptions I can think of are "user monograms" (but we don't export those right now) and "negative numbers" (but also no direct export today). We can add exceptions for those as they arise. Test Plan: Exported a task named `=cmd|'/C evil.exe'!A0`, saw the title get mangled with "(!)" in front. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12800 Differential Revision: https://secure.phabricator.com/D18974 --- .../export/format/PhabricatorCSVExportFormat.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/infrastructure/export/format/PhabricatorCSVExportFormat.php b/src/infrastructure/export/format/PhabricatorCSVExportFormat.php index 67f0a4963a..8f3879d5fb 100644 --- a/src/infrastructure/export/format/PhabricatorCSVExportFormat.php +++ b/src/infrastructure/export/format/PhabricatorCSVExportFormat.php @@ -42,6 +42,16 @@ final class PhabricatorCSVExportFormat private function addRow(array $values) { $row = array(); foreach ($values as $value) { + + // Excel is extremely interested in executing arbitrary code it finds in + // untrusted CSV files downloaded from the internet. When a cell looks + // like it might be too tempting for Excel to ignore, mangle the value + // to dissuade remote code execution. See T12800. + + if (preg_match('/^\s*[+=@-]/', $value)) { + $value = '(!) '.$value; + } + if (preg_match('/\s|,|\"/', $value)) { $value = str_replace('"', '""', $value); $value = '"'.$value.'"'; From 6d5f265a57957d9b7bbba54a0355bd004b8619aa Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Jan 2018 11:11:29 -0800 Subject: [PATCH 31/33] Accept `null` via `conduit.edit` to unassign a task Summary: See . This unusual field doesn't actually accept `null`, although the documentation says it does and that was the intent. Accept `null`, and show `phid|null` in the docs. Test Plan: Viewed docs, saw `phid|null`. Unassigned with `null`. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18976 --- .../ConduitPHIDParameterType.php | 31 +++++++++++++++++-- .../maniphest/editor/ManiphestEditEngine.php | 1 + .../PhabricatorPHIDListEditField.php | 16 ++++++++-- .../edittype/PhabricatorPHIDListEditType.php | 21 +++++++------ 4 files changed, 56 insertions(+), 13 deletions(-) diff --git a/src/applications/conduit/parametertype/ConduitPHIDParameterType.php b/src/applications/conduit/parametertype/ConduitPHIDParameterType.php index 3bb45697dc..eafee4d453 100644 --- a/src/applications/conduit/parametertype/ConduitPHIDParameterType.php +++ b/src/applications/conduit/parametertype/ConduitPHIDParameterType.php @@ -3,9 +3,26 @@ final class ConduitPHIDParameterType extends ConduitParameterType { + private $isNullable; + + public function setIsNullable($is_nullable) { + $this->isNullable = $is_nullable; + return $this; + } + + public function getIsNullable() { + return $this->isNullable; + } + protected function getParameterValue(array $request, $key, $strict) { $value = parent::getParameterValue($request, $key, $strict); + if ($this->getIsNullable()) { + if ($value === null) { + return $value; + } + } + if (!is_string($value)) { $this->raiseValidationException( $request, @@ -17,7 +34,11 @@ final class ConduitPHIDParameterType } protected function getParameterTypeName() { - return 'phid'; + if ($this->getIsNullable()) { + return 'phid|null'; + } else { + return 'phid'; + } } protected function getParameterFormatDescriptions() { @@ -27,9 +48,15 @@ final class ConduitPHIDParameterType } protected function getParameterExamples() { - return array( + $examples = array( '"PHID-WXYZ-1111222233334444"', ); + + if ($this->getIsNullable()) { + $examples[] = 'null'; + } + + return $examples; } } diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php index 359ba493d4..c270104034 100644 --- a/src/applications/maniphest/editor/ManiphestEditEngine.php +++ b/src/applications/maniphest/editor/ManiphestEditEngine.php @@ -196,6 +196,7 @@ EODOCS pht('New task owner, or `null` to unassign.')) ->setTransactionType(ManiphestTaskOwnerTransaction::TRANSACTIONTYPE) ->setIsCopyable(true) + ->setIsNullable(true) ->setSingleValue($object->getOwnerPHID()) ->setCommentActionLabel(pht('Assign / Claim')) ->setCommentActionValue($owner_value), diff --git a/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php index b084c142e5..d2b647b9aa 100644 --- a/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php +++ b/src/applications/transactions/editfield/PhabricatorPHIDListEditField.php @@ -5,6 +5,7 @@ abstract class PhabricatorPHIDListEditField private $useEdgeTransactions; private $isSingleValue; + private $isNullable; public function setUseEdgeTransactions($use_edge_transactions) { $this->useEdgeTransactions = $use_edge_transactions; @@ -30,13 +31,23 @@ abstract class PhabricatorPHIDListEditField return $this->isSingleValue; } + public function setIsNullable($is_nullable) { + $this->isNullable = $is_nullable; + return $this; + } + + public function getIsNullable() { + return $this->isNullable; + } + protected function newHTTPParameterType() { return new AphrontPHIDListHTTPParameterType(); } protected function newConduitParameterType() { if ($this->getIsSingleValue()) { - return new ConduitPHIDParameterType(); + return id(new ConduitPHIDParameterType()) + ->setIsNullable($this->getIsNullable()); } else { return new ConduitPHIDListParameterType(); } @@ -99,7 +110,8 @@ abstract class PhabricatorPHIDListEditField } return id(new PhabricatorDatasourceEditType()) - ->setIsSingleValue($this->getIsSingleValue()); + ->setIsSingleValue($this->getIsSingleValue()) + ->setIsNullable($this->getIsNullable()); } protected function newBulkEditTypes() { diff --git a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php index 07489521b6..763f6ff001 100644 --- a/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php +++ b/src/applications/transactions/edittype/PhabricatorPHIDListEditType.php @@ -6,6 +6,7 @@ abstract class PhabricatorPHIDListEditType private $datasource; private $isSingleValue; private $defaultValue; + private $isNullable; public function setDatasource(PhabricatorTypeaheadDatasource $datasource) { $this->datasource = $datasource; @@ -30,16 +31,17 @@ abstract class PhabricatorPHIDListEditType return $this; } - public function getDefaultValue() { - return $this->defaultValue; + public function setIsNullable($is_nullable) { + $this->isNullable = $is_nullable; + return $this; } - public function getValueType() { - if ($this->getIsSingleValue()) { - return 'phid'; - } else { - return 'list'; - } + public function getIsNullable() { + return $this->isNullable; + } + + public function getDefaultValue() { + return $this->defaultValue; } protected function newConduitParameterType() { @@ -49,7 +51,8 @@ abstract class PhabricatorPHIDListEditType } if ($this->getIsSingleValue()) { - return new ConduitPHIDParameterType(); + return id(new ConduitPHIDParameterType()) + ->setIsNullable($this->getIsNullable()); } else { return new ConduitPHIDListParameterType(); } From f535981c0d61c4ddfa956c0d4baae8573dcc9e94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Jan 2018 11:21:24 -0800 Subject: [PATCH 32/33] Fix a missing getSSHUser() callsite Summary: See . I renamed this method in D18912 but missed this callsite since the workflow doesn't live alongside the other ones. Test Plan: Ran `git push` in an LFS repository over SSH. Before: fatal; after: clean push. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18977 --- .../diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php b/src/applications/diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php index 41c009455d..56297f4d8b 100644 --- a/src/applications/diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php +++ b/src/applications/diffusion/gitlfs/DiffusionGitLFSAuthenticateWorkflow.php @@ -84,7 +84,7 @@ final class DiffusionGitLFSAuthenticateWorkflow // This works even if normal HTTP repository operations are not available // on this host, and does not require the user to have a VCS password. - $user = $this->getUser(); + $user = $this->getSSHUser(); $authorization = DiffusionGitLFSTemporaryTokenType::newHTTPAuthorization( $repository, From 032f5b22941f85340b70da6d164d9dc17cb1b0f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 31 Jan 2018 12:19:19 -0800 Subject: [PATCH 33/33] Allow revisions to revert commits and one another, and commits to revert revisions Summary: Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts ` from a revision. When you do, the corresponding object will get a more-visible cross-reference marker in its timeline: {F5405517} From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges. Test Plan: Used "reverts " and "reverts " in Differential and Diffusion, got sensible results in the timeline. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13057 Differential Revision: https://secure.phabricator.com/D18978 --- .../audit/editor/PhabricatorAuditEditor.php | 56 ++++++++++++++-- .../editor/DifferentialTransactionEditor.php | 39 ++++++++++- ...iffusionCommitRevertedByCommitEdgeType.php | 12 ++-- .../DiffusionCommitRevertsCommitEdgeType.php | 12 ++-- ...habricatorRepositoryCommitHeraldWorker.php | 26 +------- .../PhabricatorApplicationTransaction.php | 14 ++++ .../PhabricatorUSEnglishTranslation.php | 64 +++++++++---------- 7 files changed, 149 insertions(+), 74 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 0cf6339239..049733f777 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -358,9 +358,17 @@ final class PhabricatorAuditEditor array $changes, PhutilMarkupEngine $engine) { - // we are only really trying to find unmentionable phids here... - // don't bother with this outside initial commit (i.e. create) - // transaction + $actor = $this->getActor(); + $result = array(); + + // Some interactions (like "Fixes Txxx" interacting with Maniphest) have + // already been processed, so we're only re-parsing them here to avoid + // generating an extra redundant mention. Other interactions are being + // processed for the first time. + + // We're only recognizing magic in the commit message itself, not in + // audit comments. + $is_commit = false; foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { @@ -370,8 +378,6 @@ final class PhabricatorAuditEditor } } - // "result" is always an array.... - $result = array(); if (!$is_commit) { return $result; } @@ -403,6 +409,46 @@ final class PhabricatorAuditEditor ->withNames($monograms) ->execute(); $phid_map[] = mpull($objects, 'getPHID', 'getPHID'); + + + $reverts_refs = id(new DifferentialCustomFieldRevertsParser()) + ->parseCorpus($huge_block); + $reverts = array_mergev(ipull($reverts_refs, 'monograms')); + if ($reverts) { + // Only allow commits to revert other commits in the same repository. + $reverted_commits = id(new DiffusionCommitQuery()) + ->setViewer($actor) + ->withRepository($object->getRepository()) + ->withIdentifiers($reverts) + ->execute(); + + $reverted_revisions = id(new PhabricatorObjectQuery()) + ->setViewer($actor) + ->withNames($reverts) + ->withTypes( + array( + DifferentialRevisionPHIDType::TYPECONST, + )) + ->execute(); + + $reverted_phids = + mpull($reverted_commits, 'getPHID', 'getPHID') + + mpull($reverted_revisions, 'getPHID', 'getPHID'); + + // NOTE: Skip any write attempts if a user cleverly implies a commit + // reverts itself, although this would be exceptionally clever in Git + // or Mercurial. + unset($reverted_phids[$object->getPHID()]); + + $reverts_edge = DiffusionCommitRevertsCommitEdgeType::EDGECONST; + $result[] = id(new PhabricatorAuditTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $reverts_edge) + ->setNewValue(array('+' => $reverted_phids)); + + $phid_map[] = $reverted_phids; + } + $phid_map = array_mergev($phid_map); $this->setUnmentionablePHIDMap($phid_map); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index ecd1e6c95c..3a1537b01d 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -919,7 +919,44 @@ final class DifferentialTransactionEditor } } - $this->setUnmentionablePHIDMap(array_merge($task_phids, $rev_phids)); + $revert_refs = id(new DifferentialCustomFieldRevertsParser()) + ->parseCorpus($content_block); + + $revert_monograms = array(); + foreach ($revert_refs as $match) { + foreach ($match['monograms'] as $monogram) { + $revert_monograms[] = $monogram; + } + } + + if ($revert_monograms) { + $revert_objects = id(new PhabricatorObjectQuery()) + ->setViewer($this->getActor()) + ->withNames($revert_monograms) + ->withTypes( + array( + DifferentialRevisionPHIDType::TYPECONST, + PhabricatorRepositoryCommitPHIDType::TYPECONST, + )) + ->execute(); + + $revert_phids = mpull($revert_objects, 'getPHID', 'getPHID'); + + // Don't let an object revert itself, although other silly stuff like + // cycles of objects reverting each other is not prevented. + unset($revert_phids[$object->getPHID()]); + + $revert_type = DiffusionCommitRevertsCommitEdgeType::EDGECONST; + $edges[$revert_type] = $revert_phids; + } else { + $revert_phids = array(); + } + + $this->setUnmentionablePHIDMap( + array_merge( + $task_phids, + $rev_phids, + $revert_phids)); $result = array(); foreach ($edges as $type => $specs) { diff --git a/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php index 53c39e54c9..ae59a3b1e6 100644 --- a/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php +++ b/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php @@ -19,7 +19,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $add_edges) { return pht( - '%s added %s reverting commit(s): %s.', + '%s added %s reverting change(s): %s.', $actor, $add_count, $add_edges); @@ -31,7 +31,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $rem_edges) { return pht( - '%s removed %s reverting commit(s): %s.', + '%s removed %s reverting change(s): %s.', $actor, $rem_count, $rem_edges); @@ -46,7 +46,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $rem_edges) { return pht( - '%s edited reverting commit(s), added %s: %s; removed %s: %s.', + '%s edited reverting change(s), added %s: %s; removed %s: %s.', $actor, $add_count, $add_edges, @@ -61,7 +61,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $add_edges) { return pht( - '%s added %s reverting commit(s) for %s: %s.', + '%s added %s reverting change(s) for %s: %s.', $actor, $add_count, $object, @@ -75,7 +75,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $rem_edges) { return pht( - '%s removed %s reverting commit(s) for %s: %s.', + '%s removed %s reverting change(s) for %s: %s.', $actor, $rem_count, $object, @@ -92,7 +92,7 @@ final class DiffusionCommitRevertedByCommitEdgeType $rem_edges) { return pht( - '%s edited reverting commit(s) for %s, added %s: %s; removed %s: %s.', + '%s edited reverting change(s) for %s, added %s: %s; removed %s: %s.', $actor, $object, $add_count, diff --git a/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php index 8f32797173..ee0223c966 100644 --- a/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php +++ b/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php @@ -22,7 +22,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $add_edges) { return pht( - '%s added %s reverted commit(s): %s.', + '%s added %s reverted change(s): %s.', $actor, $add_count, $add_edges); @@ -34,7 +34,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $rem_edges) { return pht( - '%s removed %s reverted commit(s): %s.', + '%s removed %s reverted change(s): %s.', $actor, $rem_count, $rem_edges); @@ -49,7 +49,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $rem_edges) { return pht( - '%s edited reverted commit(s), added %s: %s; removed %s: %s.', + '%s edited reverted change(s), added %s: %s; removed %s: %s.', $actor, $add_count, $add_edges, @@ -64,7 +64,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $add_edges) { return pht( - '%s added %s reverted commit(s) for %s: %s.', + '%s added %s reverted change(s) for %s: %s.', $actor, $add_count, $object, @@ -78,7 +78,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $rem_edges) { return pht( - '%s removed %s reverted commit(s) for %s: %s.', + '%s removed %s reverted change(s) for %s: %s.', $actor, $rem_count, $object, @@ -95,7 +95,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType { $rem_edges) { return pht( - '%s edited reverted commit(s) for %s, added %s: %s; removed %s: %s.', + '%s edited reverted change(s) for %s, added %s: %s; removed %s: %s.', $actor, $object, $add_count, diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 9fb6667924..818d1e8781 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -15,6 +15,7 @@ final class PhabricatorRepositoryCommitHeraldWorker protected function parseCommit( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { + $viewer = PhabricatorUser::getOmnipotentUser(); if ($this->shouldSkipImportStep()) { // This worker has no followup tasks, so we can just bail out @@ -50,7 +51,7 @@ final class PhabricatorRepositoryCommitHeraldWorker id(new PhabricatorDiffusionApplication())->getPHID()); $editor = id(new PhabricatorAuditEditor()) - ->setActor(PhabricatorUser::getOmnipotentUser()) + ->setActor($viewer) ->setActingAsPHID($acting_as_phid) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true) @@ -69,29 +70,6 @@ final class PhabricatorRepositoryCommitHeraldWorker 'committerPHID' => $data->getCommitDetail('committerPHID'), )); - $reverts_refs = id(new DifferentialCustomFieldRevertsParser()) - ->parseCorpus($data->getCommitMessage()); - $reverts = array_mergev(ipull($reverts_refs, 'monograms')); - - if ($reverts) { - $reverted_commits = id(new DiffusionCommitQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withRepository($repository) - ->withIdentifiers($reverts) - ->execute(); - $reverted_commit_phids = mpull($reverted_commits, 'getPHID', 'getPHID'); - - // NOTE: Skip any write attempts if a user cleverly implies a commit - // reverts itself. - unset($reverted_commit_phids[$commit->getPHID()]); - - $reverts_edge = DiffusionCommitRevertsCommitEdgeType::EDGECONST; - $xactions[] = id(new PhabricatorAuditTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) - ->setMetadataValue('edge:type', $reverts_edge) - ->setNewValue(array('+' => array_fuse($reverted_commit_phids))); - } - try { $raw_patch = $this->loadRawPatchText($repository, $commit); } catch (Exception $ex) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 97b57009ec..d5c28b83bf 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -458,6 +458,12 @@ abstract class PhabricatorApplicationTransaction case PhabricatorTransactions::TYPE_JOIN_POLICY: return 'fa-lock'; case PhabricatorTransactions::TYPE_EDGE: + switch ($this->getMetadataValue('edge:type')) { + case DiffusionCommitRevertedByCommitEdgeType::EDGECONST: + return 'fa-undo'; + case DiffusionCommitRevertsCommitEdgeType::EDGECONST: + return 'fa-ambulance'; + } return 'fa-link'; case PhabricatorTransactions::TYPE_BUILDABLE: return 'fa-wrench'; @@ -496,6 +502,14 @@ abstract class PhabricatorApplicationTransaction return 'black'; } break; + case PhabricatorTransactions::TYPE_EDGE: + switch ($this->getMetadataValue('edge:type')) { + case DiffusionCommitRevertedByCommitEdgeType::EDGECONST: + return 'pink'; + case DiffusionCommitRevertsCommitEdgeType::EDGECONST: + return 'sky'; + } + break; case PhabricatorTransactions::TYPE_BUILDABLE: switch ($this->getNewValue()) { case HarbormasterBuildable::STATUS_PASSED: diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 40cedacf06..eb74f8debf 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -676,73 +676,73 @@ final class PhabricatorUSEnglishTranslation '%s edited commit(s), added %s: %s; removed %s: %s.' => '%s edited commits, added %3$s; removed %5$s.', - '%s added %s reverted commit(s): %s.' => array( + '%s added %s reverted change(s): %s.' => array( array( - '%s added a reverted commit: %3$s.', - '%s added reverted commits: %3$s.', + '%s added a reverted change: %3$s.', + '%s added reverted changes: %3$s.', ), ), - '%s removed %s reverted commit(s): %s.' => array( + '%s removed %s reverted change(s): %s.' => array( array( - '%s removed a reverted commit: %3$s.', - '%s removed reverted commits: %3$s.', + '%s removed a reverted change: %3$s.', + '%s removed reverted changes: %3$s.', ), ), - '%s edited reverted commit(s), added %s: %s; removed %s: %s.' => - '%s edited reverted commits, added %3$s; removed %5$s.', + '%s edited reverted change(s), added %s: %s; removed %s: %s.' => + '%s edited reverted changes, added %3$s; removed %5$s.', - '%s added %s reverted commit(s) for %s: %s.' => array( + '%s added %s reverted change(s) for %s: %s.' => array( array( - '%s added a reverted commit for %3$s: %4$s.', - '%s added reverted commits for %3$s: %4$s.', + '%s added a reverted change for %3$s: %4$s.', + '%s added reverted changes for %3$s: %4$s.', ), ), - '%s removed %s reverted commit(s) for %s: %s.' => array( + '%s removed %s reverted change(s) for %s: %s.' => array( array( - '%s removed a reverted commit for %3$s: %4$s.', - '%s removed reverted commits for %3$s: %4$s.', + '%s removed a reverted change for %3$s: %4$s.', + '%s removed reverted changes for %3$s: %4$s.', ), ), - '%s edited reverted commit(s) for %s, added %s: %s; removed %s: %s.' => - '%s edited reverted commits for %2$s, added %4$s; removed %6$s.', + '%s edited reverted change(s) for %s, added %s: %s; removed %s: %s.' => + '%s edited reverted changes for %2$s, added %4$s; removed %6$s.', - '%s added %s reverting commit(s): %s.' => array( + '%s added %s reverting change(s): %s.' => array( array( - '%s added a reverting commit: %3$s.', - '%s added reverting commits: %3$s.', + '%s added a reverting change: %3$s.', + '%s added reverting changes: %3$s.', ), ), - '%s removed %s reverting commit(s): %s.' => array( + '%s removed %s reverting change(s): %s.' => array( array( - '%s removed a reverting commit: %3$s.', - '%s removed reverting commits: %3$s.', + '%s removed a reverting change: %3$s.', + '%s removed reverting changes: %3$s.', ), ), - '%s edited reverting commit(s), added %s: %s; removed %s: %s.' => - '%s edited reverting commits, added %3$s; removed %5$s.', + '%s edited reverting change(s), added %s: %s; removed %s: %s.' => + '%s edited reverting changes, added %3$s; removed %5$s.', - '%s added %s reverting commit(s) for %s: %s.' => array( + '%s added %s reverting change(s) for %s: %s.' => array( array( - '%s added a reverting commit for %3$s: %4$s.', - '%s added reverting commits for %3$s: %4$s.', + '%s added a reverting change for %3$s: %4$s.', + '%s added reverting changes for %3$s: %4$s.', ), ), - '%s removed %s reverting commit(s) for %s: %s.' => array( + '%s removed %s reverting change(s) for %s: %s.' => array( array( - '%s removed a reverting commit for %3$s: %4$s.', - '%s removed reverting commits for %3$s: %4$s.', + '%s removed a reverting change for %3$s: %4$s.', + '%s removed reverting changes for %3$s: %4$s.', ), ), - '%s edited reverting commit(s) for %s, added %s: %s; removed %s: %s.' => - '%s edited reverting commits for %s, added %4$s; removed %6$s.', + '%s edited reverting change(s) for %s, added %s: %s; removed %s: %s.' => + '%s edited reverting changes for %s, added %4$s; removed %6$s.', '%s changed project member(s), added %d: %s; removed %d: %s.' => '%s changed project members, added %3$s; removed %5$s.',