From e469f8594ed310a662ba98aa586e560be6c4ab87 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 20 Dec 2018 13:38:34 -0800 Subject: [PATCH] Implement Pholio file add/remove transactions without "applyInitialEffects" Summary: Depends on D19924. Ref T11351. Like in D19924, apply these transactions by accepting PHIDs instead of objects so we don't need to juggle the `Image` objects down to PHIDs in `applyInitialEffects`. (Validation is a little light here for now, but only first-party code can reach this, and you can't violate policies or do anything truly bad even if you could pick values to feed in here.) Test Plan: Created and edited Mocks; added, removed, and reordered images in a Pholio Mock. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T11351 Differential Revision: https://secure.phabricator.com/D19926 --- .../controller/PholioMockEditController.php | 8 ++- .../pholio/editor/PholioMockEditor.php | 68 +------------------ .../xaction/PholioImageFileTransaction.php | 58 +++++++++------- 3 files changed, 41 insertions(+), 93 deletions(-) diff --git a/src/applications/pholio/controller/PholioMockEditController.php b/src/applications/pholio/controller/PholioMockEditController.php index 9187d9d61f..3e33a5eff8 100644 --- a/src/applications/pholio/controller/PholioMockEditController.php +++ b/src/applications/pholio/controller/PholioMockEditController.php @@ -162,11 +162,13 @@ final class PholioMockEditController extends PholioController { ->attachFile($file) ->setName(strlen($title) ? $title : $file->getName()) ->setDescription($description) - ->setSequence($sequence); + ->setSequence($sequence) + ->save(); + $xactions[] = id(new PholioTransaction()) ->setTransactionType(PholioImageFileTransaction::TRANSACTIONTYPE) ->setNewValue( - array('+' => array($add_image))); + array('+' => array($add_image->getPHID()))); $posted_mock_images[] = $add_image; } else { $xactions[] = id(new PholioTransaction()) @@ -193,7 +195,7 @@ final class PholioMockEditController extends PholioController { $xactions[] = id(new PholioTransaction()) ->setTransactionType(PholioImageFileTransaction::TRANSACTIONTYPE) ->setNewValue( - array('-' => array($mock_image))); + array('-' => array($mock_image->getPHID()))); } } diff --git a/src/applications/pholio/editor/PholioMockEditor.php b/src/applications/pholio/editor/PholioMockEditor.php index 4c0b14261b..7f4d589934 100644 --- a/src/applications/pholio/editor/PholioMockEditor.php +++ b/src/applications/pholio/editor/PholioMockEditor.php @@ -2,8 +2,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { - private $newImages = array(); - private $images = array(); public function getEditorApplicationClass() { @@ -14,16 +12,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { return pht('Pholio Mocks'); } - private function setNewImages(array $new_images) { - assert_instances_of($new_images, 'PholioImage'); - $this->newImages = $new_images; - return $this; - } - - public function getNewImages() { - return $this->newImages; - } - public function getCreateObjectTitle($author, $object) { return pht('%s created this mock.', $author); } @@ -43,56 +31,6 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { return $types; } - protected function shouldApplyInitialEffects( - PhabricatorLiskDAO $object, - array $xactions) { - - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PholioImageFileTransaction::TRANSACTIONTYPE: - return true; - } - } - return false; - } - - protected function applyInitialEffects( - PhabricatorLiskDAO $object, - array $xactions) { - - $new_images = array(); - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PholioImageFileTransaction::TRANSACTIONTYPE: - $new_value = $xaction->getNewValue(); - foreach ($new_value as $key => $txn_images) { - if ($key != '+') { - continue; - } - foreach ($txn_images as $image) { - $image->save(); - $new_images[] = $image; - } - } - break; - } - } - $this->setNewImages($new_images); - } - - protected function applyFinalEffects( - PhabricatorLiskDAO $object, - array $xactions) { - - $images = $this->getNewImages(); - foreach ($images as $image) { - $image->setMockPHID($object->getPHID()); - $image->save(); - } - - return $xactions; - } - protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { @@ -105,11 +43,11 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { } protected function buildMailTemplate(PhabricatorLiskDAO $object) { - $id = $object->getID(); + $monogram = $object->getMonogram(); $name = $object->getName(); return id(new PhabricatorMetaMTAMail()) - ->setSubject("M{$id}: {$name}"); + ->setSubject("{$monogram}: {$name}"); } protected function getMailTo(PhabricatorLiskDAO $object) { @@ -150,7 +88,7 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor { $body->addLinkSection( pht('MOCK DETAIL'), - PhabricatorEnv::getProductionURI('/M'.$object->getID())); + PhabricatorEnv::getProductionURI($object->getURI())); return $body; } diff --git a/src/applications/pholio/xaction/PholioImageFileTransaction.php b/src/applications/pholio/xaction/PholioImageFileTransaction.php index 6d257c313e..e18fca28e2 100644 --- a/src/applications/pholio/xaction/PholioImageFileTransaction.php +++ b/src/applications/pholio/xaction/PholioImageFileTransaction.php @@ -11,28 +11,34 @@ final class PholioImageFileTransaction } public function generateNewValue($object, $value) { - $new_value = array(); - foreach ($value as $key => $images) { - $new_value[$key] = mpull($images, 'getPHID'); - } - $old = array_fuse($this->getOldValue()); - return $this->getEditor()->getPHIDList($old, $new_value); + $editor = $this->getEditor(); + + $old_value = $this->getOldValue(); + $new_value = $value; + + return $editor->getPHIDList($old_value, $new_value); } - public function applyInternalEffects($object, $value) { + public function applyExternalEffects($object, $value) { $old_map = array_fuse($this->getOldValue()); $new_map = array_fuse($this->getNewValue()); - $obsolete_map = array_diff_key($old_map, $new_map); - $images = $object->getActiveImages(); - foreach ($images as $seq => $image) { - if (isset($obsolete_map[$image->getPHID()])) { - $image->setIsObsolete(1); - $image->save(); - unset($images[$seq]); - } + $add_map = array_diff_key($new_map, $old_map); + $rem_map = array_diff_key($old_map, $new_map); + + $editor = $this->getEditor(); + + foreach ($rem_map as $phid) { + $editor->loadPholioImage($object, $phid) + ->setIsObsolete(1) + ->save(); + } + + foreach ($add_map as $phid) { + $editor->loadPholioImage($object, $phid) + ->setMockPHID($object->getPHID()) + ->save(); } - $object->attachImages($images); } public function getTitle() { @@ -95,18 +101,20 @@ final class PholioImageFileTransaction } public function extractFilePHIDs($object, $value) { - $images = $this->getEditor()->getNewImages(); - $images = mpull($images, null, 'getPHID'); + $editor = $this->getEditor(); + // NOTE: This method is a little weird (and includes ALL the file PHIDs, + // including old file PHIDs) because we currently don't have a storage + // object when called. This might change at some point. + + $new_phids = $value; $file_phids = array(); - foreach ($value as $image_phid) { - $image = idx($images, $image_phid); - if (!$image) { - continue; - } - $file_phids[] = $image->getFilePHID(); + foreach ($new_phids as $phid) { + $file_phids[] = $editor->loadPholioImage($object, $phid) + ->getFilePHID(); } + return $file_phids; } @@ -114,7 +122,7 @@ final class PholioImageFileTransaction $object, PhabricatorApplicationTransaction $u, PhabricatorApplicationTransaction $v) { - return $this->getEditor()->mergePHIDOrEdgeTransactions($u, $v); + return $this->getEditor()->mergePHIDOrEdgeTransactions($u, $v); } }