Merge pull request #66 from mareksapota-fb/master

Pull request for differential revisions D1001, D1002 and D1004

conf/default.conf.php

@@ -232,6 +232,11 @@ return array(
   // Prefix prepended to mail sent by Differential.
   'metamta.differential.subject-prefix' => '[Differential]',
 
+  // Set this to true if you want patches to be attached to mail from
+  // Differential.  This won't work if you are using SendGrid as your mail
+  // adapter.
+  'metamta.differential.attach-patches' => false,
+
   // By default, Phabricator generates unique reply-to addresses and sends a
   // separate email to each recipient when you enable reply handling. This is
   // more secure than using "From" to establish user identity, but can mean

src/applications/conduit/method/differential/getdiff/ConduitAPI_differential_getdiff_Method.php

@@ -72,60 +72,7 @@ class ConduitAPI_differential_getdiff_Method extends ConduitAPIMethod {
       $changeset->attachHunks($changeset->loadHunks());
     }
 
-    return $this->createDiffDict($diff);
+    return $diff->getDiffDict();
-  }
-
-  public static function createDiffDict(DifferentialDiff $diff) {
-    $dict = array(
-      'id' => $diff->getID(),
-      'parent' => $diff->getParentRevisionID(),
-      'revisionID' => $diff->getRevisionID(),
-      'sourceControlBaseRevision' => $diff->getSourceControlBaseRevision(),
-      'sourceControlPath' => $diff->getSourceControlPath(),
-      'unitStatus' => $diff->getUnitStatus(),
-      'lintStatus' => $diff->getLintStatus(),
-      'changes' => array(),
-      'properties' => array(),
-    );
-
-    foreach ($diff->getChangesets() as $changeset) {
-      $hunks = array();
-      foreach ($changeset->getHunks() as $hunk) {
-        $hunks[] = array(
-          'oldOffset' => $hunk->getOldOffset(),
-          'newOffset' => $hunk->getNewOffset(),
-          'oldLength' => $hunk->getOldLen(),
-          'newLength' => $hunk->getNewLen(),
-          'addLines'  => null,
-          'delLines'  => null,
-          'isMissingOldNewline' => null,
-          'isMissingNewNewline' => null,
-          'corpus'    => $hunk->getChanges(),
-        );
-      }
-      $change = array(
-        'metadata'      => $changeset->getMetadata(),
-        'oldPath'       => $changeset->getOldFile(),
-        'currentPath'   => $changeset->getFileName(),
-        'awayPaths'     => $changeset->getAwayPaths(),
-        'oldProperties' => $changeset->getOldProperties(),
-        'newProperties' => $changeset->getNewProperties(),
-        'type'          => $changeset->getChangeType(),
-        'fileType'      => $changeset->getFileType(),
-        'commitHash'    => null,
-        'hunks'         => $hunks,
-      );
-      $dict['changes'][] = $change;
-    }
-
-    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
-      'diffID = %d',
-      $diff->getID());
-    foreach ($properties as $property) {
-      $dict['properties'][$property->getName()] = $property->getData();
-    }
-
-    return $dict;
   }
 
 }

src/applications/conduit/method/differential/getdiff/__init__.php

@@ -9,7 +9,6 @@
 phutil_require_module('phabricator', 'applications/conduit/method/base');
 phutil_require_module('phabricator', 'applications/conduit/protocol/exception');
 phutil_require_module('phabricator', 'applications/differential/storage/diff');
-phutil_require_module('phabricator', 'applications/differential/storage/diffproperty');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
 
 phutil_require_module('phutil', 'utils');

src/applications/conduit/method/differential/getrevision/ConduitAPI_differential_getrevision_Method.php

@@ -62,8 +62,7 @@ class ConduitAPI_differential_getrevision_Method extends ConduitAPIMethod {
       foreach ($diff->getChangesets() as $changeset) {
         $changeset->attachHunks($changeset->loadHunks());
       }
-      $diff_dicts[] =
+      $diff_dicts[] = $diff->getDiffDict();
-        ConduitAPI_differential_getdiff_Method::createDiffDict($diff);
     }
 
     $commit_dicts = array();

src/applications/conduit/method/differential/getrevision/__init__.php

@@ -7,7 +7,6 @@
 
 
 phutil_require_module('phabricator', 'applications/conduit/method/base');
-phutil_require_module('phabricator', 'applications/conduit/method/differential/getdiff');
 phutil_require_module('phabricator', 'applications/conduit/protocol/exception');
 phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/differential/field/selector/base');

src/applications/differential/mail/base/DifferentialMail.php

@@ -70,9 +70,10 @@ abstract class DifferentialMail {
       throw new Exception('No "To:" users provided!');
     }
 
-    $cc_phids = $this->getCCPHIDs();
+    $cc_phids    = $this->getCCPHIDs();
-    $subject  = $this->buildSubject();
+    $subject     = $this->buildSubject();
-    $body     = $this->buildBody();
+    $body        = $this->buildBody();
+    $attachments = $this->buildAttachments();
 
     $template = new PhabricatorMetaMTAMail();
     $actor_handle = $this->getActorHandle();
@@ -89,6 +90,14 @@ abstract class DifferentialMail {
       ->setParentMessageID($this->parentMessageID)
       ->addHeader('Thread-Topic', $this->getRevision()->getTitle());
 
+    foreach ($attachments as $attachment) {
+      $template->addAttachment(
+        $attachment['data'],
+        $attachment['filename'],
+        $attachment['mimetype']
+      );
+    }
+
     $template->setThreadID(
       $this->getThreadID(),
       $this->isFirstMailAboutRevision());
@@ -165,6 +174,21 @@ EOTEXT;
     return $body;
   }
 
+  /**
+   * You can override this method in a subclass and return array of attachments
+   * to be sent with the email.  Each attachment is a dictionary with 'data',
+   * 'filename' and 'mimetype' keys.  For example:
+   *
+   *   array(
+   *     'data' => 'some text',
+   *     'filename' => 'example.txt',
+   *     'mimetype' => 'text/plain'
+   *   );
+   */
+  protected function buildAttachments() {
+    return array();
+  }
+
   public function getReplyHandler() {
     if ($this->replyHandler) {
       return $this->replyHandler;

src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php

@@ -77,4 +77,39 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
 
     return implode("\n", $body);
   }
+
+  protected function buildAttachments() {
+    $attachments = array();
+    if (PhabricatorEnv::getEnvConfig('metamta.differential.attach-patches')) {
+      $revision = $this->getRevision();
+      $revision_id = $revision->getID();
+
+      $diffs = $revision->loadDiffs();
+      $diff_number = count($diffs);
+      $diff = array_pop($diffs);
+
+      $filename = "D{$revision_id}.{$diff_number}.diff";
+
+      $diff->attachChangesets($diff->loadChangesets());
+      // TODO: We could batch this to improve performance.
+      foreach ($diff->getChangesets() as $changeset) {
+        $changeset->attachHunks($changeset->loadHunks());
+      }
+      $diff_dict = $diff->getDiffDict();
+
+      $changes = array();
+      foreach ($diff_dict['changes'] as $changedict) {
+        $changes[] = ArcanistDiffChange::newFromDictionary($changedict);
+      }
+      $bundle = ArcanistBundle::newFromChanges($changes);
+      $unified_diff = $bundle->toUnifiedDiff();
+
+      $attachments[] = array(
+        'data' => $unified_diff,
+        'filename' => $filename,
+        'mimetype' => 'text/x-diff; charset=utf-8'
+      );
+    }
+    return $attachments;
+  }
 }

src/applications/differential/mail/reviewrequest/__init__.php

@@ -6,8 +6,12 @@
 
 
 
+phutil_require_module('arcanist', 'parser/bundle');
+phutil_require_module('arcanist', 'parser/diff/change');
+
 phutil_require_module('phabricator', 'applications/differential/mail/base');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
+phutil_require_module('phabricator', 'infrastructure/env');
 
 phutil_require_module('phutil', 'utils');
 

src/applications/differential/storage/diff/DifferentialDiff.php

@@ -146,4 +146,56 @@ class DifferentialDiff extends DifferentialDAO {
     return $diff;
   }
 
+  public function getDiffDict() {
+    $dict = array(
+      'id' => $this->getID(),
+      'parent' => $this->getParentRevisionID(),
+      'revisionID' => $this->getRevisionID(),
+      'sourceControlBaseRevision' => $this->getSourceControlBaseRevision(),
+      'sourceControlPath' => $this->getSourceControlPath(),
+      'unitStatus' => $this->getUnitStatus(),
+      'lintStatus' => $this->getLintStatus(),
+      'changes' => array(),
+      'properties' => array(),
+    );
+
+    foreach ($this->getChangesets() as $changeset) {
+      $hunks = array();
+      foreach ($changeset->getHunks() as $hunk) {
+        $hunks[] = array(
+          'oldOffset' => $hunk->getOldOffset(),
+          'newOffset' => $hunk->getNewOffset(),
+          'oldLength' => $hunk->getOldLen(),
+          'newLength' => $hunk->getNewLen(),
+          'addLines'  => null,
+          'delLines'  => null,
+          'isMissingOldNewline' => null,
+          'isMissingNewNewline' => null,
+          'corpus'    => $hunk->getChanges(),
+        );
+      }
+      $change = array(
+        'metadata'      => $changeset->getMetadata(),
+        'oldPath'       => $changeset->getOldFile(),
+        'currentPath'   => $changeset->getFileName(),
+        'awayPaths'     => $changeset->getAwayPaths(),
+        'oldProperties' => $changeset->getOldProperties(),
+        'newProperties' => $changeset->getNewProperties(),
+        'type'          => $changeset->getChangeType(),
+        'fileType'      => $changeset->getFileType(),
+        'commitHash'    => null,
+        'hunks'         => $hunks,
+      );
+      $dict['changes'][] = $change;
+    }
+
+    $properties = id(new DifferentialDiffProperty())->loadAllWhere(
+      'diffID = %d',
+      $this->getID());
+    foreach ($properties as $property) {
+      $dict['properties'][$property->getName()] = $property->getData();
+    }
+
+    return $dict;
+  }
 }

src/applications/differential/storage/diff/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/differential/storage/base');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
+phutil_require_module('phabricator', 'applications/differential/storage/diffproperty');
 phutil_require_module('phabricator', 'applications/differential/storage/hunk');
 phutil_require_module('phabricator', 'applications/repository/storage/arcanistproject');
 

src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php

@@ -22,6 +22,7 @@ abstract class PhabricatorMailImplementationAdapter {
   abstract public function addReplyTo($email, $name = '');
   abstract public function addTos(array $emails);
   abstract public function addCCs(array $emails);
+  abstract public function addAttachment($data, $filename, $mimetype);
   abstract public function addHeader($header_name, $header_value);
   abstract public function setBody($body);
   abstract public function setSubject($subject);

src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php

@@ -55,6 +55,16 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter
     return $this;
   }
 
+  public function addAttachment($data, $filename, $mimetype) {
+    $this->mailer->AddStringAttachment(
+      $data,
+      $filename,
+      'base64',
+      $mimetype
+    );
+    return $this;
+  }
+
   public function addHeader($header_name, $header_value) {
     if (strtolower($header_name) == 'message-id') {
       $this->mailer->MessageID = $header_value;
@@ -75,7 +85,7 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter
   }
 
   public function setIsHTML($is_html) {
-    $this->mailer->IsHTML(true);
+    $this->mailer->IsHTML($is_html);
     return $this;
   }
 

src/applications/metamta/adapter/sendgrid/PhabricatorMailImplementationSendGridAdapter.php

@@ -55,6 +55,12 @@ class PhabricatorMailImplementationSendGridAdapter
     return $this;
   }
 
+  public function addAttachment($data, $filename, $mimetype) {
+    throw new Exception(
+      'SendGrid adapter does not currently support attachments.'
+    );
+  }
+
   public function addHeader($header_name, $header_value) {
     $this->params['headers'][] = array($header_name, $header_value);
     return $this;

src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php

@@ -61,6 +61,15 @@ class PhabricatorMailImplementationTestAdapter
     return $this;
   }
 
+  public function addAttachment($data, $filename, $mimetype) {
+    $this->guts['attachments'][] = array(
+      'data' => $data,
+      'filename' => $filename,
+      'mimetype' => $mimetype
+    );
+    return $this;
+  }
+
   public function addHeader($header_name, $header_value) {
     $this->guts['headers'][] = array($header_name, $header_value);
     return $this;

src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php

@@ -104,6 +104,15 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this;
   }
 
+  public function addAttachment($data, $filename, $mimetype) {
+    $this->parameters['attachments'][] = array(
+      'data' => $data,
+      'filename' => $filename,
+      'mimetype' => $mimetype
+    );
+    return $this;
+  }
+
   public function setFrom($from) {
     $this->setParam('from', $from);
     return $this;
@@ -284,6 +293,15 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
               $mailer->addHeader($header_key, $header_value);
             }
             break;
+          case 'attachments':
+            foreach ($value as $attachment) {
+              $mailer->addAttachment(
+                $attachment['data'],
+                $attachment['filename'],
+                $attachment['mimetype']
+              );
+            }
+            break;
           case 'body':
             $mailer->setBody($value);
             break;