Conpherence - make attaching files work better

Summary: unifies the code and presentation between adding files via email and web. Also makes it possible to "attach" the same file multiple times, either by just talking about it in the different messages or multiple times in the same message.

Test Plan: sent message with attachment - it worked! sent a message referencing previous attachment - it work! sent a message with the same attachment in it like 12 times - it worked!

Reviewers: epriestley, chad

Reviewed By: chad

CC: aran, Korvin

Maniphest Tasks: T2399

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

src/applications/conpherence/controller/ConpherenceUpdateController.php

@@ -43,41 +43,21 @@ final class ConpherenceUpdateController extends
         array(
           'ip' => $request->getRemoteAddr()
         ));
+      $editor = id(new ConpherenceEditor())
+        ->setContentSource($content_source)
+        ->setActor($user);
 
       $action = $request->getStr('action');
       switch ($action) {
         case 'message':
           $message = $request->getStr('text');
-          $files = array();
-          $file_phids =
-            PhabricatorMarkupEngine::extractFilePHIDsFromEmbeddedFiles(
-              array($message)
-            );
-          if ($file_phids) {
-            $files = id(new PhabricatorFileQuery())
-              ->setViewer($user)
-              ->withPHIDs($file_phids)
-              ->execute();
-          }
-          $xactions = array();
-          if ($files) {
-            $xactions[] = id(new ConpherenceTransaction())
-              ->setTransactionType(ConpherenceTransactionType::TYPE_FILES)
-              ->setNewValue(array('+' => mpull($files, 'getPHID')));
-          }
-          $xactions[] = id(new ConpherenceTransaction())
-            ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-            ->attachComment(
-              id(new ConpherenceTransactionComment())
-              ->setContent($message)
-              ->setConpherencePHID($conpherence->getPHID())
-            );
+          $xactions = $editor->generateTransactionsFromText(
+            $conpherence,
+            $message
+          );
           $time = time();
           $conpherence->openTransaction();
-          $xactions = id(new ConpherenceEditor())
-            ->setContentSource($content_source)
-            ->setActor($user)
-            ->applyTransactions($conpherence, $xactions);
+          $xactions = $editor->applyTransactions($conpherence, $xactions);
           $last_xaction = end($xactions);
           $xaction_phid = $last_xaction->getPHID();
           $behind = ConpherenceParticipationStatus::BEHIND;
@@ -135,9 +115,7 @@ final class ConpherenceUpdateController extends
 
           if ($xactions) {
             $conpherence->openTransaction();
-            $xactions = id(new ConpherenceEditor())
-              ->setContentSource($content_source)
-              ->setActor($user)
+            $xactions = $editor
               ->setContinueOnNoEffect(true)
               ->applyTransactions($conpherence, $xactions);
             $updated = $conpherence->saveTransaction();

src/applications/conpherence/editor/ConpherenceEditor.php

@@ -5,6 +5,42 @@
  */
 final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
 
+  public function generateTransactionsFromText(
+    ConpherenceThread $conpherence,
+    $text) {
+
+    $files = array();
+    $file_phids =
+      PhabricatorMarkupEngine::extractFilePHIDsFromEmbeddedFiles(
+        array($text)
+      );
+    // Since these are extracted from text, we might be re-including the
+    // same file -- e.g. a mock under discussion. Filter files we
+    // already have.
+    $existing_file_phids = $conpherence->getFilePHIDs();
+    $file_phids = array_diff($file_phids, $existing_file_phids);
+    if ($file_phids) {
+      $files = id(new PhabricatorFileQuery())
+        ->setViewer($this->getActor())
+        ->withPHIDs($file_phids)
+        ->execute();
+    }
+    $xactions = array();
+    if ($files) {
+      $xactions[] = id(new ConpherenceTransaction())
+        ->setTransactionType(ConpherenceTransactionType::TYPE_FILES)
+        ->setNewValue(array('+' => mpull($files, 'getPHID')));
+    }
+    $xactions[] = id(new ConpherenceTransaction())
+      ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
+      ->attachComment(
+        id(new ConpherenceTransactionComment())
+        ->setContent($text)
+        ->setConpherencePHID($conpherence->getPHID())
+      );
+    return $xactions;
+  }
+
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
 

src/applications/conpherence/mail/ConpherenceReplyHandler.php

@@ -48,25 +48,6 @@ final class ConpherenceReplyHandler extends PhabricatorMailReplyHandler {
       $conpherence->attachParticipants($participants);
     }
 
-    $body = $mail->getCleanTextBody();
-    $body = trim($body);
-    $file_phids = $mail->getAttachments();
-    $body = $this->enhanceBodyWithAttachments($body, $file_phids);
-
-    $xactions = array();
-    if ($file_phids) {
-      $xactions[] = id(new ConpherenceTransaction())
-        ->setTransactionType(ConpherenceTransactionType::TYPE_FILES)
-        ->setNewValue(array('+' => $file_phids));
-    }
-    $xactions[] = id(new ConpherenceTransaction())
-      ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-      ->attachComment(
-        id(new ConpherenceTransactionComment())
-        ->setContent($body)
-        ->setConpherencePHID($conpherence->getPHID())
-      );
-
     $content_source = PhabricatorContentSource::newForSource(
       PhabricatorContentSource::SOURCE_EMAIL,
       array(
@@ -76,8 +57,22 @@ final class ConpherenceReplyHandler extends PhabricatorMailReplyHandler {
     $editor = id(new ConpherenceEditor())
       ->setActor($user)
       ->setContentSource($content_source)
-      ->setParentMessageID($mail->getMessageID())
-      ->applyTransactions($conpherence, $xactions);
+      ->setParentMessageID($mail->getMessageID());
+
+    $body = $mail->getCleanTextBody();
+    $body = trim($body);
+    $file_phids = $mail->getAttachments();
+    $body = $this->enhanceBodyWithAttachments(
+      $body,
+      $file_phids,
+      '{F%d}'
+    );
+    $xactions = $editor->generateTransactionsFromText(
+      $conpherence,
+      $body
+    );
+
+    $editor->applyTransactions($conpherence, $xactions);
 
     return null;
   }

src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php

@@ -295,8 +295,10 @@ EOBODY;
     return $this->getSingleReplyHandlerPrefix($address);
   }
 
-  final protected function enhanceBodyWithAttachments($body,
-                                                      array $attachments) {
+  final protected function enhanceBodyWithAttachments(
+    $body,
+    array $attachments,
+    $format = '- {F%d, layout=link}') {
     if (!$attachments) {
       return $body;
     }
@@ -310,7 +312,7 @@ EOBODY;
     }
 
     foreach ($files as $file) {
-      $file_str = sprintf('- {F%d, layout=link}', $file->getID());
+      $file_str = sprintf($format, $file->getID());
       $body .= $file_str."\n";
     }