From 1e51f1d0dc25fd7e1f5fd653272e95333f509607 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 19 Jan 2018 05:49:44 -0800 Subject: [PATCH] Reuse more transaction construction code in bulk editor Summary: Ref T13025. Currently, the bulk editor takes an HTTP request and emits a list of "raw" transactions (simple dictionaries). This goes into the job queue, and the background job builds a real transaction. However, the logic to turn an HTTP request into a raw transaction is ending up with some duplication, since we generally already have logic to turn an HTTP request into a full object. Instead: build real objects first, then serialize them to dictionaries. Send those to the job queue, rebuild them into objects again, and we end up in the same spot with a little less code duplication. Finally, delete the mostly-copied code. Test Plan: Used bulk editor to add comments, projects, and rename tasks. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13025 Differential Revision: https://secure.phabricator.com/D18875 --- .../bulk/PhabricatorEditEngineBulkJobType.php | 8 +++-- .../editengine/PhabricatorEditEngine.php | 32 +++++++++++++------ .../edittype/PhabricatorCommentEditType.php | 11 ------- .../PhabricatorDatasourceEditType.php | 16 ---------- .../edittype/PhabricatorEdgeEditType.php | 14 -------- .../edittype/PhabricatorEditType.php | 5 --- 6 files changed, 29 insertions(+), 57 deletions(-) diff --git a/src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php b/src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php index 9728e2c5b5..e7ff5d3414 100644 --- a/src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php +++ b/src/applications/transactions/bulk/PhabricatorEditEngineBulkJobType.php @@ -72,8 +72,8 @@ final class PhabricatorEditEngineBulkJobType $xaction = $object->getApplicationTransactionTemplate() ->setTransactionType($raw_xaction['type']); - if (isset($raw_xaction['value'])) { - $xaction->setNewValue($raw_xaction['value']); + if (isset($raw_xaction['new'])) { + $xaction->setNewValue($raw_xaction['new']); } if (isset($raw_xaction['comment'])) { @@ -88,6 +88,10 @@ final class PhabricatorEditEngineBulkJobType } } + if (array_key_exists('old', $raw_xaction)) { + $xaction->setOldValue($raw_xaction['old']); + } + $xactions[] = $xaction; } diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 4a0d109590..2abc419895 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -2472,14 +2472,15 @@ abstract class PhabricatorEditEngine $fields = $this->buildEditFields($object); $edit_types = $this->getBulkEditTypesFromFields($fields); + $template = $object->getApplicationTransactionTemplate(); + $raw_xactions = array(); foreach ($xactions as $key => $xaction) { PhutilTypeSpec::checkMap( $xaction, array( 'type' => 'string', 'value' => 'optional wild', - 'comment' => 'optional string', )); $type = $xaction['type']; @@ -2497,18 +2498,31 @@ abstract class PhabricatorEditEngine // but it's possible that this isn't the case. $xaction['type'] = $edit_type->getTransactionType(); - $xaction['metadata'] = $edit_type->getMetadata(); + $xaction_objects = $edit_type->generateTransactions( + clone $template, + $xaction); - $xaction = $edit_type->newRawBulkTransaction($xaction); - if ($xaction === null) { - unset($xactions[$key]); - continue; + foreach ($xaction_objects as $xaction_object) { + $raw_xaction = array( + 'type' => $xaction_object->getTransactionType(), + 'metadata' => $xaction_object->getMetadata(), + 'new' => $xaction_object->getNewValue(), + ); + + if ($xaction_object->hasOldValue()) { + $raw_xaction['old'] = $xaction_object->getOldValue(); + } + + if ($xaction_object->hasComment()) { + $comment = $xaction_object->getComment(); + $raw_xaction['comment'] = $comment->getContent(); + } + + $raw_xactions[] = $raw_xaction; } - - $xactions[$key] = $xaction; } - return $xactions; + return $raw_xactions; } private function getBulkEditTypesFromFields(array $fields) { diff --git a/src/applications/transactions/edittype/PhabricatorCommentEditType.php b/src/applications/transactions/edittype/PhabricatorCommentEditType.php index 0bc82f7dc7..38194e4498 100644 --- a/src/applications/transactions/edittype/PhabricatorCommentEditType.php +++ b/src/applications/transactions/edittype/PhabricatorCommentEditType.php @@ -10,17 +10,6 @@ final class PhabricatorCommentEditType extends PhabricatorEditType { return new BulkRemarkupParameterType(); } - public function newRawBulkTransaction(array $xaction) { - if (!strlen($xaction['value'])) { - return null; - } - - $xaction['comment'] = $xaction['value']; - unset($xaction['value']); - - return $xaction; - } - public function generateTransactions( PhabricatorApplicationTransaction $template, array $spec) { diff --git a/src/applications/transactions/edittype/PhabricatorDatasourceEditType.php b/src/applications/transactions/edittype/PhabricatorDatasourceEditType.php index dcb4f6f023..b6e1a5d7bb 100644 --- a/src/applications/transactions/edittype/PhabricatorDatasourceEditType.php +++ b/src/applications/transactions/edittype/PhabricatorDatasourceEditType.php @@ -19,20 +19,4 @@ final class PhabricatorDatasourceEditType return '?'; } - public function newRawBulkTransaction(array $xaction) { - $value = idx($xaction, 'value'); - - if ($this->getIsSingleValue()) { - if ($value) { - $value = head($value); - } else { - $value = null; - } - - $xaction['value'] = $value; - } - - return $xaction; - } - } diff --git a/src/applications/transactions/edittype/PhabricatorEdgeEditType.php b/src/applications/transactions/edittype/PhabricatorEdgeEditType.php index 976ab3ec8e..9398cfb9aa 100644 --- a/src/applications/transactions/edittype/PhabricatorEdgeEditType.php +++ b/src/applications/transactions/edittype/PhabricatorEdgeEditType.php @@ -43,18 +43,4 @@ final class PhabricatorEdgeEditType ->setDatasource($this->getDatasource()); } - public function newRawBulkTransaction(array $xaction) { - $value = idx($xaction, 'value'); - - if ($this->getEdgeOperation() !== null) { - $value = array_fuse($value); - $value = array( - $this->getEdgeOperation() => $value, - ); - $xaction['value'] = $value; - } - - return $xaction; - } - } diff --git a/src/applications/transactions/edittype/PhabricatorEditType.php b/src/applications/transactions/edittype/PhabricatorEditType.php index 127fe93e3a..de9404866d 100644 --- a/src/applications/transactions/edittype/PhabricatorEditType.php +++ b/src/applications/transactions/edittype/PhabricatorEditType.php @@ -115,11 +115,6 @@ abstract class PhabricatorEditType extends Phobject { } - public function newRawBulkTransaction(array $xaction) { - return $xaction; - } - - /* -( Conduit )------------------------------------------------------------ */