Merge pull request #1018 from mailpoet/prevent_sending_with_broken_newsletter_body

Prevents sending emails when rendered newsletter is broken [MAILPOET-1020]

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

@@ -1,4 +1,5 @@
 <?php
+
 namespace MailPoet\Cron\Workers\SendingQueue\Tasks;
 
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Links as LinksTask;
@@ -7,6 +8,7 @@ use MailPoet\Cron\Workers\SendingQueue\Tasks\Shortcodes as ShortcodesTask;
 use MailPoet\Mailer\MailerLog;
 use MailPoet\Models\Newsletter as NewsletterModel;
 use MailPoet\Models\NewsletterSegment as NewsletterSegmentModel;
+use MailPoet\Models\SendingQueue as SendingQueueModel;
 use MailPoet\Models\Setting;
 use MailPoet\Newsletter\Links\Links as NewsletterLinks;
 use MailPoet\Newsletter\Renderer\PostProcess\OpenTracking;
@@ -27,20 +29,24 @@ class Newsletter {
     // get existing active or sending newsletter
     $newsletter = $queue->newsletter()
       ->whereNull('deleted_at')
-      ->whereAnyIs(array(
+      ->whereAnyIs(
-        array('status' => NewsletterModel::STATUS_ACTIVE),
+        array(
-        array('status' => NewsletterModel::STATUS_SENDING)
+          array('status' => NewsletterModel::STATUS_ACTIVE),
-      ))
+          array('status' => NewsletterModel::STATUS_SENDING)
+        )
+      )
       ->findOne();
     if(!$newsletter) return false;
     // if this is a notification history, get existing active or sending parent newsletter
     if($newsletter->type == NewsletterModel::TYPE_NOTIFICATION_HISTORY) {
       $parent_newsletter = $newsletter->parent()
         ->whereNull('deleted_at')
-        ->whereAnyIs(array(
+        ->whereAnyIs(
-          array('status' => NewsletterModel::STATUS_ACTIVE),
+          array(
-          array('status' => NewsletterModel::STATUS_SENDING)
+            array('status' => NewsletterModel::STATUS_ACTIVE),
-        ))
+            array('status' => NewsletterModel::STATUS_SENDING)
+          )
+        )
         ->findOne();
       if(!$parent_newsletter) return false;
     }
@@ -50,7 +56,9 @@ class Newsletter {
   function preProcessNewsletter($newsletter, $queue) {
     // return the newsletter if it was previously rendered
     if(!is_null($queue->getNewsletterRenderedBody())) {
-      return $newsletter;
+      return (!$queue->validate()) ?
+        $this->stopNewsletterPreProcessing() :
+        $newsletter;
     }
     // if tracking is enabled, do additional processing
     if($this->tracking_enabled) {
@@ -77,7 +85,7 @@ class Newsletter {
     // check if this is a post notification and if it contains posts
     $newsletter_contains_posts = strpos($rendered_newsletter['html'], 'data-post-id');
     if($newsletter->type === NewsletterModel::TYPE_NOTIFICATION_HISTORY &&
-       !$newsletter_contains_posts
+      !$newsletter_contains_posts
     ) {
       // delete notification history record since it will never be sent
       $newsletter->delete();
@@ -89,11 +97,15 @@ class Newsletter {
     $queue->newsletter_rendered_subject = Shortcodes::process($newsletter->subject, $newsletter, null, $queue);
     $queue->newsletter_rendered_body = $rendered_newsletter;
     $queue->save();
-    if($queue->getErrors()) {
+    // catch DB errors
-      return MailerLog::processError(
+    $queue_errors = $queue->getErrors();
-        'queue_save',
+    if(!$queue_errors) {
-        __('There was an error processing your newsletter during sending. If possible, please contact us and report this issue.')
+      // verify that the rendered body was successfully saved
-      );
+      $queue = SendingQueueModel::findOne($queue->id);
+      $queue_errors = ($queue->validate() !== true);
+    }
+    if($queue_errors) {
+      $this->stopNewsletterPreProcessing();
     }
     return $newsletter;
   }
@@ -149,4 +161,11 @@ class Newsletter {
       ->findArray();
     return Helpers::flattenArray($segments);
   }
+
+  function stopNewsletterPreProcessing() {
+    MailerLog::processError(
+      'queue_save',
+      __('There was an error processing your newsletter during sending. If possible, please contact us and report this issue.')
+    );
+  }
 }

lib/Models/Model.php

@@ -1,4 +1,5 @@
 <?php
+
 namespace MailPoet\Models;
 
 if(!defined('ABSPATH')) exit;
@@ -78,14 +79,12 @@ class Model extends \Sudzy\ValidModel {
 
   static function bulkTrash($orm) {
     $model = get_called_class();
-    $count = self::bulkAction($orm, function($ids) use($model) {
+    $count = self::bulkAction($orm, function($ids) use ($model) {
       $model::rawExecute(join(' ', array(
-          'UPDATE `'.$model::$_table.'`',
+        'UPDATE `' . $model::$_table . '`',
-          'SET `deleted_at` = NOW()',
+        'SET `deleted_at` = NOW()',
-          'WHERE `id` IN ('.rtrim(str_repeat('?,', count($ids)), ',').')'
+        'WHERE `id` IN (' . rtrim(str_repeat('?,', count($ids)), ',') . ')'
-        )),
+      )), $ids);
-        $ids
-      );
     });
 
     return array('count' => $count);
@@ -93,7 +92,7 @@ class Model extends \Sudzy\ValidModel {
 
   static function bulkDelete($orm) {
     $model = get_called_class();
-    $count = self::bulkAction($orm, function($ids) use($model) {
+    $count = self::bulkAction($orm, function($ids) use ($model) {
       $model::whereIn('id', $ids)->deleteMany();
     });
 
@@ -106,14 +105,12 @@ class Model extends \Sudzy\ValidModel {
 
   static function bulkRestore($orm) {
     $model = get_called_class();
-    $count = self::bulkAction($orm, function($ids) use($model) {
+    $count = self::bulkAction($orm, function($ids) use ($model) {
       $model::rawExecute(join(' ', array(
-          'UPDATE `'.$model::$_table.'`',
+        'UPDATE `' . $model::$_table . '`',
-          'SET `deleted_at` = NULL',
+        'SET `deleted_at` = NULL',
-          'WHERE `id` IN ('.rtrim(str_repeat('?,', count($ids)), ',').')'
+        'WHERE `id` IN (' . rtrim(str_repeat('?,', count($ids)), ',') . ')'
-        )),
+      )), $ids);
-        $ids
-      );
     });
 
     return array('count' => $count);
@@ -124,7 +121,7 @@ class Model extends \Sudzy\ValidModel {
 
     if($total === 0) return false;
 
-    $rows = $orm->select(static::$_table.'.id')
+    $rows = $orm->select(static::$_table . '.id')
       ->offset(null)
       ->limit(null)
       ->findArray();
@@ -138,7 +135,8 @@ class Model extends \Sudzy\ValidModel {
     }
 
     // get number of affected rows
-    return $orm->get_last_statement()->rowCount();
+    return $orm->get_last_statement()
+      ->rowCount();
   }
 
   function duplicate($data = array()) {
@@ -146,7 +144,7 @@ class Model extends \Sudzy\ValidModel {
     $model_data = array_merge($this->asArray(), $data);
     unset($model_data['id']);
 
-    $duplicate =  $model::create();
+    $duplicate = $model::create();
     $duplicate->hydrate($model_data);
     $duplicate->set_expr('created_at', 'NOW()');
     $duplicate->set_expr('updated_at', 'NOW()');
@@ -185,8 +183,17 @@ class Model extends \Sudzy\ValidModel {
   public static function __callStatic($method, $parameters) {
     try {
       return parent::__callStatic($method, $parameters);
-    } catch (\PDOException $e) {
+    } catch(\PDOException $e) {
       throw new \Exception($e->getMessage());
     }
   }
+
+  public function validate() {
+    $success = true;
+    foreach(array_keys($this->_validations) as $field) {
+      $success = $success && $this->validateField($field, $this->$field);
+    }
+    $this->setError($this->getValidationErrors());
+    return $success;
+  }
 }

lib/Models/ModelValidator.php

@@ -10,7 +10,8 @@ class ModelValidator extends \Sudzy\Engine {
   function __construct() {
     parent::__construct();
     $this->validators = array(
-      'validEmail' => 'validateEmail'
+      'validEmail' => 'validateEmail',
+      'validRenderedNewsletterBody' => 'validateRenderedNewsletterBody'
     );
     $this->setupValidators();
   }
@@ -27,4 +28,11 @@ class ModelValidator extends \Sudzy\Engine {
   function validateEmail($email) {
     return is_email($email) !== false;
   }
+
+  function validateRenderedNewsletterBody($newsletter_body) {
+    $newsletter_body = (!is_serialized($newsletter_body)) ?
+      $newsletter_body :
+      unserialize($newsletter_body);
+    return (is_null($newsletter_body) || (is_array($newsletter_body) && !empty($newsletter_body['html']) && !empty($newsletter_body['text'])));
+  }
 }

lib/Models/SendingQueue.php

@@ -12,6 +12,14 @@ class SendingQueue extends Model {
   const PRIORITY_MEDIUM = 5;
   const PRIORITY_LOW = 10;
 
+  function __construct() {
+    parent::__construct();
+
+    $this->addValidations('newsletter_rendered_body', array(
+      'validRenderedNewsletterBody' => __('Rendered newsletter body is invalid!', 'mailpoet')
+    ));
+  }
+
   function newsletter() {
     return $this->has_one(__NAMESPACE__ . '\Newsletter', 'id', 'newsletter_id');
   }
@@ -43,10 +51,10 @@ class SendingQueue extends Model {
   }
 
   function save() {
-    if(!is_serialized($this->subscribers)) {
+    if(!is_serialized($this->subscribers) && !is_null($this->subscribers)) {
       $this->set('subscribers', serialize($this->subscribers));
     }
-    if(!is_serialized($this->newsletter_rendered_body)) {
+    if(!is_serialized($this->newsletter_rendered_body) && !is_null($this->newsletter_rendered_body)) {
       $this->set('newsletter_rendered_body', serialize($this->newsletter_rendered_body));
     }
     // set the default priority to medium
@@ -86,9 +94,8 @@ class SendingQueue extends Model {
 
   function asArray() {
     $model = parent::asArray();
-    $model['subscribers'] = (is_serialized($this->subscribers))
+    $model['subscribers'] = $this->getSubscribers();
-      ? unserialize($this->subscribers)
+    $model['newsletter_rendered_body'] = $this->getNewsletterRenderedBody();
-      : $this->subscribers;
     return $model;
   }
 

tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php

@@ -1,5 +1,6 @@
 <?php
 
+use AspectMock\Test as Mock;
 use Codeception\Util\Fixtures;
 use Helper\WordPressHooks as WPHooksHelper;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask;
@@ -115,7 +116,7 @@ class NewsletterTaskTest extends MailPoetTest {
 
   function testItReturnsNewsletterObjectWhenRenderedNewsletterBodyExistsInTheQueue() {
     $queue = $this->queue;
-    $queue->newsletter_rendered_body = true;
+    $queue->newsletter_rendered_body = array('html' => 'test', 'text' => 'test');
     $result = $this->newsletter_task->preProcessNewsletter($this->newsletter, $queue);
     expect($result instanceof \MailPoet\Models\Newsletter)->true();
   }
@@ -271,6 +272,62 @@ class NewsletterTaskTest extends MailPoetTest {
     }
   }
 
+  function testItLogsErrorWhenExistingRenderedNewsletterBodyIsInvalid() {
+    $queue_mock = Mock::double(
+      new stdClass(),
+      array(
+        'getNewsletterRenderedBody' => 'a:2:{s:4:"html"'
+      )
+    );
+    try {
+      $this->newsletter_task->preProcessNewsletter($this->newsletter, $queue_mock);
+      self::fail('Sending error exception was not thrown.');
+    } catch(Exception $e) {
+      $mailer_log = MailerLog::getMailerLog();
+      expect($mailer_log['error']['operation'])->equals('queue_save');
+      expect($mailer_log['error']['error_message'])->equals('There was an error processing your newsletter during sending. If possible, please contact us and report this issue.');
+    }
+  }
+
+  function testItLogsErrorWhenNewlyRenderedNewsletterBodyIsInvalid() {
+    $queue = $this->queue;
+    $queue_mock = Mock::double(
+      new stdClass(),
+      array(
+        'getNewsletterRenderedBody' => null
+      )
+    );
+    $queue_mock->id = $queue->id;
+
+    // broken serialized object
+    $queue->newsletter_rendered_body = 'a:2:{s:4:"html"';
+    $queue->save();
+    try {
+      $this->newsletter_task->preProcessNewsletter($this->newsletter, $queue_mock);
+      self::fail('Sending error exception was not thrown.');
+    } catch(Exception $e) {
+      $mailer_log = MailerLog::getMailerLog();
+      expect($mailer_log['error']['operation'])->equals('queue_save');
+      expect($mailer_log['error']['error_message'])->equals('There was an error processing your newsletter during sending. If possible, please contact us and report this issue.');
+    }
+  }
+
+  function testItPreProcessesNewsletterWhenNewlyRenderedNewsletterBodyIsValid() {
+    $queue = $this->queue;
+    $queue_mock = Mock::double(
+      new stdClass(),
+      array(
+        'getNewsletterRenderedBody' => null
+      )
+    );
+    $queue_mock->id = $queue->id;
+
+    // properly serialized object
+    $queue->newsletter_rendered_body = 'a:2:{s:4:"html";s:4:"test";s:4:"text";s:4:"test";}';
+    $queue->save();
+    expect($this->newsletter_task->preProcessNewsletter($this->newsletter, $queue_mock))->equals($this->newsletter);
+  }
+
   function _after() {
     WPHooksHelper::releaseAllHooks();
     ORM::raw_execute('TRUNCATE ' . Subscriber::$_table);

tests/unit/Models/ModelValidatorTest.php

@@ -20,6 +20,19 @@ class ModelValidatorTest extends MailPoetTest {
   function testItValidatesEmail() {
     expect($this->validator->validateEmail('test'))->false();
     expect($this->validator->validateEmail('tést@éxample.com'))->false();
+    
     expect($this->validator->validateEmail('test@example.com'))->true();
   }
+
+  function testItValidatesRenderedNewsletterBody() {
+    expect($this->validator->validateRenderedNewsletterBody('test'))->false();
+    expect($this->validator->validateRenderedNewsletterBody(serialize('test')))->false();
+    expect($this->validator->validateRenderedNewsletterBody(array('html' => 'test', 'text' => null)))->false();
+    expect($this->validator->validateRenderedNewsletterBody(array('html' => null, 'text' => 'test')))->false();
+    
+    expect($this->validator->validateRenderedNewsletterBody(null))->true();
+    expect($this->validator->validateRenderedNewsletterBody(serialize(null)))->true();
+    expect($this->validator->validateRenderedNewsletterBody(serialize(array('html' => 'test', 'text' => 'test'))))->true();
+    expect($this->validator->validateRenderedNewsletterBody(array('html' => 'test', 'text' => 'test')))->true();
+  }
 }

tests/unit/Newsletter/ViewInBrowserTest.php

@@ -60,10 +60,12 @@ class ViewInBrowserTest extends MailPoetTest {
         'status' => 'active'
       );
     $this->queue_rendered_newsletter_without_tracking = array(
-      'html' => '<p>Newsletter from queue. Hello, [subscriber:firstname | default:reader]. <a href="[link:newsletter_view_in_browser_url]">Unsubscribe</a> or visit <a href="http://google.com">Google</a></p>'
+      'html' => '<p>Newsletter from queue. Hello, [subscriber:firstname | default:reader]. <a href="[link:newsletter_view_in_browser_url]">Unsubscribe</a> or visit <a href="http://google.com">Google</a></p>',
+      'text' => 'test'
     );
     $this->queue_rendered_newsletter_with_tracking = array(
-      'html' => '<p>Newsletter from queue. Hello, [subscriber:firstname | default:reader]. <a href="' . Links::DATA_TAG_CLICK . '-90e56">Unsubscribe</a> or visit <a href="' . Links::DATA_TAG_CLICK . '-i1893">Google</a><img alt="" class="" src="' . Links::DATA_TAG_OPEN . '"></p>'
+      'html' => '<p>Newsletter from queue. Hello, [subscriber:firstname | default:reader]. <a href="' . Links::DATA_TAG_CLICK . '-90e56">Unsubscribe</a> or visit <a href="' . Links::DATA_TAG_CLICK . '-i1893">Google</a><img alt="" class="" src="' . Links::DATA_TAG_OPEN . '"></p>',
+      'text' => 'test'
     );
     $this->view_in_browser = new ViewInBrowser();
     // create newsletter