Clean up some rough Macro transaction edges

Summary: Ref T12685, cleans up various macro issues, remove subscribers, fix feed stories, etc.

Test Plan: Create a new macro, see no subscribers, edit various macros.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12685

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

src/applications/macro/editor/PhabricatorMacroEditEngine.php

@@ -21,6 +21,10 @@ final class PhabricatorMacroEditEngine
     return 'PhabricatorMacroApplication';
   }
 
+  public function isEngineConfigurable() {
+    return false;
+  }
+
   protected function newEditableObject() {
     $viewer = $this->getViewer();
     return PhabricatorFileImageMacro::initializeNewFileImageMacro($viewer);
@@ -35,7 +39,7 @@ final class PhabricatorMacroEditEngine
   }
 
   protected function getObjectEditTitleText($object) {
-    return pht('Edit %s', $object->getName());
+    return pht('Edit Macro %s', $object->getName());
   }
 
   protected function getObjectEditShortText($object) {
@@ -63,6 +67,19 @@ final class PhabricatorMacroEditEngine
       PhabricatorMacroManageCapability::CAPABILITY);
   }
 
+  protected function willConfigureFields($object, array $fields) {
+    if ($this->getIsCreate()) {
+      $subscribers_field = idx($fields,
+        PhabricatorSubscriptionsEditEngineExtension::FIELDKEY);
+      if ($subscribers_field) {
+        // By default, hide the subscribers field when creating a macro
+        // because it makes the workflow SO HARD and wastes SO MUCH TIME.
+        $subscribers_field->setIsHidden(true);
+      }
+    }
+    return $fields;
+  }
+
   protected function buildCustomEditFields($object) {
 
     return array(
@@ -73,6 +90,7 @@ final class PhabricatorMacroEditEngine
         ->setConduitDescription(pht('Rename the macro.'))
         ->setConduitTypeDescription(pht('New macro name.'))
         ->setTransactionType(PhabricatorMacroNameTransaction::TRANSACTIONTYPE)
+        ->setIsRequired(true)
         ->setValue($object->getName()),
       id(new PhabricatorFileEditField())
         ->setKey('filePHID')
@@ -80,7 +98,8 @@ final class PhabricatorMacroEditEngine
         ->setDescription(pht('Image file to import.'))
         ->setTransactionType(PhabricatorMacroFileTransaction::TRANSACTIONTYPE)
         ->setConduitDescription(pht('File PHID to import.'))
-        ->setConduitTypeDescription(pht('File PHID.')),
+        ->setConduitTypeDescription(pht('File PHID.'))
+        ->setValue($object->getFilePHID()),
     );
 
   }

src/applications/macro/xaction/PhabricatorMacroFileTransaction.php

@@ -49,7 +49,7 @@ final class PhabricatorMacroFileTransaction
 
   public function getTitleForFeed() {
     return pht(
-      '%s changed the image for macro %s.',
+      '%s changed the image for %s.',
       $this->renderAuthor(),
       $this->renderObject());
   }
@@ -58,29 +58,37 @@ final class PhabricatorMacroFileTransaction
     $errors = array();
     $viewer = $this->getActor();
 
+    $old_phid = $object->getFilePHID();
+
     foreach ($xactions as $xaction) {
       $file_phid = $xaction->getNewValue();
 
-      if ($this->isEmptyTextTransaction($file_phid, $xactions)) {
+      if (!$old_phid) {
-        $errors[] = $this->newRequiredError(
+        if ($this->isEmptyTextTransaction($file_phid, $xactions)) {
-          pht('Image macros must have a file.'));
+          $errors[] = $this->newRequiredError(
+            pht('Image macros must have a file.'));
+          return $errors;
+        }
       }
 
-      $file = id(new PhabricatorFileQuery())
+      // Only validate if file was uploaded
-        ->setViewer($viewer)
+      if ($file_phid) {
-        ->withPHIDs(array($file_phid))
+        $file = id(new PhabricatorFileQuery())
-        ->executeOne();
+          ->setViewer($viewer)
+          ->withPHIDs(array($file_phid))
+          ->executeOne();
 
-      if (!$file) {
+        if (!$file) {
-        $errors[] = $this->newInvalidError(
-          pht('"%s" is not a valid file PHID.',
-          $file_phid));
-      } else {
-        if (!$file->isViewableInBrowser()) {
-          $mime_type = $file->getMimeType();
           $errors[] = $this->newInvalidError(
-            pht('File mime type of "%s" is not a valid viewable image.',
+            pht('"%s" is not a valid file PHID.',
-            $mime_type));
+            $file_phid));
+        } else {
+          if (!$file->isViewableImage()) {
+            $mime_type = $file->getMimeType();
+            $errors[] = $this->newInvalidError(
+              pht('File mime type of "%s" is not a valid viewable image.',
+              $mime_type));
+          }
         }
       }
 

src/applications/macro/xaction/PhabricatorMacroNameTransaction.php

@@ -23,7 +23,7 @@ final class PhabricatorMacroNameTransaction
 
   public function getTitleForFeed() {
     return pht(
-      '%s renamed %s macro %s to %s.',
+      '%s renamed %s from %s to %s.',
       $this->renderAuthor(),
       $this->renderObject(),
       $this->renderOldValue(),
@@ -37,6 +37,7 @@ final class PhabricatorMacroNameTransaction
     if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
       $errors[] = $this->newRequiredError(
         pht('Macros must have a name.'));
+      return $errors;
     }
 
     $max_length = $object->getColumnMaximumByteLength('name');

src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php

@@ -4,6 +4,7 @@ final class PhabricatorSubscriptionsEditEngineExtension
   extends PhabricatorEditEngineExtension {
 
   const EXTENSIONKEY = 'subscriptions.subscribers';
+  const FIELDKEY = 'subscriberPHIDs';
 
   const EDITKEY_ADD = 'subscribers.add';
   const EDITKEY_SET = 'subscribers.set';
@@ -42,7 +43,7 @@ final class PhabricatorSubscriptionsEditEngineExtension
     }
 
     $subscribers_field = id(new PhabricatorSubscribersEditField())
-      ->setKey('subscriberPHIDs')
+      ->setKey(self::FIELDKEY)
       ->setLabel(pht('Subscribers'))
       ->setEditTypeKey('subscribers')
       ->setAliases(array('subscriber', 'subscribers'))