File names should be editable.

Summary: Fixes T7480, File names should be editable and the event should show up in feed.

Test Plan: Upload a file, view file details, edit file, change file name by adding a space and a word to the name, save changes, file name should retain space and not normalize the name, file details should show the edit event, install feed should correctly show an event for the action.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Maniphest Tasks: T7480

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

src/applications/files/controller/PhabricatorFileEditController.php

@@ -26,11 +26,17 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
     }
 
     $title = pht('Edit %s', $file->getName());
+    $file_name = $file->getName();
     $view_uri = '/'.$file->getMonogram();
-
+    $error_name = true;
     $validation_exception = null;
+
     if ($request->isFormPost()) {
       $can_view = $request->getStr('canView');
+      $file_name = $request->getStr('name');
+      $errors = array();
+
+      $type_name = PhabricatorFileTransaction::TYPE_NAME;
 
       $xactions = array();
 
@@ -38,6 +44,10 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
         ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
         ->setNewValue($can_view);
 
+      $xactions[] = id(new PhabricatorFileTransaction())
+        ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME)
+        ->setNewValue($file_name);
+
       $editor = id(new PhabricatorFileEditor())
         ->setActor($viewer)
         ->setContentSourceFromRequest($request)
@@ -48,6 +58,7 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
         return id(new AphrontRedirectResponse())->setURI($view_uri);
       } catch (PhabricatorApplicationTransactionValidationException $ex) {
         $validation_exception = $ex;
+        $error_name = $ex->getShortMessage($type_name);
 
         $file->setViewPolicy($can_view);
       }
@@ -61,6 +72,12 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
 
     $form = id(new AphrontFormView())
       ->setUser($viewer)
+       ->appendChild(
+        id(new AphrontFormTextControl())
+        ->setName('name')
+        ->setValue($file_name)
+        ->setLabel(pht('Name'))
+        ->setError($error_name))
       ->appendChild(
         id(new AphrontFormPolicyControl())
           ->setUser($viewer)

src/applications/files/editor/PhabricatorFileEditor.php

@@ -17,16 +17,30 @@ final class PhabricatorFileEditor
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
     $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
 
+    $types[] = PhabricatorFileTransaction::TYPE_NAME;
+
     return $types;
   }
 
   protected function getCustomTransactionOldValue(
     PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {}
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorFileTransaction::TYPE_NAME:
+        return $object->getName();
+    }
+  }
 
   protected function getCustomTransactionNewValue(
     PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {}
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorFileTransaction::TYPE_NAME:
+        return $xaction->getNewValue();
+    }
+  }
 
   protected function applyCustomInternalTransaction(
     PhabricatorLiskDAO $object,
@@ -36,6 +50,9 @@ final class PhabricatorFileEditor
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
         $object->setViewPolicy($xaction->getNewValue());
         break;
+      case PhabricatorFileTransaction::TYPE_NAME:
+        $object->setName($xaction->getNewValue());
+        break;
     }
   }
 
@@ -97,4 +114,34 @@ final class PhabricatorFileEditor
     return false;
   }
 
+  protected function validateTransaction(
+    PhabricatorLiskDAO $object,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateTransaction($object, $type, $xactions);
+
+    switch ($type) {
+      case PhabricatorFileTransaction::TYPE_NAME:
+        $missing = $this->validateIsEmptyTextField(
+          $object->getName(),
+          $xactions);
+
+        if ($missing) {
+          $error = new PhabricatorApplicationTransactionValidationError(
+            $type,
+            pht('Required'),
+            pht('File name is required.'),
+            nonempty(last($xactions), null));
+
+          $error->setIsMissingFieldError(true);
+          $errors[] = $error;
+        }
+        break;
+    }
+
+    return $errors;
+  }
+
+
 }

src/applications/files/storage/PhabricatorFile.php

@@ -209,7 +209,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
     $file = id(new PhabricatorFile())->loadOneWhere(
       'name = %s AND contentHash = %s LIMIT 1',
-      self::normalizeFileName(idx($params, 'name')),
+      idx($params, 'name'),
       self::hashFileContent($data));
 
     if (!$file) {
@@ -692,7 +692,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
   private function getCDNURI($token) {
-    $name = phutil_escape_uri($this->getName());
+    $name = self::normalizeFileName($this->getName());
+    $name = phutil_escape_uri($name);
 
     $parts = array();
     $parts[] = 'file';
@@ -1210,7 +1211,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
    */
   private function readPropertiesFromParameters(array $params) {
     $file_name = idx($params, 'name');
-    $file_name = self::normalizeFileName($file_name);
     $this->setName($file_name);
 
     $author_phid = idx($params, 'authorPHID');

src/applications/files/storage/PhabricatorFileTransaction.php

@@ -3,6 +3,8 @@
 final class PhabricatorFileTransaction
   extends PhabricatorApplicationTransaction {
 
+  const TYPE_NAME  = 'file:name';
+
   public function getApplicationName() {
     return 'file';
   }
@@ -15,4 +17,69 @@ final class PhabricatorFileTransaction
     return new PhabricatorFileTransactionComment();
   }
 
+  public function getTitle() {
+    $author_phid = $this->getAuthorPHID();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_NAME:
+        return pht(
+          '%s updated the name for this file from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $old,
+          $new);
+        break;
+    }
+
+    return parent::getTitle();
+  }
+
+  public function getTitleForFeed() {
+    $author_phid = $this->getAuthorPHID();
+    $object_phid = $this->getObjectPHID();
+
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $type = $this->getTransactionType();
+    switch ($type) {
+      case self::TYPE_NAME:
+        return pht(
+          '%s updated the name of %s from "%s" to "%s".',
+          $this->renderHandleLink($author_phid),
+          $this->renderHandleLink($object_phid),
+          $old,
+          $new);
+        break;
+      }
+
+    return parent::getTitleForFeed();
+  }
+
+  public function getIcon() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_NAME:
+        return 'fa-pencil';
+    }
+
+    return parent::getIcon();
+  }
+
+
+  public function getColor() {
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    switch ($this->getTransactionType()) {
+      case self::TYPE_NAME:
+        return PhabricatorTransactions::COLOR_BLUE;
+    }
+
+    return parent::getColor();
+  }
 }