Migrate Project image to modular transactions

Summary: I'm not sure you can actually remove a project's image (maybe via the API?), but I kept the code for rendering the relevant title/feed anyway.

Test Plan: Unit tests + adding/changing project pictures.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

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

src/__phutil_library_map__.php

@@ -3621,6 +3621,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectHovercardEngineExtension' => 'applications/project/engineextension/PhabricatorProjectHovercardEngineExtension.php',
     'PhabricatorProjectIconSet' => 'applications/project/icon/PhabricatorProjectIconSet.php',
     'PhabricatorProjectIconsConfigOptionType' => 'applications/project/config/PhabricatorProjectIconsConfigOptionType.php',
+    'PhabricatorProjectImageTransaction' => 'applications/project/xaction/PhabricatorProjectImageTransaction.php',
     'PhabricatorProjectInterface' => 'applications/project/interface/PhabricatorProjectInterface.php',
     'PhabricatorProjectListController' => 'applications/project/controller/PhabricatorProjectListController.php',
     'PhabricatorProjectListView' => 'applications/project/view/PhabricatorProjectListView.php',
@@ -9035,6 +9036,7 @@ phutil_register_library_map(array(
     'PhabricatorProjectHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension',
     'PhabricatorProjectIconSet' => 'PhabricatorIconSet',
     'PhabricatorProjectIconsConfigOptionType' => 'PhabricatorConfigJSONOptionType',
+    'PhabricatorProjectImageTransaction' => 'PhabricatorProjectTransactionType',
     'PhabricatorProjectListController' => 'PhabricatorProjectController',
     'PhabricatorProjectListView' => 'AphrontView',
     'PhabricatorProjectLockController' => 'PhabricatorProjectController',

src/applications/files/controller/PhabricatorFileComposeController.php

@@ -48,7 +48,8 @@ final class PhabricatorFileComposeController
 
         $xactions = array();
         $xactions[] = id(new PhabricatorProjectTransaction())
-          ->setTransactionType(PhabricatorProjectTransaction::TYPE_IMAGE)
+          ->setTransactionType(
+              PhabricatorProjectImageTransaction::TRANSACTIONTYPE)
           ->setNewValue($file->getPHID());
 
         $editor = id(new PhabricatorProjectTransactionEditor())

src/applications/project/controller/PhabricatorProjectEditPictureController.php

@@ -78,7 +78,8 @@ final class PhabricatorProjectEditPictureController
 
         $xactions = array();
         $xactions[] = id(new PhabricatorProjectTransaction())
-          ->setTransactionType(PhabricatorProjectTransaction::TYPE_IMAGE)
+          ->setTransactionType(
+              PhabricatorProjectImageTransaction::TRANSACTIONTYPE)
           ->setNewValue($new_value);
 
         $editor = id(new PhabricatorProjectTransactionEditor())

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -30,7 +30,6 @@ final class PhabricatorProjectTransactionEditor
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
     $types[] = PhabricatorTransactions::TYPE_JOIN_POLICY;
 
-    $types[] = PhabricatorProjectTransaction::TYPE_IMAGE;
     $types[] = PhabricatorProjectTransaction::TYPE_ICON;
     $types[] = PhabricatorProjectTransaction::TYPE_COLOR;
     $types[] = PhabricatorProjectTransaction::TYPE_LOCKED;
@@ -49,8 +48,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
-        return $object->getProfileImagePHID();
       case PhabricatorProjectTransaction::TYPE_ICON:
         return $object->getIcon();
       case PhabricatorProjectTransaction::TYPE_COLOR:
@@ -78,7 +75,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
       case PhabricatorProjectTransaction::TYPE_ICON:
       case PhabricatorProjectTransaction::TYPE_COLOR:
       case PhabricatorProjectTransaction::TYPE_LOCKED:
@@ -105,9 +101,6 @@ final class PhabricatorProjectTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
-        $object->setProfileImagePHID($xaction->getNewValue());
-        return;
       case PhabricatorProjectTransaction::TYPE_ICON:
         $object->setIcon($xaction->getNewValue());
         return;
@@ -150,7 +143,6 @@ final class PhabricatorProjectTransactionEditor
     $new = $xaction->getNewValue();
 
     switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
       case PhabricatorProjectTransaction::TYPE_ICON:
       case PhabricatorProjectTransaction::TYPE_COLOR:
       case PhabricatorProjectTransaction::TYPE_LOCKED:
@@ -336,7 +328,7 @@ final class PhabricatorProjectTransactionEditor
     switch ($xaction->getTransactionType()) {
       case PhabricatorProjectNameTransaction::TRANSACTIONTYPE:
       case PhabricatorProjectStatusTransaction::TRANSACTIONTYPE:
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
+      case PhabricatorProjectImageTransaction::TRANSACTIONTYPE:
       case PhabricatorProjectTransaction::TYPE_ICON:
       case PhabricatorProjectTransaction::TYPE_COLOR:
         PhabricatorPolicyFilter::requireCapability(
@@ -475,22 +467,6 @@ final class PhabricatorProjectTransactionEditor
     return true;
   }
 
-  protected function extractFilePHIDsFromCustomTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorProjectTransaction::TYPE_IMAGE:
-        $new = $xaction->getNewValue();
-        if ($new) {
-          return array($new);
-        }
-        break;
-    }
-
-    return parent::extractFilePHIDsFromCustomTransaction($object, $xaction);
-  }
-
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {

src/applications/project/storage/PhabricatorProjectTransaction.php

@@ -3,7 +3,6 @@
 final class PhabricatorProjectTransaction
   extends PhabricatorModularTransaction {
 
-  const TYPE_IMAGE      = 'project:image';
   const TYPE_ICON       = 'project:icon';
   const TYPE_COLOR      = 'project:color';
   const TYPE_LOCKED     = 'project:locked';
@@ -45,10 +44,6 @@ final class PhabricatorProjectTransaction
         $rem = array_diff($old, $new);
         $req_phids = array_merge($add, $rem);
         break;
-      case self::TYPE_IMAGE:
-        $req_phids[] = $old;
-        $req_phids[] = $new;
-        break;
     }
 
     return array_merge($req_phids, parent::getRequiredHandlePHIDs());
@@ -106,8 +101,6 @@ final class PhabricatorProjectTransaction
         }
       case self::TYPE_ICON:
         return PhabricatorProjectIconSet::getIconIcon($new);
-      case self::TYPE_IMAGE:
-        return 'fa-photo';
       case self::TYPE_MEMBERS:
         return 'fa-user';
     }
@@ -126,26 +119,6 @@ final class PhabricatorProjectTransaction
           '%s created this project.',
           $this->renderHandleLink($author_phid));
 
-      case self::TYPE_IMAGE:
-        // TODO: Some day, it would be nice to show the images.
-        if (!$old) {
-          return pht(
-            "%s set this project's image to %s.",
-            $author_handle,
-            $this->renderHandleLink($new));
-        } else if (!$new) {
-          return pht(
-            "%s removed this project's image.",
-            $author_handle);
-        } else {
-          return pht(
-            "%s updated this project's image from %s to %s.",
-            $author_handle,
-            $this->renderHandleLink($old),
-            $this->renderHandleLink($new));
-        }
-        break;
-
       case self::TYPE_ICON:
         $set = new PhabricatorProjectIconSet();
 
@@ -253,27 +226,6 @@ final class PhabricatorProjectTransaction
     $new = $this->getNewValue();
 
     switch ($this->getTransactionType()) {
-      case self::TYPE_IMAGE:
-        // TODO: Some day, it would be nice to show the images.
-        if (!$old) {
-          return pht(
-            '%s set the image for %s to %s.',
-            $author_handle,
-            $object_handle,
-            $this->renderHandleLink($new));
-        } else if (!$new) {
-          return pht(
-            '%s removed the image for %s.',
-            $author_handle,
-            $object_handle);
-        } else {
-          return pht(
-            '%s updated the image for %s from %s to %s.',
-            $author_handle,
-            $object_handle,
-            $this->renderHandleLink($old),
-            $this->renderHandleLink($new));
-        }
 
       case self::TYPE_ICON:
         $set = new PhabricatorProjectIconSet();
@@ -313,7 +265,7 @@ final class PhabricatorProjectTransaction
     switch ($this->getTransactionType()) {
       case PhabricatorProjectNameTransaction::TRANSACTIONTYPE:
       case PhabricatorProjectSlugsTransaction::TRANSACTIONTYPE:
-      case self::TYPE_IMAGE:
+      case PhabricatorProjectImageTransaction::TRANSACTIONTYPE:
       case self::TYPE_ICON:
       case self::TYPE_COLOR:
         $tags[] = self::MAILTAG_METADATA;

src/applications/project/xaction/PhabricatorProjectImageTransaction.php

@@ -0,0 +1,136 @@
+<?php
+
+final class PhabricatorProjectImageTransaction
+  extends PhabricatorProjectTransactionType {
+
+  const TRANSACTIONTYPE = 'project:image';
+
+  public function generateOldValue($object) {
+    return $object->getProfileImagePHID();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setProfileImagePHID($value);
+  }
+
+  public function applyExternalEffects($object, $value) {
+    $old = $this->getOldValue();
+    $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() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    // TODO: Some day, it would be nice to show the images.
+    if (!$old) {
+      return pht(
+        "%s set this project's image to %s.",
+        $this->renderAuthor(),
+        $this->renderNewHandle());
+    } else if (!$new) {
+      return pht(
+        "%s removed this project's image.",
+        $this->renderAuthor());
+    } else {
+      return pht(
+        "%s updated this project's image from %s to %s.",
+        $this->renderAuthor(),
+        $this->renderOldHandle(),
+        $this->renderNewHandle());
+    }
+  }
+
+  public function getTitleForFeed() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    // TODO: Some day, it would be nice to show the images.
+    if (!$old) {
+      return pht(
+        '%s set the image for %s to %s.',
+        $this->renderAuthor(),
+        $this->renderObject(),
+        $this->renderNewHandle());
+    } else if (!$new) {
+      return pht(
+        '%s removed the image for %s.',
+        $this->renderAuthor(),
+        $this->renderObject());
+    } else {
+      return pht(
+        '%s updated the image for %s from %s to %s.',
+        $this->renderAuthor(),
+        $this->renderObject(),
+        $this->renderOldHandle(),
+        $this->renderNewHandle());
+    }
+  }
+
+  public function getIcon() {
+    return 'fa-photo';
+  }
+
+  public function extractFilePHIDs($object, $value) {
+    if ($value) {
+      return array($value);
+    }
+    return array();
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+    $viewer = $this->getActor();
+
+    foreach ($xactions as $xaction) {
+      $file_phid = $xaction->getNewValue();
+
+      // Only validate if file was uploaded
+      if ($file_phid) {
+        $file = id(new PhabricatorFileQuery())
+          ->setViewer($viewer)
+          ->withPHIDs(array($file_phid))
+          ->executeOne();
+
+        if (!$file) {
+          $errors[] = $this->newInvalidError(
+            pht('"%s" is not a valid file PHID.',
+            $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));
+          }
+        }
+      }
+    }
+
+    return $errors;
+  }
+
+}

src/applications/project/xaction/PhabricatorProjectSlugsTransaction.php

@@ -61,7 +61,6 @@ final class PhabricatorProjectSlugsTransaction
           count($rem),
           $this->renderSlugList($rem));
     }
-    break;
   }
 
   public function getTitleForFeed() {