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

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())));
         }
       }
 

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;
   }

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);
   }
 
 }