Fix file attach bug in Macro

Summary: This was mis-tested by only using one account, which could always see the image. External transaction moved file attachment to the modular transaction for file and audio instead.

Test Plan: Test adding audio and a macro on a pleb account, visit with normal account and see macro fine.

Reviewers: epriestley, amckinley

Reviewed By: amckinley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17836

src/applications/macro/editor/PhabricatorMacroEditEngine.php

@@ -6,7 +6,7 @@ final class PhabricatorMacroEditEngine
   const ENGINECONST = 'macro.image';
 
   public function getEngineName() {
-    return pht('Macro Imagea');
+    return pht('Macro Image');
   }
 
   public function getSummaryHeader() {

src/applications/macro/editor/PhabricatorMacroEditor.php

@@ -19,44 +19,6 @@ final class PhabricatorMacroEditor
     return pht('%s created %s.', $author, $object);
   }
 
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorMacroFileTransaction::TRANSACTIONTYPE:
-      case PhabricatorMacroAudioTransaction::TRANSACTIONTYPE:
-        // When changing a macro's image or audio, attach the underlying files
-        // to the macro (and detach the old files).
-        $old = $xaction->getOldValue();
-        $new = $xaction->getNewValue();
-        $all = array();
-        if ($old) {
-          $all[] = $old;
-        }
-        if ($new) {
-          $all[] = $new;
-        }
-
-        $files = id(new PhabricatorFileQuery())
-          ->setViewer($this->requireActor())
-          ->withPHIDs($all)
-          ->execute();
-        $files = mpull($files, null, 'getPHID');
-
-        $old_file = idx($files, $old);
-        if ($old_file) {
-          $old_file->detachFromObject($object->getPHID());
-        }
-
-        $new_file = idx($files, $new);
-        if ($new_file) {
-          $new_file->attachToObject($object->getPHID());
-        }
-        break;
-    }
-  }
-
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {

src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php

@@ -13,6 +13,34 @@ final class PhabricatorMacroAudioTransaction
     $object->setAudioPHID($value);
   }
 
+  public function applyExternalEffects($object, $value) {
+    $old = $this->generateOldValue($object);
+    $new = $value;
+    $all = array();
+    if ($old) {
+      $all[] = $old;
+    }
+    if ($new) {
+      $all[] = $new;
+    }
+
+    $files = id(new PhabricatorFileQuery())
+      ->setViewer($this->getActor())
+      ->withPHIDs($all)
+      ->execute();
+    $files = mpull($files, null, 'getPHID');
+
+    $old_file = idx($files, $old);
+    if ($old_file) {
+      $old_file->detachFromObject($object->getPHID());
+    }
+
+    $new_file = idx($files, $new);
+    if ($new_file) {
+      $new_file->attachToObject($object->getPHID());
+    }
+  }
+
   public function getTitle() {
     $new = $this->getNewValue();
     $old = $this->getOldValue();

src/applications/macro/xaction/PhabricatorMacroFileTransaction.php

@@ -13,6 +13,34 @@ final class PhabricatorMacroFileTransaction
     $object->setFilePHID($value);
   }
 
+  public function applyExternalEffects($object, $value) {
+    $old = $this->generateOldValue($object);
+    $new = $value;
+    $all = array();
+    if ($old) {
+      $all[] = $old;
+    }
+    if ($new) {
+      $all[] = $new;
+    }
+
+    $files = id(new PhabricatorFileQuery())
+      ->setViewer($this->getActor())
+      ->withPHIDs($all)
+      ->execute();
+    $files = mpull($files, null, 'getPHID');
+
+    $old_file = idx($files, $old);
+    if ($old_file) {
+      $old_file->detachFromObject($object->getPHID());
+    }
+
+    $new_file = idx($files, $new);
+    if ($new_file) {
+      $new_file->attachToObject($object->getPHID());
+    }
+  }
+
   public function getTitle() {
     return pht(
       '%s changed the image for this macro.',