dev/core#2800 Fix bounce processing to handle verp emails
authorEileen McNaughton <emcnaughton@wikimedia.org>
Thu, 7 Sep 2023 08:08:17 +0000 (20:08 +1200)
committerEileen McNaughton <emcnaughton@wikimedia.org>
Mon, 11 Sep 2023 20:11:15 +0000 (08:11 +1200)
This fixes 2 issues when combining bounce processing with verp emails

1) the emails being processed are being matched to email they come from -
ie the verp email. Hence the created activities are not linked to the
actual contact and instead create endless variants of the sender.
This happens with both the job.process_activities job and with
job.fetch_bounces with is_create_activities = TRUE.
This fix adapts the handling to identify verp emails (using existing regex)
and look up the contact ID from the mailing_event_queue. This is
used as the source (I didn't add a target or assignee at this stage cos
I wasn't sure if that was just data-cruft or useful stuff - but it would
be retrieved from the mailing)

2) this second one is a doozy & probably only showed up in tests because the
tests mostly cover obscure email formats. The create activities code does
a call to get the body and attachments using a function that looks like it
came from a tutorial on how to best use ezcmail. It seems to parse
all the email variants and attachments and, importantly, moves those attachments
to the civicrm files as part of creating the activity.

The code used to getBody() for bounces looks like it was ... written by us.
It does a subset of the processing in earlier call to get body and in
many of our test cases falls back on a blunt generateBody(). This call parses the
email, including the attachments, which are no longer there cos they got moved....
so it fatals.

I dug into the 2 functions and I feel pretty sure the bounce one doesn't add any
additional value but DOES have less capability than the other. So I removed that code,

Unfortunately the person received weird email from Exchange 2003 did not log
the headers https://issues.civicrm.org/jira/browse/CRM-9361 - but even if the
preferred version of the function doesn't parse that there is default text so
it won't fail & it seems like the risk of having a bit less info about an (untested)
obscure email bounce body is not that probematic

CRM/Utils/Mail/EmailProcessor.php
CRM/Utils/Mail/Incoming.php
CRM/Utils/Mail/IncomingMail.php
tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php

index d004a808d6b43a4cc051269e9e46077dddf7ce32..eb4f991f1c644bed30737c6f3c553b2835357a4f 100644 (file)
@@ -77,12 +77,13 @@ class CRM_Utils_Mail_EmailProcessor {
    */
   private static function _process($civiMail, $dao, $is_create_activities) {
     // 0 = activities; 1 = bounce;
-    $usedfor = $dao->is_default;
-    if ($usedfor == 0) {
-      // create an array of all of to, from, cc, bcc that are in use for this Mail Account, so we don't create contacts for emails we aren't adding to the activity
-      $emailFields = array_filter(array_unique(array_merge(explode(",", $dao->activity_targets), explode(",", $dao->activity_assignees), explode(",", $dao->activity_source))));
-      $createContact = !($dao->is_contact_creation_disabled_if_no_match ?? FALSE);
-    }
+    $isBounceProcessing = $dao->is_default;
+    $targetFields = array_filter(explode(',', $dao->activity_targets));
+    $assigneeFields = array_filter(explode(",", $dao->activity_assignees));
+    $sourceFields = array_filter(explode(",", $dao->activity_source));
+    // create an array of all of to, from, cc, bcc that are in use for this Mail Account, so we don't create contacts for emails we aren't adding to the activity.
+    $emailFields = array_merge($targetFields, $assigneeFields, $sourceFields);
+    $createContact = !($dao->is_contact_creation_disabled_if_no_match);
 
     // retrieve the emails
     try {
@@ -104,8 +105,8 @@ class CRM_Utils_Mail_EmailProcessor {
         $queue = $incomingMail->getQueueID();
         $hash = $incomingMail->getHash();
 
-        // preseve backward compatibility
-        if ($usedfor == 0 || $is_create_activities) {
+        // preserve backward compatibility
+        if (!$isBounceProcessing || $is_create_activities) {
           // Mail account may have 'Skip emails which do not have a Case ID
           // or Case hash' option, if its enabled and email is not related
           // to cases - then we need to put email to ignored folder.
@@ -117,34 +118,39 @@ class CRM_Utils_Mail_EmailProcessor {
 
           // if its the activities that needs to be processed ..
           try {
-            $mailParams = CRM_Utils_Mail_Incoming::parseMailingObject($mail, $createContact, FALSE, $emailFields);
+            $mailParams = CRM_Utils_Mail_Incoming::parseMailingObject($mail, $incomingMail->getAttachments(), $createContact, $emailFields);
             $activityParams = [
               'activity_type_id' => (int) $dao->activity_type_id,
               'campaign_id' => $dao->campaign_id ? (int) $dao->campaign_id : NULL,
               'status_id' => $dao->activity_status,
+              'subject' => $incomingMail->getSubject(),
+              'activity_date_time' => $incomingMail->getDate(),
+              'details' => $incomingMail->getBody(),
             ];
+            if ($incomingMail->isVerp()) {
+              $activityParams['source_contact_id'] = $incomingMail->lookup('Queue', 'contact_id');
+            }
+            else {
+              $activityParams['source_contact_id'] = $mailParams[$dao->activity_source][0]['id'];
 
-            $activityParams['source_contact_id'] = $mailParams[$dao->activity_source][0]['id'];
-
-            $activityContacts = ['target_contact_id' => 'activity_targets', 'assignee_contact_id' => 'activity_assignees'];
-            foreach ($activityContacts as $activityContact => $daoName) {
-              $activityParams[$activityContact] = [];
-              $activityKeys = array_filter(explode(",", $dao->$daoName));
-              foreach ($activityKeys as $activityKey) {
-                if (is_array($mailParams[$activityKey])) {
-                  foreach ($mailParams[$activityKey] as $keyValue) {
-                    if (!empty($keyValue['id'])) {
-                      $activityParams[$activityContact][] = $keyValue['id'];
+              $activityContacts = [
+                'target_contact_id' => $targetFields,
+                'assignee_contact_id' => $assigneeFields,
+              ];
+              foreach ($activityContacts as $activityContact => $activityKeys) {
+                $activityParams[$activityContact] = [];
+                foreach ($activityKeys as $activityKey) {
+                  if (is_array($mailParams[$activityKey])) {
+                    foreach ($mailParams[$activityKey] as $keyValue) {
+                      if (!empty($keyValue['id'])) {
+                        $activityParams[$activityContact][] = $keyValue['id'];
+                      }
                     }
                   }
                 }
               }
             }
 
-            $activityParams['subject'] = $mailParams['subject'];
-            $activityParams['activity_date_time'] = $mailParams['date'];
-            $activityParams['details'] = $mailParams['body'];
-
             $numAttachments = Civi::settings()->get('max_attachments_backend') ?? CRM_Core_BAO_File::DEFAULT_MAX_ATTACHMENTS_BACKEND;
             for ($i = 1; $i <= $numAttachments; $i++) {
               if (isset($mailParams["attachFile_$i"])) {
@@ -184,54 +190,13 @@ class CRM_Utils_Mail_EmailProcessor {
 
           switch ($action) {
             case 'b':
-              $text = '';
-              if ($mail->body instanceof ezcMailText) {
-                $text = $mail->body->text;
-              }
-              elseif ($mail->body instanceof ezcMailMultipart) {
-                $text = self::getTextFromMultipart($mail->body);
-              }
-              elseif ($mail->body instanceof ezcMailFile) {
-                $text = file_get_contents($mail->body->__get('fileName'));
-              }
-
-              if (
-                empty($text) &&
-                $mail->subject === 'Delivery Status Notification (Failure)'
-              ) {
-                // Exchange error - CRM-9361
-                foreach ($mail->body->getParts() as $part) {
-                  if ($part instanceof ezcMailDeliveryStatus) {
-                    foreach ($part->recipients as $rec) {
-                      if ($rec['Status'] === '5.1.1') {
-                        if (isset($rec['Description'])) {
-                          $text = $rec['Description'];
-                        }
-                        else {
-                          $text = $rec['Status'] . ' Delivery to the following recipients failed';
-                        }
-                        break;
-                      }
-                    }
-                  }
-                }
-              }
-
-              if (empty($text)) {
-                // If bounce processing fails, just take the raw body. Cf. CRM-11046
-                $text = $mail->generateBody();
-
-                // if text is still empty, lets fudge a blank text so the api call below will succeed
-                if (empty($text)) {
-                  $text = ts('We could not extract the mail body from this bounce message.');
-                }
-              }
+              $text = $incomingMail->getBody();
 
               $activityParams = [
                 'job_id' => $job,
                 'event_queue_id' => $queue,
                 'hash' => $hash,
-                'body' => $text,
+                'body' => $text ?: ts('We could not extract the mail body from this bounce message.'),
                 'version' => 3,
                 // Setting is_transactional means it will rollback if
                 // it crashes part way through creating the bounce.
index 7f27bab6700769e064252e75f3c42183d656fd48..11f0b238090ce8fb0eef3d958ffdc442abb9ff6c 100644 (file)
@@ -287,14 +287,14 @@ class CRM_Utils_Mail_Incoming {
 
   /**
    * @param $mail
-   * @param $createContact
-   * @param $requireContact
+   * @param $attachments
+   * @param bool $createContact
    * @param array $emailFields
    *   Which fields to process and create contacts for of from, to, cc, bcc
    *
    * @return array
    */
-  public static function parseMailingObject(&$mail, $createContact = TRUE, $requireContact = TRUE, $emailFields = ['from', 'to', 'cc', 'bcc']) {
+  public static function parseMailingObject(&$mail, $attachments, $createContact = TRUE, $emailFields = ['from', 'to', 'cc', 'bcc']) {
 
     // Sometimes $mail->from is unset because ezcMail didn't handle format
     // of From header. CRM-19215.
@@ -314,15 +314,6 @@ class CRM_Utils_Mail_Incoming {
       }
       self::parseAddresses($value, $field, $params, $mail, $createContact);
     }
-
-    // define other parameters
-    $params['subject'] = $mail->subject;
-    $params['date'] = date("YmdHi00",
-      strtotime($mail->getHeader("Date"))
-    );
-    $attachments = [];
-    $params['body'] = self::formatMailPart($mail->body, $attachments);
-
     // format and move attachments to the civicrm area
     if (!empty($attachments)) {
       $date = date('YmdHis');
index 4da3997eeedfdb35a589b99a0c8f0acfc8a5fd57..abf9f055c0366005f952c0f2b28cd87b1012fb66 100644 (file)
@@ -9,6 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\API\EntityLookupTrait;
+use Civi\Api4\MailingJob;
+
 /**
  * Incoming mail class.
  *
@@ -19,6 +22,8 @@
  */
 class CRM_Utils_Mail_IncomingMail {
 
+  use EntityLookupTrait;
+
   /**
    * @var \ezcMail
    */
@@ -44,6 +49,27 @@ class CRM_Utils_Mail_IncomingMail {
    */
   private $hash;
 
+  /**
+   * @var string|null
+   */
+  private $body;
+
+  /**
+   * @return string|null
+   */
+  public function getBody(): ?string {
+    return $this->body;
+  }
+
+  /**
+   * @return array
+   */
+  public function getAttachments(): array {
+    return $this->attachments;
+  }
+
+  private $attachments = [];
+
   public function getAction() : ?string {
     return $this->action;
   }
@@ -69,6 +95,17 @@ class CRM_Utils_Mail_IncomingMail {
     return $this->hash;
   }
 
+  /**
+   * @return string
+   */
+  public function getSubject(): string {
+    return (string) $this->mail->subject;
+  }
+
+  public function getDate(): string {
+    return date('YmdHis', strtotime($this->mail->getHeader('Date')));
+  }
+
   /**
    * Is this a verp email.
    *
@@ -90,6 +127,7 @@ class CRM_Utils_Mail_IncomingMail {
    */
   public function __construct(ezcMail $mail, string $emailDomain, string $emailLocalPart) {
     $this->mail = $mail;
+    $this->body = CRM_Utils_Mail_Incoming::formatMailPart($mail->body, $this->attachments);
 
     $verpSeparator = preg_quote(\Civi::settings()->get('verpSeparator') ?: '');
     $emailDomain = preg_quote($emailDomain);
@@ -145,6 +183,26 @@ class CRM_Utils_Mail_IncomingMail {
     if (!$matches && preg_match($regex, ($mail->getHeader('Delivered-To') ?? ''), $matches)) {
       [, $this->action, $this->jobID, $this->queueID, $this->hash] = $matches;
     }
+    if ($this->isVerp()) {
+      $queue = CRM_Mailing_Event_BAO_MailingEventQueue::verify($this->getJobID(), $this->getQueueID(), $this->getHash());
+      if (!$queue) {
+        throw new CRM_Core_Exception('Contact could not be found from civimail response');
+      }
+      $this->define('Queue', 'Queue', [
+        'id' => $queue->id,
+        'hash' => $queue->hash,
+        'contact_id' => $queue->contact_id,
+        'job_id' => $queue->contact_id,
+      ]);
+      $this->define('Mailing', 'Mailing', [
+        'id' => MailingJob::get(FALSE)
+          ->addWhere('id', '=', $this->getJobID())
+          ->addSelect('mailing_id')
+          ->execute()
+          ->first()['mailing_id'],
+      ]);
+    }
+
   }
 
 }
index de3a686dcd30e7998ee9a37df8bfca7111bde0ae..e89393999853c6a4b93d0109579b946c002d7e03 100644 (file)
@@ -113,9 +113,10 @@ class CRM_Utils_Mail_EmailProcessorTest extends CiviUnitTestCase {
     $mail = 'test_sample_message.eml';
 
     copy(__DIR__ . '/data/bounces/' . $mail, __DIR__ . '/data/mail/' . $mail);
-    $this->callAPISuccess('job', 'fetch_bounces', []);
+    $this->callAPISuccess('job', 'fetch_bounces', ['is_create_activities' => TRUE]);
     $this->assertFileDoesNotExist(__DIR__ . '/data/mail/' . $mail);
     $this->checkMailingBounces(1);
+    $this->callAPISuccessGetSingle('Activity', ['source_contact_id' => $this->contactID, 'activity_type_id' => 'Inbound Email']);
   }
 
   /**