From 22c3e49b5176f97a18d2c51a4baf223d2ca05add Mon Sep 17 00:00:00 2001
From: Chad Little <chad@phacility.com>
Date: Fri, 5 May 2017 19:50:05 -0700
Subject: [PATCH] (stable) 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
---
 .../editor/PhabricatorMacroEditEngine.php     |  2 +-
 .../macro/editor/PhabricatorMacroEditor.php   | 38 -------------------
 .../PhabricatorMacroAudioTransaction.php      | 28 ++++++++++++++
 .../PhabricatorMacroFileTransaction.php       | 28 ++++++++++++++
 4 files changed, 57 insertions(+), 39 deletions(-)

diff --git a/src/applications/macro/editor/PhabricatorMacroEditEngine.php b/src/applications/macro/editor/PhabricatorMacroEditEngine.php
index 3f472ff0ce..2cafea937f 100644
--- a/src/applications/macro/editor/PhabricatorMacroEditEngine.php
+++ b/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() {
diff --git a/src/applications/macro/editor/PhabricatorMacroEditor.php b/src/applications/macro/editor/PhabricatorMacroEditor.php
index d9067c5367..5d28b78f5f 100644
--- a/src/applications/macro/editor/PhabricatorMacroEditor.php
+++ b/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) {
diff --git a/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php b/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php
index aacf9f4016..26dc64c1f3 100644
--- a/src/applications/macro/xaction/PhabricatorMacroAudioTransaction.php
+++ b/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();
diff --git a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php b/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php
index 77c9f24dd9..35091cd585 100644
--- a/src/applications/macro/xaction/PhabricatorMacroFileTransaction.php
+++ b/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.',