From 24b274ded334963cb79f9fa8f595af6a50af55d4 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Sat, 26 Jan 2013 19:56:39 -0800 Subject: [PATCH] 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 --- .../ConpherenceUpdateController.php | 40 +++++-------------- .../conpherence/editor/ConpherenceEditor.php | 36 +++++++++++++++++ .../mail/ConpherenceReplyHandler.php | 37 ++++++++--------- .../PhabricatorMailReplyHandler.php | 8 ++-- 4 files changed, 66 insertions(+), 55 deletions(-) diff --git a/src/applications/conpherence/controller/ConpherenceUpdateController.php b/src/applications/conpherence/controller/ConpherenceUpdateController.php index 4fe4ddcb1e..cf193f4419 100644 --- a/src/applications/conpherence/controller/ConpherenceUpdateController.php +++ b/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(); diff --git a/src/applications/conpherence/editor/ConpherenceEditor.php b/src/applications/conpherence/editor/ConpherenceEditor.php index f993696ed0..9304e0f14e 100644 --- a/src/applications/conpherence/editor/ConpherenceEditor.php +++ b/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(); diff --git a/src/applications/conpherence/mail/ConpherenceReplyHandler.php b/src/applications/conpherence/mail/ConpherenceReplyHandler.php index 60aa0e152e..f7de36cb25 100644 --- a/src/applications/conpherence/mail/ConpherenceReplyHandler.php +++ b/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; } diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index d6b862e23d..51784d7b96 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/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"; }