From 3cde437628e77cddb5932c4fb15a91ff68e7d0d6 Mon Sep 17 00:00:00 2001 From: Vlad Date: Thu, 9 Mar 2017 18:51:18 -0500 Subject: [PATCH] Adds enforcement of global subcriber subscription status and subscribption to segments to which newsletter is sent --- .../Workers/SendingQueue/SendingQueue.php | 16 ++-- .../Workers/SendingQueue/Tasks/Newsletter.php | 8 ++ lib/Models/Subscriber.php | 16 +++- .../Workers/SendingQueue/SendingQueueTest.php | 44 +++++++-- .../SendingQueue/Tasks/NewsletterTest.php | 13 +++ tests/unit/Models/SubscriberTest.php | 92 +++++++++++++++++++ 6 files changed, 172 insertions(+), 17 deletions(-) diff --git a/lib/Cron/Workers/SendingQueue/SendingQueue.php b/lib/Cron/Workers/SendingQueue/SendingQueue.php index c662dd20f6..00366b1226 100644 --- a/lib/Cron/Workers/SendingQueue/SendingQueue.php +++ b/lib/Cron/Workers/SendingQueue/SendingQueue.php @@ -6,9 +6,8 @@ use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask; use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; use MailPoet\Mailer\MailerLog; use MailPoet\Models\SendingQueue as SendingQueueModel; -use MailPoet\Models\StatisticsNewsletters as StatisticsNewslettersModel; use MailPoet\Models\Subscriber as SubscriberModel; -use MailPoet\Models\Subscriber; +use MailPoet\Models\StatisticsNewsletters as StatisticsNewslettersModel; if(!defined('ABSPATH')) exit; @@ -35,6 +34,8 @@ class SendingQueue { } // configure mailer $this->mailer_task->configureMailer($newsletter); + // get newsletter segments + $newsletter_segments_ids = $this->newsletter_task->getSegments($newsletter); // get subscribers $queue->subscribers = $queue->getSubscribers(); $subscriber_batches = array_chunk( @@ -42,13 +43,10 @@ class SendingQueue { self::BATCH_SIZE ); foreach($subscriber_batches as $subscribers_to_process_ids) { - $found_subscribers = SubscriberModel::whereIn('id', $subscribers_to_process_ids) - ->where('status', Subscriber::STATUS_SUBSCRIBED) - ->whereNull('deleted_at') - ->findMany(); - $found_subscribers_ids = array_map(function($subscriber) { - return $subscriber->id; - }, $found_subscribers); + $found_subscribers = SubscriberModel::findSubscribersInSegments( + $subscribers_to_process_ids, $newsletter_segments_ids + )->findMany(); + $found_subscribers_ids = SubscriberModel::extractSubscribersIds($found_subscribers); // if some subscribers weren't found, remove them from the processing list if(count($found_subscribers_ids) !== count($subscribers_to_process_ids)) { $subscibers_to_remove = array_diff( diff --git a/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php b/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php index 9cd1c94638..0245f96456 100644 --- a/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php +++ b/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php @@ -5,6 +5,7 @@ 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\Models\Newsletter as NewsletterModel; +use MailPoet\Models\NewsletterSegment as NewsletterSegmentModel; use MailPoet\Models\Setting; use MailPoet\Newsletter\Links\Links as NewsletterLinks; use MailPoet\Newsletter\Renderer\PostProcess\OpenTracking; @@ -100,4 +101,11 @@ class Newsletter { $newsletter->setStatus(NewsletterModel::STATUS_SENT); } } + + function getSegments($newsletter) { + $segments = NewsletterSegmentModel::where('newsletter_id', $newsletter->id) + ->select('segment_id') + ->findArray(); + return Helpers::flattenArray($segments); + } } \ No newline at end of file diff --git a/lib/Models/Subscriber.php b/lib/Models/Subscriber.php index 54c0293ba8..a91861f102 100644 --- a/lib/Models/Subscriber.php +++ b/lib/Models/Subscriber.php @@ -868,4 +868,18 @@ class Subscriber extends Model { ) ); } -} + + static function findSubscribersInSegments(array $subscribers_ids, array $segments_ids) { + return self::getSubscribedInSegments($segments_ids) + ->whereIn('subscribers.id', $subscribers_ids) + ->select('subscribers.*'); + } + + static function extractSubscribersIds(array $subscribers) { + return array_filter( + array_map(function($subscriber) { + return (!empty($subscriber->id)) ? $subscriber->id : false; + }, $subscribers) + ); + } +} \ No newline at end of file diff --git a/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php b/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php index 7aff6dc353..54e6cc28b8 100644 --- a/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php +++ b/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php @@ -10,9 +10,12 @@ use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterLink; use MailPoet\Models\NewsletterPost; +use MailPoet\Models\NewsletterSegment; +use MailPoet\Models\Segment; use MailPoet\Models\SendingQueue; use MailPoet\Models\StatisticsNewsletters; use MailPoet\Models\Subscriber; +use MailPoet\Models\SubscriberSegment; class SendingQueueTest extends MailPoetTest { function _before() { @@ -26,11 +29,22 @@ class SendingQueueTest extends MailPoetTest { $this->subscriber->last_name = 'Doe'; $this->subscriber->status = Subscriber::STATUS_SUBSCRIBED; $this->subscriber->save(); + $this->segment = Segment::create(); + $this->segment->name = 'segment'; + $this->segment->save(); + $this->subscriber_segment = SubscriberSegment::create(); + $this->subscriber_segment->subscriber_id = $this->subscriber->id; + $this->subscriber_segment->segment_id = $this->segment->id; + $this->subscriber_segment->save(); $this->newsletter = Newsletter::create(); $this->newsletter->type = Newsletter::TYPE_STANDARD; $this->newsletter->subject = Fixtures::get('newsletter_subject_template'); $this->newsletter->body = Fixtures::get('newsletter_body_template'); $this->newsletter->save(); + $this->newsletter_segment = NewsletterSegment::create(); + $this->newsletter_segment->newsletter_id = $this->newsletter->id; + $this->newsletter_segment->segment_id = $this->segment->id; + $this->newsletter_segment->save(); $this->queue = SendingQueue::create(); $this->queue->newsletter_id = $this->newsletter->id; $this->queue->subscribers = serialize( @@ -363,19 +377,14 @@ class SendingQueueTest extends MailPoetTest { expect((int)$updated_queue->count_total)->equals(0); } - function testItDoesNotSendToUnsubscribedSubscribers() { + function testItDoesNotSendToGloballyUnsubscribedSubscribers() { $sending_queue_worker = $this->sending_queue_worker; $sending_queue_worker->mailer_task = Stub::make( new MailerTask(), array('send' => function($newsletter, $subscriber) { return true; }) ); - // newsletter is sent to existing subscriber - $sending_queue_worker->process(); - $updated_queue = SendingQueue::findOne($this->queue->id); - expect((int)$updated_queue->count_total)->equals(1); - - // newsletter is not sent to trashed subscriber + // newsletter is not sent to globally unsubscribed subscriber $this->_after(); $this->_before(); $subscriber = $this->subscriber; @@ -386,12 +395,33 @@ class SendingQueueTest extends MailPoetTest { expect((int)$updated_queue->count_total)->equals(0); } + function testItDoesNotSendToSubscribersUnsubscribedFromSegments() { + $sending_queue_worker = $this->sending_queue_worker; + $sending_queue_worker->mailer_task = Stub::make( + new MailerTask(), + array('send' => function($newsletter, $subscriber) { return true; }) + ); + + // newsletter is not sent to subscriber unsubscribed from segment + $this->_after(); + $this->_before(); + $subscriber_segment = $this->subscriber_segment; + $subscriber_segment->status = Subscriber::STATUS_UNSUBSCRIBED; + $subscriber_segment->save(); + $sending_queue_worker->process(); + $updated_queue = SendingQueue::findOne($this->queue->id); + expect((int)$updated_queue->count_total)->equals(0); + } + function _after() { ORM::raw_execute('TRUNCATE ' . Subscriber::$_table); + ORM::raw_execute('TRUNCATE ' . SubscriberSegment::$_table); + ORM::raw_execute('TRUNCATE ' . Segment::$_table); ORM::raw_execute('TRUNCATE ' . SendingQueue::$_table); ORM::raw_execute('TRUNCATE ' . Newsletter::$_table); ORM::raw_execute('TRUNCATE ' . NewsletterLink::$_table); ORM::raw_execute('TRUNCATE ' . NewsletterPost::$_table); + ORM::raw_execute('TRUNCATE ' . NewsletterSegment::$_table); ORM::raw_execute('TRUNCATE ' . StatisticsNewsletters::$_table); } } \ No newline at end of file diff --git a/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php b/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php index 29fa39ffdd..0dabd266df 100644 --- a/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php +++ b/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php @@ -4,6 +4,7 @@ use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterLink; use MailPoet\Models\NewsletterPost; +use MailPoet\Models\NewsletterSegment; use MailPoet\Models\SendingQueue; use MailPoet\Models\Subscriber; use MailPoet\Router\Router; @@ -154,6 +155,18 @@ class NewsletterTaskTest extends MailPoetTest { ->notContains(Router::NAME . '&endpoint=track&action=click&data='); } + function testItGetsSegments() { + for($i = 1; $i<=3; $i++) { + $newsletter_segment = NewsletterSegment::create(); + $newsletter_segment->newsletter_id = $this->newsletter->id; + $newsletter_segment->segment_id = $i; + $newsletter_segment->save(); + } + expect($this->newsletter_task->getSegments($this->newsletter))->equals( + array(1,2,3) + ); + } + function _after() { ORM::raw_execute('TRUNCATE ' . Subscriber::$_table); ORM::raw_execute('TRUNCATE ' . Newsletter::$_table); diff --git a/tests/unit/Models/SubscriberTest.php b/tests/unit/Models/SubscriberTest.php index 6f8a69f3f2..4f4a7409f6 100644 --- a/tests/unit/Models/SubscriberTest.php +++ b/tests/unit/Models/SubscriberTest.php @@ -776,6 +776,98 @@ class SubscriberTest extends MailPoetTest { expect(Subscriber::findArray())->isEmpty(); } + function testItCanFindSubscribersInSegments() { + // create 3 subscribers, segments and subscriber-segment relations + $prepare_data = function() { + $this->_after(); + for($i = 1; $i <= 3; $i++) { + $subscriber[$i] = Subscriber::create(); + $subscriber[$i]->status = Subscriber::STATUS_SUBSCRIBED; + $subscriber[$i]->email = $i . '@test.com'; + $subscriber[$i]->first_name = 'first ' + $i; + $subscriber[$i]->last_name = 'last ' + $i; + $subscriber[$i]->save(); + $segment[$i] = Segment::create(); + $segment[$i]->name = 'segment ' + $i; + $segment[$i]->save(); + $subscriber_segment[$i] = SubscriberSegment::create(); + $subscriber_segment[$i]->subscriber_id = $subscriber[$i]->id; + $subscriber_segment[$i]->segment_id = $segment[$i]->id; + $subscriber_segment[$i]->save(); + } + return array( + $subscriber, + $segment, + $subscriber_segment + ); + }; + + // it should not find deleted and nonexistent subscribers + list($subscriber, $segment,) = $prepare_data(); + $subscriber[1]->deleted_at = date("Y-m-d H:i:s"); + $subscriber[1]->save(); + $subscriber[2]->delete(); + $subscribers = Subscriber::findSubscribersInSegments( + array( + $subscriber[1]->id, + $subscriber[2]->id, + $subscriber[3]->id + ), + array( + $segment[1]->id, + $segment[2]->id, + $segment[3]->id + ) + )->findMany(); + expect(Subscriber::extractSubscribersIds($subscribers))->equals( + array($subscriber[3]->id) + ); + + // it should not find subscribers with global unsubscribe status + list($subscriber, $segment,) = $prepare_data(); + $subscriber[2]->status = Subscriber::STATUS_UNSUBSCRIBED; + $subscriber[2]->save(); + $subscribers = Subscriber::findSubscribersInSegments( + array( + $subscriber[1]->id, + $subscriber[2]->id, + $subscriber[3]->id + ), + array( + $segment[1]->id, + $segment[2]->id, + $segment[3]->id + ) + )->findMany(); + expect(Subscriber::extractSubscribersIds($subscribers))->equals( + array( + $subscriber[1]->id, + $subscriber[3]->id + ) + ); + + // it should not find subscribers unsubscribed from segment or when segment doesn't exist + list($subscriber, $segment, $subscriber_segment) = $prepare_data(); + $subscriber_segment[3]->status = Subscriber::STATUS_UNSUBSCRIBED; + $subscriber_segment[3]->save(); + $subscriber_segment[2]->delete(); + $subscribers = Subscriber::findSubscribersInSegments( + array( + $subscriber[1]->id, + $subscriber[2]->id, + $subscriber[3]->id + ), + array( + $segment[1]->id, + $segment[2]->id, + $segment[3]->id + ) + )->findMany(); + expect(Subscriber::extractSubscribersIds($subscribers))->equals( + array($subscriber[1]->id) + ); + } + function _after() { ORM::raw_execute('TRUNCATE ' . Subscriber::$_table); ORM::raw_execute('TRUNCATE ' . Segment::$_table);