Refactor fetching newsletter options to respect its type [MAILPOET-1431]

2018-06-19 16:09:29 +02:00
parent 9446e3cd50
commit 9308cffd45
10 changed files with 45 additions and 37 deletions
--- a/lib/API/JSON/v1/Newsletters.php
+++ b/lib/API/JSON/v1/Newsletters.php
@@ -99,8 +99,7 @@ class Newsletters extends APIEndpoint {
        }
      }
      // reload newsletter with updated options
-      $newsletter = Newsletter::filter('filterWithOptions')
-        ->findOne($newsletter->id);
+      $newsletter = Newsletter::filter('filterWithOptions', $newsletter->type)->findOne($newsletter->id);
      // if this is a post notification, process newsletter options and update its schedule
      if($newsletter->type === Newsletter::TYPE_NOTIFICATION) {
        // generate the new schedule from options and get the new "next run" date
@@ -151,7 +150,7 @@ class Newsletters extends APIEndpoint {
    }

    $id = (isset($data['id'])) ? (int)$data['id'] : false;
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($id);
+    $newsletter = Newsletter::findOneWithOptions($id);

    if($newsletter === false) {
      return $this->errorResponse(array(
@@ -487,7 +486,7 @@ class Newsletters extends APIEndpoint {
        &&
        $data['type'] === Newsletter::TYPE_NOTIFICATION
      ) {
-        $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+        $newsletter = Newsletter::filter('filterWithOptions', $data['type'])->findOne($newsletter->id);
        Scheduler::processPostNotificationSchedule($newsletter);
      }

--- a/lib/API/JSON/v1/SendingQueue.php
+++ b/lib/API/JSON/v1/SendingQueue.php
@@ -25,8 +25,7 @@ class SendingQueue extends APIEndpoint {
    );

    // check that the newsletter exists
-    $newsletter = Newsletter::filter('filterWithOptions')
-      ->findOne($newsletter_id);
+    $newsletter = Newsletter::findOneWithOptions($newsletter_id);

    if($newsletter === false) {
      return $this->errorResponse(array(
--- a/lib/Cron/Workers/Scheduler.php
+++ b/lib/Cron/Workers/Scheduler.php
@@ -33,7 +33,7 @@ class Scheduler {
    if(!count($scheduled_queues)) return false;
    $this->updateTasks($scheduled_queues);
    foreach($scheduled_queues as $i => $queue) {
-      $newsletter = Newsletter::filter('filterWithOptions')->findOne($queue->newsletter_id);
+      $newsletter = Newsletter::findOneWithOptions($queue->newsletter_id);
      if(!$newsletter || $newsletter->deleted_at !== null) {
        $queue->delete();
      } elseif($newsletter->status !== Newsletter::STATUS_ACTIVE && $newsletter->status !== Newsletter::STATUS_SCHEDULED) {
--- a/lib/Models/Newsletter.php
+++ b/lib/Models/Newsletter.php
@@ -666,10 +666,13 @@ class Newsletter extends Model {
    return $orm;
  }

-  static function filterWithOptions($orm) {
+  static function filterWithOptions($orm, $type) {
    $orm = $orm->select(MP_NEWSLETTERS_TABLE.'.*');
    $optionFields = NewsletterOptionField::findArray();
    foreach($optionFields as $optionField) {
+      if($optionField['newsletter_type'] !== $type) {
+        continue;
+      }
      $orm = $orm->select_expr(
        'IFNULL(GROUP_CONCAT(CASE WHEN ' .
        MP_NEWSLETTER_OPTION_FIELDS_TABLE . '.id=' . $optionField['id'] . ' THEN ' .
@@ -981,4 +984,12 @@ class Newsletter extends Model {

    return (Helpers::isJson($this->meta)) ? json_decode($this->meta, true) : $this->meta;
  }
+
+  static function findOneWithOptions($id) {
+    $newsletter = self::findOne($id);
+    if($newsletter === false) {
+      return false;
+    }
+    return self::filter('filterWithOptions', $newsletter->type)->findOne($id);
+  }
 }
--- a/lib/Newsletter/Scheduler/Scheduler.php
+++ b/lib/Newsletter/Scheduler/Scheduler.php
@@ -228,7 +228,7 @@ class Scheduler {
    return Newsletter::getPublished()
      ->filter('filterType', $type, $group)
      ->filter('filterStatus', Newsletter::STATUS_ACTIVE)
-      ->filter('filterWithOptions')
+      ->filter('filterWithOptions', $type)
      ->findMany();
  }

--- a/tests/unit/API/JSON/v1/NewslettersTest.php
+++ b/tests/unit/API/JSON/v1/NewslettersTest.php
@@ -132,7 +132,7 @@ class NewslettersTest extends \MailPoetTest {

    $router = new Newsletters();
    $response = $router->save($valid_data);
-    $saved_newsletter = Newsletter::filter('filterWithOptions')
+    $saved_newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_STANDARD)
      ->findOne($response->data['id']);
    expect($response->status)->equals(APIResponse::STATUS_OK);
    expect($response->data)->equals($saved_newsletter->asArray());
@@ -253,7 +253,7 @@ class NewslettersTest extends \MailPoetTest {
      )
    );
    $response = $router->save($newsletter_data);
-    $saved_newsletter = Newsletter::filter('filterWithOptions')
+    $saved_newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($response->data['id']);
    expect($response->status)->equals(APIResponse::STATUS_OK);
    expect($response->data)->equals($saved_newsletter->asArray());
@@ -261,7 +261,7 @@ class NewslettersTest extends \MailPoetTest {
    // schedule should be recalculated when options change
    $newsletter_data['options']['intervalType'] = Scheduler::INTERVAL_IMMEDIATELY;
    $response = $router->save($newsletter_data);
-    $saved_newsletter = Newsletter::filter('filterWithOptions')
+    $saved_newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($response->data['id']);
    expect($response->status)->equals(APIResponse::STATUS_OK);
    expect($saved_newsletter->schedule)->equals('* * * * *');
--- a/tests/unit/API/MP/APITest.php
+++ b/tests/unit/API/MP/APITest.php
@@ -408,15 +408,14 @@ class APITest extends \MailPoetTest {
  }

  function testItThrowsWhenConfirmationEmailFailsToSend() {
-    $API = Stub::makeEmptyExcept(
-      new \MailPoet\API\MP\v1\API(),
-      'addSubscriber',
-      array(
+    $API = new \MailPoet\API\MP\v1\API();
+    Mock::double($API, array(
        '_sendConfirmationEmail' => function ($subscriber) {
          $subscriber->setError('Big Error');
          return false;
-        },
-      ), $this);
+        }
+      )
+    );
    $segment = Segment::createOrUpdate(
      array(
        'name' => 'Default',
--- a/tests/unit/Cron/Workers/SchedulerTest.php
+++ b/tests/unit/Cron/Workers/SchedulerTest.php
@@ -77,7 +77,7 @@ class SchedulerTest extends \MailPoetTest {
    $newsletter_option_field =
      $this->_createNewsletterOptionField('intervalType', Newsletter::TYPE_WELCOME);
    $newsletter_option = $this->_createNewsletterOption($newsletter_option_field->id, $newsletter->id, 'immediately');
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -93,7 +93,7 @@ class SchedulerTest extends \MailPoetTest {
    $newsletter_option_field =
      $this->_createNewsletterOptionField('intervalType', Newsletter::TYPE_WELCOME);
    $newsletter_option = $this->_createNewsletterOption($newsletter_option_field->id, $newsletter->id, 'daily');
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -103,7 +103,7 @@ class SchedulerTest extends \MailPoetTest {
    $queue = $this->_createQueue($newsletter->id);
    $newsletter_option->value = 'daily';
    $newsletter_option->save();
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)->findOne($newsletter->id);
    expect($queue->scheduled_at)->null();
    $newsletter->schedule = '0 5 * * *'; // set it to daily at 5
    $scheduler->deleteQueueOrUpdateNextRunDate($queue, $newsletter);
@@ -123,7 +123,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id,
      $newsletter->id, 'author'
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -146,7 +146,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id,
      $newsletter->id, 'author'
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -168,7 +168,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id,
      $newsletter->id, 'author'
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -188,7 +188,7 @@ class SchedulerTest extends \MailPoetTest {
    $newsletter_option = $this->_createNewsletterOption(
      $newsletter_option_field->id, $newsletter->id,
      \MailPoet\Newsletter\Scheduler\Scheduler::WORDPRESS_ALL_ROLES);
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -292,7 +292,7 @@ class SchedulerTest extends \MailPoetTest {
    $newsletter = $this->_createNewsletter();
    $newsletter_option_field = $this->_createNewsletterOptionField('segment', Newsletter::TYPE_NOTIFICATION);
    $newsletter_option = $this->_createNewsletterOption($newsletter_option_field->id, $newsletter->id, $segment->id);
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -315,7 +315,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id, $newsletter->id,
      $segment->id
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -342,7 +342,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id, $newsletter->id,
      $segment->id
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -364,7 +364,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id, $newsletter->id,
      $segment->id
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($newsletter->id);
    $queue = $this->_createQueue($newsletter->id);
    $scheduler = new Scheduler();
@@ -426,7 +426,7 @@ class SchedulerTest extends \MailPoetTest {
      $newsletter_option_field->id, $newsletter->id,
      $segment->id
    );
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_NOTIFICATION)
      ->findOne($newsletter->id);
    $scheduler = new Scheduler();
    $finder = Mock::double('MailPoet\Segments\SubscribersFinder');
--- a/tests/unit/Models/NewsletterTest.php
+++ b/tests/unit/Models/NewsletterTest.php
@@ -273,7 +273,7 @@ class NewsletterTest extends \MailPoetTest {
  function testItCanHaveOptions() {
    $newsletter_options = array(
      'name' => 'Event',
-      'newsletter_type' => 'welcome',
+      'newsletter_type' => Newsletter::TYPE_WELCOME,
    );
    $option_field = NewsletterOptionField::create();
    $option_field->hydrate($newsletter_options);
@@ -283,7 +283,7 @@ class NewsletterTest extends \MailPoetTest {
    $association->option_field_id = $option_field->id;
    $association->value = 'list';
    $association->save();
-    $newsletter = Newsletter::filter('filterWithOptions')
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_WELCOME)
      ->findOne($this->newsletter->id);
    expect($newsletter->Event)->equals($association->value);
  }
@@ -871,7 +871,7 @@ class NewsletterTest extends \MailPoetTest {
    expect($newsletter->getMeta())->isEmpty();

    // if meta option exists, it should be returned as an array
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_AUTOMATIC)->findOne($newsletter->id);
    expect($newsletter->getMeta())->equals($meta);
  }

--- a/tests/unit/Newsletter/Scheduler/SchedulerTest.php
+++ b/tests/unit/Newsletter/Scheduler/SchedulerTest.php
@@ -513,7 +513,7 @@ class SchedulerTest extends \MailPoetTest {
        'afterTimeNumber' => 2
      )
    );
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_AUTOMATIC)->findOne($newsletter->id);
    $subscriber = Subscriber::create();
    $subscriber->hydrate(Fixtures::get('subscriber_template'));
    $subscriber->save();
@@ -544,7 +544,7 @@ class SchedulerTest extends \MailPoetTest {
        'afterTimeNumber' => 2
      )
    );
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_AUTOMATIC)->findOne($newsletter->id);
    $subscriber = Subscriber::create();
    $subscriber->hydrate(Fixtures::get('subscriber_template'));
    $subscriber->save();
@@ -567,7 +567,7 @@ class SchedulerTest extends \MailPoetTest {
        'afterTimeNumber' => 2
      )
    );
-    $newsletter = Newsletter::filter('filterWithOptions')->findOne($newsletter->id);
+    $newsletter = Newsletter::filter('filterWithOptions', Newsletter::TYPE_AUTOMATIC)->findOne($newsletter->id);

    Scheduler::createAutomaticEmailSendingTask($newsletter, $subscriber = null, $meta = null);
    // new scheduled task should be created