Updates class to use newly added validate() method on the model

Validates existing queue's rendered newsletter body
Cleans code formatting

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

@@ -1,13 +1,14 @@
 <?php
+
 namespace MailPoet\Cron\Workers\SendingQueue\Tasks;
 
-use MailPoet\Models\SendingQueue as SendingQueueModel;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Links as LinksTask;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Posts as PostsTask;
 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;
@@ -28,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;
     }
@@ -51,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) {
@@ -78,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();
@@ -95,13 +102,10 @@ class Newsletter {
     if(!$queue_errors) {
       // verify that the rendered body was successfully saved
       $queue = SendingQueueModel::findOne($queue->id);
-      $queue_errors = ($queue->isRenderedNewsletterBodyValid() !== true);
+      $queue_errors = ($queue->validate() !== true);
     }
     if($queue_errors) {
-      return MailerLog::processError(
+      $this->stopNewsletterPreProcessing();
-        'queue_save',
-        __('There was an error processing your newsletter during sending. If possible, please contact us and report this issue.')
-      );
     }
     return $newsletter;
   }
@@ -157,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/SendingQueue.php

@@ -51,7 +51,7 @@ class SendingQueue extends Model {
   }
 
   function save() {
-    if(!is_serialized($this->subscribers) && !is_null($this->newsletter_rendered_body)) {
+    if(!is_serialized($this->subscribers) && !is_null($this->subscribers)) {
       $this->set('subscribers', serialize($this->subscribers));
     }
     if(!is_serialized($this->newsletter_rendered_body) && !is_null($this->newsletter_rendered_body)) {

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

@@ -116,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();
   }
@@ -272,7 +272,24 @@ class NewsletterTaskTest extends MailPoetTest {
     }
   }
 
-  function testItLogsErrorWhenQueueRenderedNewsletterBodyIsInvalid() {
+  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(),
@@ -295,7 +312,7 @@ class NewsletterTaskTest extends MailPoetTest {
     }
   }
 
-  function testItPreProcessesNewsletterWhenQueueRenderedNewsletterBodyIsValid() {
+  function testItPreProcessesNewsletterWhenNewlyRenderedNewsletterBodyIsValid() {
     $queue = $this->queue;
     $queue_mock = Mock::double(
       new stdClass(),

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