Add subscriber errors passing via MailerError object

We want to process errors for individual subscribers.
Subscriber errors were inlined into error message string.
This commit changes this so that we are now able to get
subscriber errors as a data which are easy to process further.

[MAILPOET-1154]

lib/Cron/Workers/SendingQueue/SendingQueue.php

@@ -168,9 +168,9 @@ class SendingQueue {
       $error = $send_result['error'];
       assert($error instanceof MailerError);
       if($error->getRetryInterval() !== null) {
-        MailerLog::processNonBlockingError($error->getOperation(), $error->getMessage(), $error->getRetryInterval());
+        MailerLog::processNonBlockingError($error->getOperation(), $error->getMessageWithFailedSubscribers(), $error->getRetryInterval());
       } else {
-        MailerLog::processError($error->getOperation(), $error->getMessage());
+        MailerLog::processError($error->getOperation(), $error->getMessageWithFailedSubscribers());
       }
     }
     // update processed/to process list

lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php

@@ -53,7 +53,7 @@ class Newsletter {
     return $newsletter;
   }
 
-  function preProcessNewsletter($newsletter, $queue) {
+  function preProcessNewsletter(\MailPoet\Models\Newsletter $newsletter, $queue) {
     // return the newsletter if it was previously rendered
     if(!is_null($queue->getNewsletterRenderedBody())) {
       return (!$queue->validate()) ?

lib/Mailer/MailerError.php

@@ -20,17 +20,22 @@ class MailerError {
   /** @var int|null */
   private $retry_interval;
 
+  /** @var array */
+  private $subscribers_errors = [];
+
   /**
    * @param string $operation
    * @param string $level
    * @param null|string $message
    * @param int|null $retry_interval
+   * @param array $subscribers_errors
    */
-  function __construct($operation, $level, $message = null, $retry_interval = null) {
+  function __construct($operation, $level, $message = null, $retry_interval = null, array $subscribers_errors = []) {
     $this->operation = $operation;
     $this->level = $level;
     $this->message = $message;
     $this->retry_interval = $retry_interval;
+    $this->subscribers_errors = $subscribers_errors;
   }
 
   /**
@@ -60,4 +65,34 @@ class MailerError {
   function getRetryInterval() {
     return $this->retry_interval;
   }
+
+  /**
+   * @return array
+   */
+  function getSubscriberErrors() {
+    return $this->subscribers_errors;
+  }
+
+  function getMessageWithFailedSubscribers() {
+    $message = $this->message ?: '';
+    if(!$this->subscribers_errors) {
+      return $message;
+    }
+
+    $message .= $this->message ? ' ' : '';
+
+    if(count($this->subscribers_errors) === 1) {
+      $message .=  __('Unprocessed subscriber:', 'mailpoet') . ' ';
+    } else {
+      $message .=  __('Unprocessed subscribers:', 'mailpoet') . ' ';
+    }
+
+    $message .= implode(
+      ', ',
+      array_map(function (SubscriberError $subscriber_error) {
+        return "($subscriber_error)";
+      }, $this->subscribers_errors)
+    );
+    return $message;
+  }
 }

lib/Mailer/Methods/ErrorMappers/AmazonSESMapper.php

@@ -3,6 +3,7 @@ namespace MailPoet\Mailer\Methods\ErrorMappers;
 
 use MailPoet\Mailer\MailerError;
 use MailPoet\Mailer\Mailer;
+use MailPoet\Mailer\SubscriberError;
 
 class AmazonSESMapper {
   use ConnectionErrorMapperTrait;
@@ -15,9 +16,11 @@ class AmazonSESMapper {
     $response = ($response) ?
       $response->Error->Message->__toString() :
       sprintf(__('%s has returned an unknown error.', 'mailpoet'), Mailer::METHOD_AMAZONSES);
+
+    $subscriber_errors = [];
     if(empty($extra_params['test_email'])) {
-      $response .= sprintf(' %s: %s', __('Unprocessed subscriber', 'mailpoet'), $subscriber);
+      $subscriber_errors[] = new SubscriberError($subscriber, null);
     }
-    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $response);
+    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $response, null, $subscriber_errors);
   }
 }

lib/Mailer/Methods/ErrorMappers/MailPoetMapper.php

@@ -2,7 +2,9 @@
 namespace MailPoet\Mailer\Methods\ErrorMappers;
 
 use MailPoet\Mailer\MailerError;
+use MailPoet\Mailer\SubscriberError;
 use MailPoet\Services\Bridge\API;
+use InvalidArgumentException;
 
 if(!defined('ABSPATH')) exit;
 
@@ -20,17 +22,27 @@ class MailPoetMapper {
   }
 
   function getErrorForResult(array $result, $subscribers) {
-    $message = $result['message'];
     $level = MailerError::LEVEL_HARD;
     $retry_interval = null;
+    $subscribers_errors = [];
+    $result_code = !empty($result['code']) ? $result['code'] : null;
 
-    if(!empty($result['code'])) {
-      switch($result['code']) {
+    switch($result_code) {
       case API::RESPONSE_CODE_NOT_ARRAY:
         $message = __('JSON input is not an array', 'mailpoet');
         break;
       case API::RESPONSE_CODE_PAYLOAD_ERROR:
-          $message = $this->parseErrorResponse($result['message'], $subscribers);
+        $result_parsed = json_decode($result['message'], true);
+        $message = __('Error while sending.', 'mailpoet');
+        if(is_array($result_parsed)) {
+          try {
+            $subscribers_errors = $this->getSubscribersErrors($result_parsed, $subscribers);
+          } catch (InvalidArgumentException $e) {
+            $message .= ' ' . $e->getMessage();
+          }
+        } else {
+          $message .= ' ' . $result['message'];
+        }
         break;
       case API::RESPONSE_CODE_TEMPORARY_UNAVAILABLE:
         $message = __('Email service is temporarily not available, please try again in a few minutes.', 'mailpoet');
@@ -41,40 +53,24 @@ class MailPoetMapper {
       default:
         $message = $result['message'];
     }
-    }
-    return new MailerError(MailerError::OPERATION_SEND, $level, $message, $retry_interval);
+    return new MailerError(MailerError::OPERATION_SEND, $level, $message, $retry_interval, $subscribers_errors);
   }
 
-  private function parseErrorResponse($result, $subscriber) {
-    $result_parsed = json_decode($result, true);
+  private function getSubscribersErrors($result_parsed, $subscribers) {
     $errors = [];
-    if(is_array($result_parsed)) {
     foreach($result_parsed as $result_error) {
-        $errors[] = $this->processSingleSubscriberError($result_error, $subscriber);
+      if(!is_array($result_error) || !isset($result_error['index']) || !isset($subscribers[$result_error['index']])) {
+        throw new InvalidArgumentException( __('Invalid MSS response format.', 'mailpoet'));
       }
-    }
-    if(!empty($errors)) {
-      return __('Error while sending: ', 'mailpoet') . join(', ', $errors);
-    } else {
-      return __('Error while sending newsletters. ', 'mailpoet') . $result;
-    }
-  }
-
-  private function processSingleSubscriberError($result_error, $subscriber) {
-    $error = '';
-    if(is_array($result_error)) {
       $subscriber_errors = [];
       if(isset($result_error['errors']) && is_array($result_error['errors'])) {
         array_walk_recursive($result_error['errors'], function($item) use (&$subscriber_errors) {
           $subscriber_errors[] = $item;
         });
       }
-      $error .= join(', ', $subscriber_errors);
-
-      if(isset($result_error['index']) && isset($subscriber[$result_error['index']])) {
-        $error = '(' . $subscriber[$result_error['index']] . ': ' . $error . ')';
-      }
-    }
-    return $error;
+      $message = join(', ', $subscriber_errors);
+      $errors[] = new SubscriberError($subscribers[$result_error['index']], $message);
+    }
+    return $errors;
   }
 }

lib/Mailer/Methods/ErrorMappers/PHPMailMapper.php

@@ -3,6 +3,7 @@ namespace MailPoet\Mailer\Methods\ErrorMappers;
 
 use MailPoet\Mailer\MailerError;
 use MailPoet\Mailer\Mailer;
+use MailPoet\Mailer\SubscriberError;
 
 class PHPMailMapper {
   use ConnectionErrorMapperTrait;
@@ -13,9 +14,10 @@ class PHPMailMapper {
 
   function getErrorForSubscriber($subscriber, $extra_params) {
     $message = sprintf(__('%s has returned an unknown error.', 'mailpoet'), Mailer::METHOD_PHPMAIL);
+    $subscriber_errors = [];
     if(empty($extra_params['test_email'])) {
-      $message .= sprintf(' %s: %s', __('Unprocessed subscriber', 'mailpoet'), $subscriber);
+      $subscriber_errors[] = new SubscriberError($subscriber, null);
     }
-    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $message);
+    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $message, null, $subscriber_errors);
   }
 }

lib/Mailer/Methods/ErrorMappers/SMTPMapper.php

@@ -3,6 +3,7 @@ namespace MailPoet\Mailer\Methods\ErrorMappers;
 
 use MailPoet\Mailer\MailerError;
 use MailPoet\Mailer\Mailer;
+use MailPoet\Mailer\SubscriberError;
 
 class SMTPMapper {
   use ConnectionErrorMapperTrait;
@@ -23,9 +24,11 @@ class SMTPMapper {
     } else {
       $message = sprintf(__('%s has returned an unknown error.', 'mailpoet'), Mailer::METHOD_SMTP);
     }
+
+    $subscriber_errors = [];
     if(empty($extra_params['test_email'])) {
-      $message .= sprintf(' %s: %s', __('Unprocessed subscriber', 'mailpoet'), $subscriber);
+      $subscriber_errors[] = new SubscriberError($subscriber, null);
     }
-    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $message);
+    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $message, null, $subscriber_errors);
   }
 }

lib/Mailer/Methods/ErrorMappers/SendGridMapper.php

@@ -3,6 +3,7 @@ namespace MailPoet\Mailer\Methods\ErrorMappers;
 
 use MailPoet\Mailer\MailerError;
 use MailPoet\Mailer\Mailer;
+use MailPoet\Mailer\SubscriberError;
 
 class SendGridMapper {
   use ConnectionErrorMapperTrait;
@@ -11,9 +12,11 @@ class SendGridMapper {
     $response = (!empty($response['errors'][0])) ?
       $response['errors'][0] :
       sprintf(__('%s has returned an unknown error.', 'mailpoet'), Mailer::METHOD_SENDGRID);
+
+    $subscriber_errors = [];
     if(empty($extra_params['test_email'])) {
-      $response .= sprintf(' %s: %s', __('Unprocessed subscriber', 'mailpoet'), $subscriber);
+      $subscriber_errors[] = new SubscriberError($subscriber, null);
     }
-    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $response);
+    return new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, $response, null, $subscriber_errors);
   }
 }

lib/Mailer/SubscriberError.php

@@ -0,0 +1,38 @@
+<?php
+namespace MailPoet\Mailer;
+
+class SubscriberError {
+
+  /** @var string */
+  private $email;
+
+  /** @var string|null */
+  private $message;
+
+  /**
+   * @param string $email
+   * @param string $message|null
+   */
+  function __construct($email, $message = null) {
+    $this->email = $email;
+    $this->message = $message;
+  }
+
+  /**
+   * @return string
+   */
+  function getEmail() {
+    return $this->email;
+  }
+
+  /**
+   * @return null|string
+   */
+  function getMessage() {
+    return $this->message;
+  }
+
+  function __toString() {
+    return $this->message ? $this->email . ': ' . $this->message : $this->email;
+  }
+}

tests/unit/Mailer/MailerErrorTest.php

@@ -0,0 +1,40 @@
+<?php
+namespace MailPoet\Test\Mailer;
+
+use MailPoet\Mailer\MailerError;
+use MailPoet\Mailer\SubscriberError;
+
+class MailerErrorTest extends \MailPoetTest {
+
+  function testItCanComposeErrorMessageWithoutSubscribers() {
+    $error = new MailerError(MailerError::OPERATION_SEND, MailerError::LEVEL_HARD, 'Some Message');
+    expect($error->getMessageWithFailedSubscribers())->equals('Some Message');
+  }
+
+  function testItCanComposeErrorMessageWithOneSubscriber() {
+    $subscriber_error = new SubscriberError('email@example.com', 'Subscriber message');
+    $error = new MailerError(
+      MailerError::OPERATION_SEND,
+      MailerError::LEVEL_HARD,
+      'Some Message',
+      null,
+      [$subscriber_error]
+    );
+    expect($error->getMessageWithFailedSubscribers())->equals('Some Message Unprocessed subscriber: (email@example.com: Subscriber message)');
+  }
+
+  function testItCanComposeErrorMessageWithMultipleSubscriberErrors() {
+    $subscriber_error_1 = new SubscriberError('email1@example.com', 'Subscriber 1 message');
+    $subscriber_error_2 = new SubscriberError('email2@example.com', null);
+    $error = new MailerError(
+      MailerError::OPERATION_SEND,
+      MailerError::LEVEL_HARD,
+      'Some Message',
+      null,
+      [$subscriber_error_1, $subscriber_error_2]
+    );
+    expect($error->getMessageWithFailedSubscribers())->equals(
+      'Some Message Unprocessed subscribers: (email1@example.com: Subscriber 1 message), (email2@example.com)'
+    );
+  }
+}

tests/unit/Mailer/Methods/ErrorMappers/MailPoetMapperTest.php

@@ -62,7 +62,7 @@ class MailPoetMapperTest extends \MailPoetTest {
     expect($error)->isInstanceOf(MailerError::class);
     expect($error->getOperation())->equals(MailerError::OPERATION_SEND);
     expect($error->getLevel())->equals(MailerError::LEVEL_HARD);
-    expect($error->getMessage())->equals('Error while sending newsletters. Api Error');
+    expect($error->getMessage())->equals('Error while sending. Api Error');
   }
 
   function testGetPayloadErrorWithErrorMessage() {
@@ -75,6 +75,24 @@ class MailPoetMapperTest extends \MailPoetTest {
     expect($error)->isInstanceOf(MailerError::class);
     expect($error->getOperation())->equals(MailerError::OPERATION_SEND);
     expect($error->getLevel())->equals(MailerError::LEVEL_HARD);
-    expect($error->getMessage())->equals('Error while sending: (a@example.com: subject is missing), (c d <b@example.com>: subject is missing)');
+    $subscriber_errors = $error->getSubscriberErrors();
+    expect(count($subscriber_errors))->equals(2);
+    expect($subscriber_errors[0]->getEmail())->equals('a@example.com');
+    expect($subscriber_errors[0]->getMessage())->equals('subject is missing');
+    expect($subscriber_errors[1]->getEmail())->equals('c d <b@example.com>');
+    expect($subscriber_errors[1]->getMessage())->equals('subject is missing');
+  }
+
+  function testGetPayloadErrorForMalformedMSSResponse() {
+    $api_result = [
+      'code' => API::RESPONSE_CODE_PAYLOAD_ERROR,
+      'status' => API::SENDING_STATUS_SEND_ERROR,
+      'message' => '[{"errors":{"subject":"subject is missing"}},{"errors":{"subject":"subject is missing"}}]'
+    ];
+    $error = $this->mapper->getErrorForResult($api_result, $this->subscribers);
+    expect($error)->isInstanceOf(MailerError::class);
+    expect($error->getOperation())->equals(MailerError::OPERATION_SEND);
+    expect($error->getLevel())->equals(MailerError::LEVEL_HARD);
+    expect($error->getMessage())->equals('Error while sending. Invalid MSS response format.');
   }
 }

tests/unit/Mailer/Methods/ErrorMappers/SMTPMapperTest.php

@@ -35,12 +35,14 @@ class SMTPMapperTest extends \MailPoetTest {
       . '<< 250 Reset OK' . PHP_EOL;
     $error = $this->mapper->getErrorFromLog($log, 'test@example.com', []);
     expect($error->getMessage())
-      ->equals('Expected response code 250/251/252 but got code "550", with message "550 No such recipient here" (code: 550) Unprocessed subscriber: test@example.com');
+      ->equals('Expected response code 250/251/252 but got code "550", with message "550 No such recipient here" (code: 550)');
+    expect($error->getSubscriberErrors()[0]->getEmail('moi@mrcasual.com'));
   }
 
   function testItReturnsGenericMessageWhenLogMessageDoesNotExist() {
     $error = $this->mapper->getErrorFromLog(null, 'test@example.com');
     expect($error->getMessage())
-      ->equals(Mailer::METHOD_SMTP . ' has returned an unknown error. Unprocessed subscriber: test@example.com');
+      ->equals(Mailer::METHOD_SMTP . ' has returned an unknown error.');
+    expect($error->getSubscriberErrors()[0]->getEmail('moi@mrcasual.com'));
   }
 }