From 195efad742e33f54ffef923f9eeb5b1ea9bea619 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 10 Dec 2018 11:31:21 +0100 Subject: [PATCH 01/22] Add stats notifications settings [MAILPOET-1571] --- lib/Config/Populator.php | 9 +++++ views/settings.html | 12 +++++++ views/settings/basics.html | 69 +++++++++++++++++++++++++++++++------- 3 files changed, 77 insertions(+), 13 deletions(-) diff --git a/lib/Config/Populator.php b/lib/Config/Populator.php index 9150ad639e..43db70790a 100644 --- a/lib/Config/Populator.php +++ b/lib/Config/Populator.php @@ -194,6 +194,15 @@ class Populator { ]); } + $stats_notifications = Setting::getValue('stats_notifications'); + if(empty($stats_notifications)) { + $sender = Setting::getValue('sender', []); + Setting::setValue('stats_notifications', [ + 'enabled' => true, + 'address' => isset($sender['address'])? $sender['address'] : null, + ]); + } + // reset mailer log MailerLog::resetMailerLog(); } diff --git a/views/settings.html b/views/settings.html index af307c6f3c..0f177877f1 100644 --- a/views/settings.html +++ b/views/settings.html @@ -98,6 +98,17 @@ } else { $('#settings_subscriber_email_notification_error').hide(); } + console.log("whatever"); + // if new subscriber notification is enabled but sender is empty, show error + var stats_notifications_enabled = $('input[name="stats_notifications[enabled]"]:checked').val(), + stats_notifications_address = $('input[name="stats_notifications[address]"]').val().trim(); + if (stats_notifications_enabled && stats_notifications_address == '') { + $('#settings_stats_notifications_error').show(); + window.location.href = '#basics'; + errorFound = true; + } else { + $('#settings_stats_notifications_error').hide(); + } // stop processing if an error was found if (errorFound) { return false; @@ -182,6 +193,7 @@ $('#settings_re_captcha_tokens_error').hide(); $('#settings_subscriber_email_notification_error').hide(); + $('#settings_stats_notifications_error').hide(); function toggleLinuxCronSettings() { if ($('input[name="cron_trigger[method]"]:checked').val() === '<%= cron_trigger.linux_cron %>') { diff --git a/views/settings/basics.html b/views/settings/basics.html index 6442df58d9..626374f283 100644 --- a/views/settings/basics.html +++ b/views/settings/basics.html @@ -281,6 +281,48 @@ + + + + +

+ <%= __('Enter the email address that should receive your newsletter’s stats 24 hours after it has been sent.') %> + +

+ +   + +
+ +
+

+ <%= __('Please fill the email address.') %> +
+ @@ -296,9 +338,9 @@ type="radio" name="subscriber_email_notification[enabled]" value="1" - <% if(settings.subscriber_email_notification.enabled) %> - checked - <% endif %> + <% if(settings.subscriber_email_notification.enabled) %> + checked + <% endif %> /><%= __('Yes') %>   @@ -307,21 +349,22 @@ type="radio" name="subscriber_email_notification[enabled]" value="" - <% if not(settings.subscriber_email_notification.enabled) %> - checked - <% endif %> + <% if not(settings.subscriber_email_notification.enabled) %> + checked + <% endif %> /><%= __('No') %>
+ id="subscriber_email_notification[address]" + name="subscriber_email_notification[address]" + value="<%= settings.subscriber_email_notification.address %>" + placeholder="me@mydomain.com" />
-
- <%= __('Please fill the email address.') %> -
+
+ <%= __('Please fill the email address.') %> +
+ From 75761d57edc31c06c7d3cc2c30e88156c8592ebe Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 10 Dec 2018 15:30:09 +0100 Subject: [PATCH 02/22] Refactor Daemon to use DI [MAILPOET-1571] --- lib/Cron/Daemon.php | 27 +++++----- lib/Cron/Workers/WorkersFactory.php | 52 +++++++++++++++++++ .../integration/Cron/DaemonHttpRunnerTest.php | 34 ++++++------ tests/integration/Cron/DaemonTest.php | 8 +-- 4 files changed, 87 insertions(+), 34 deletions(-) create mode 100644 lib/Cron/Workers/WorkersFactory.php diff --git a/lib/Cron/Daemon.php b/lib/Cron/Daemon.php index fc0c86daa4..99f82d7aa0 100644 --- a/lib/Cron/Daemon.php +++ b/lib/Cron/Daemon.php @@ -1,20 +1,19 @@ timer = microtime(true); + $this->workers_factory = $workers_factory; } function run($settings_daemon_data) { @@ -35,32 +34,32 @@ class Daemon { } function executeScheduleWorker() { - $scheduler = new SchedulerWorker($this->timer); + $scheduler = $this->workers_factory->createScheduleWorker($this->timer); return $scheduler->process(); } function executeQueueWorker() { - $queue = new SendingQueueWorker(new SendingErrorHandler(), $this->timer); + $queue = $this->workers_factory->createQueueWorker($this->timer); return $queue->process(); } function executeSendingServiceKeyCheckWorker() { - $worker = new SendingServiceKeyCheckWorker($this->timer); + $worker = $this->workers_factory->createSendingServiceKeyCheckWorker($this->timer); return $worker->process(); } function executePremiumKeyCheckWorker() { - $worker = new PremiumKeyCheckWorker($this->timer); + $worker = $this->workers_factory->createPremiumKeyCheckWorker($this->timer); return $worker->process(); } function executeBounceWorker() { - $bounce = new BounceWorker($this->timer); + $bounce = $this->workers_factory->createBounceWorker($this->timer); return $bounce->process(); } function executeMigrationWorker() { - $migration = new MigrationWorker($this->timer); + $migration = $this->workers_factory->createMigrationWorker($this->timer); return $migration->process(); } diff --git a/lib/Cron/Workers/WorkersFactory.php b/lib/Cron/Workers/WorkersFactory.php new file mode 100644 index 0000000000..f1bf5949c7 --- /dev/null +++ b/lib/Cron/Workers/WorkersFactory.php @@ -0,0 +1,52 @@ +sending_error_handler = $sending_error_handler; + } + + /** @return SchedulerWorker */ + function createScheduleWorker($timer) { + return new SchedulerWorker($timer); + } + + /** @return SendingQueueWorker */ + function createQueueWorker($timer) { + return new SendingQueueWorker($this->sending_error_handler, $timer); + } + + /** @return SendingServiceKeyCheckWorker */ + function createSendingServiceKeyCheckWorker($timer) { + return new SendingServiceKeyCheckWorker($timer); + } + + /** @return PremiumKeyCheckWorker */ + function createPremiumKeyCheckWorker($timer) { + return new PremiumKeyCheckWorker($timer); + } + + /** @return BounceWorker */ + function createBounceWorker($timer) { + return new BounceWorker($timer); + } + + /** @return MigrationWorker */ + function createMigrationWorker($timer) { + return new MigrationWorker($timer); + } + +} diff --git a/tests/integration/Cron/DaemonHttpRunnerTest.php b/tests/integration/Cron/DaemonHttpRunnerTest.php index 3e37415e02..2efb6cb842 100644 --- a/tests/integration/Cron/DaemonHttpRunnerTest.php +++ b/tests/integration/Cron/DaemonHttpRunnerTest.php @@ -6,6 +6,8 @@ use Codeception\Stub\Expected; use MailPoet\Cron\CronHelper; use MailPoet\Cron\Daemon; use MailPoet\Cron\DaemonHttpRunner; +use MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler; +use MailPoet\Cron\Workers\WorkersFactory; use MailPoet\Models\Setting; class DaemonHttpRunnerTest extends \MailPoetTest { @@ -21,7 +23,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { function testItDoesNotRunWithoutRequestData() { $daemon = Stub::construct( - new DaemonHttpRunner(new Daemon()), + new DaemonHttpRunner(new Daemon(new WorkersFactory(new SendingErrorHandler()))), array(), array( 'abortWithError' => function($message) { @@ -34,7 +36,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { function testItDoesNotRunWhenThereIsInvalidOrMissingToken() { $daemon = Stub::construct( - new DaemonHttpRunner(new Daemon()), + new DaemonHttpRunner(new Daemon(new WorkersFactory(new SendingErrorHandler()))), array(), array( 'abortWithError' => function($message) { @@ -52,7 +54,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { $data = array( 'token' => 123 ); - $daemon = Stub::make(new Daemon(), array( + $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( 'executeScheduleWorker' => function() { throw new \Exception('Message'); }, @@ -72,7 +74,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { } function testItCanPauseExecution() { - $daemon = Stub::make(new Daemon(), array( + $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( 'executeScheduleWorker' => null, 'executeQueueWorker' => null, ), $this); @@ -93,7 +95,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { function testItTerminatesExecutionWhenDaemonIsDeleted() { - $daemon = Stub::make(new DaemonHttpRunner(new Daemon()), array( + $daemon = Stub::make(DaemonHttpRunner::class, array( 'executeScheduleWorker' => function() { Setting::deleteValue(CronHelper::DAEMON_SETTING); }, @@ -105,12 +107,12 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon()); + $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); $daemon->run($data); } function testItTerminatesExecutionWhenDaemonTokenChangesAndKeepsChangedToken() { - $daemon = Stub::make(new DaemonHttpRunner(new Daemon()), array( + $daemon = Stub::make(DaemonHttpRunner::class, array( 'executeScheduleWorker' => function() { Setting::setValue( CronHelper::DAEMON_SETTING, @@ -125,14 +127,14 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon()); + $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); $daemon->run($data); $data_after_run = Setting::getValue(CronHelper::DAEMON_SETTING); expect($data_after_run['token'], 567); } function testItTerminatesExecutionWhenDaemonIsDeactivated() { - $daemon = Stub::make(new DaemonHttpRunner(new Daemon()), [ + $daemon = Stub::make(DaemonHttpRunner::class, [ 'executeScheduleWorker' => null, 'executeQueueWorker' => null, 'pauseExecution' => null, @@ -143,12 +145,12 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'status' => CronHelper::DAEMON_STATUS_INACTIVE, ]; Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon()); + $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); $daemon->run($data); } function testItUpdatesDaemonTokenDuringExecution() { - $daemon_http_runner = Stub::make(new DaemonHttpRunner(new Daemon()), array( + $daemon_http_runner = Stub::make(DaemonHttpRunner::class, array( 'executeScheduleWorker' => null, 'executeQueueWorker' => null, 'pauseExecution' => null, @@ -158,14 +160,14 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon_http_runner->__construct(new Daemon()); + $daemon_http_runner->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); $daemon_http_runner->run($data); $updated_daemon = Setting::getValue(CronHelper::DAEMON_SETTING); expect($updated_daemon['token'])->equals($daemon_http_runner->token); } function testItUpdatesTimestampsDuringExecution() { - $daemon = Stub::make(new Daemon(), array( + $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( 'executeScheduleWorker' => function() { sleep(2); }, @@ -192,7 +194,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { function testItCanRun() { ignore_user_abort(0); expect(ignore_user_abort())->equals(0); - $daemon = Stub::make(new DaemonHttpRunner(new Daemon()), array( + $daemon = Stub::make(DaemonHttpRunner::class, array( 'pauseExecution' => null, // workers should be executed 'executeScheduleWorker' => Expected::exactly(1), @@ -204,13 +206,13 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon()); + $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); $daemon->run($data); expect(ignore_user_abort())->equals(1); } function testItRespondsToPingRequest() { - $daemon = Stub::make(new DaemonHttpRunner(new Daemon()), array( + $daemon = Stub::make(DaemonHttpRunner::class, array( 'terminateRequest' => Expected::exactly(1, function($message) { expect($message)->equals('pong'); }) diff --git a/tests/integration/Cron/DaemonTest.php b/tests/integration/Cron/DaemonTest.php index 1053478f03..1dfc4433f0 100644 --- a/tests/integration/Cron/DaemonTest.php +++ b/tests/integration/Cron/DaemonTest.php @@ -6,12 +6,14 @@ use Codeception\Stub\Expected; use MailPoet\Cron\CronHelper; use MailPoet\Cron\DaemonHttpRunner; use MailPoet\Cron\Daemon; +use MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler; +use MailPoet\Cron\Workers\WorkersFactory; use MailPoet\Models\Setting; class DaemonTest extends \MailPoetTest { function testItCanExecuteWorkers() { - $daemon = Stub::make(new Daemon(), array( + $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( 'executeScheduleWorker' => Expected::exactly(1), 'executeQueueWorker' => Expected::exactly(1), 'pauseExecution' => null, @@ -21,12 +23,11 @@ class DaemonTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct($data); $daemon->run([]); } function testItCanRun() { - $daemon = Stub::make(new Daemon(), array( + $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( 'pauseExecution' => null, // workers should be executed 'executeScheduleWorker' => Expected::exactly(1), @@ -38,7 +39,6 @@ class DaemonTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(); $daemon->run($data); } From 423341abb0f4c6b0a1a4671671c9e1eb317b1038 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 11 Dec 2018 11:24:04 +0100 Subject: [PATCH 03/22] Add stats notifications email scheduler [MAILPOET-1571] --- lib/Cron/Workers/StatsNotifications.php | 85 +++++++++++++++++ .../Cron/Workers/StatsNotificationsTest.php | 93 +++++++++++++++++++ 2 files changed, 178 insertions(+) create mode 100644 lib/Cron/Workers/StatsNotifications.php create mode 100644 tests/integration/Cron/Workers/StatsNotificationsTest.php diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications.php new file mode 100644 index 0000000000..03bf1aee55 --- /dev/null +++ b/lib/Cron/Workers/StatsNotifications.php @@ -0,0 +1,85 @@ +shouldSchedule($newsletter_id)) { + return false; + } + + $task = ScheduledTask::create(); + $task->type = self::TASK_TYPE; + $task->status = ScheduledTask::STATUS_SCHEDULED; + $task->scheduled_at = $this->getNextRunDate(); + $task->save(); + + $queue = SendingQueue::create(); + $queue->newsletter_id = $newsletter_id; + $queue->task_id = $task->id; + $queue->save(); + } + + function process() { + + } + + private function shouldSchedule($newsletter_id) { + if($this->isDisabled()) { + return false; + } + if($this->isTaskScheduled($newsletter_id)) { + return false; + } + return true; + } + + private function isDisabled() { + $settings = Setting::getValue(self::SETTINGS_KEY); + if(!is_array($settings)) { + return true; + } + if(!isset($settings['enabled'])) { + return true; + } + if(!isset($settings['address'])) { + return true; + } + if(empty(trim($settings['address']))) { + return true; + } + return !(bool)$settings['enabled']; + } + + private function isTaskScheduled($newsletter_id) { + $existing = ScheduledTask::table_alias('tasks') + ->join(SendingQueue::$_table, 'tasks.id = queues.task_id', 'queues') + ->where('tasks.type', self::TASK_TYPE) + ->where('queues.newsletter_id', $newsletter_id) + ->findMany(); + return (bool) $existing; + } + + private function getNextRunDate() { + $date = new Carbon(); + $date->addHours(self::HOURS_TO_SEND_AFTER_NEWSLETTER); + return $date; + } + +} diff --git a/tests/integration/Cron/Workers/StatsNotificationsTest.php b/tests/integration/Cron/Workers/StatsNotificationsTest.php new file mode 100644 index 0000000000..d6f94b205b --- /dev/null +++ b/tests/integration/Cron/Workers/StatsNotificationsTest.php @@ -0,0 +1,93 @@ +stats_notifications = new StatsNotifications(); + Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + 'enabled' => true, + 'address' => 'email@example.com' + ]); + } + + function testShouldSchedule() { + $newsletter_id = 5; + $this->stats_notifications->schedule($newsletter_id); + $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); + expect($queue)->isInstanceOf(SendingQueue::class); + $task = ScheduledTask::where('id', $queue->task_id)->findOne(); + expect($task)->isInstanceOf(ScheduledTask::class); + } + + function testShouldNotScheduleIfDisabled() { + $newsletter_id = 6; + Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + 'enabled' => false, + 'address' => 'email@example.com' + ]); + $this->stats_notifications->schedule($newsletter_id); + $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); + expect($queue)->isEmpty(); + } + + function testShouldNotScheduleIfSettingsMissing() { + $newsletter_id = 7; + Setting::setValue(StatsNotifications::SETTINGS_KEY, []); + $this->stats_notifications->schedule($newsletter_id); + $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); + expect($queue)->isEmpty(); + } + + function testShouldNotScheduleIfEmailIsMissing() { + $newsletter_id = 8; + Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + 'enabled' => true, + ]); + $this->stats_notifications->schedule($newsletter_id); + $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); + expect($queue)->isEmpty(); + } + + function testShouldNotScheduleIfEmailIsEmpty() { + $newsletter_id = 9; + Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + 'enabled' => true, + 'address' => ' ' + ]); + $this->stats_notifications->schedule($newsletter_id); + $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); + expect($queue)->isEmpty(); + } + + function testShouldNotScheduleIfAlreadyScheduled() { + $newsletter_id = 10; + $existing_task = ScheduledTask::createOrUpdate([ + 'type' => StatsNotifications::TASK_TYPE, + 'status' => ScheduledTask::STATUS_SCHEDULED, + 'scheduled_at' => '2017-01-02 12:13:14', + ]); + $existing_queue = SendingQueue::createOrUpdate([ + 'newsletter_id' => $newsletter_id, + 'task_id' => $existing_task->id, + ]); + $this->stats_notifications->schedule($newsletter_id); + $queues = SendingQueue::where('newsletter_id', $newsletter_id)->findMany(); + expect($queues)->count(1); + $tasks = ScheduledTask::where('id', $queues[0]->task_id)->findMany(); + expect($tasks)->count(1); + expect($existing_queue->id)->equals($queues[0]->id); + expect($existing_task->id)->equals($tasks[0]->id); + } + +} From 9080b5260ee9e6d9ca75d23374e344ffba5c775e Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 11 Dec 2018 13:23:28 +0100 Subject: [PATCH 04/22] Create a new table for stats notification [MAILPOET-1571] --- lib/Config/Database.php | 2 ++ lib/Config/Migrator.php | 13 +++++++++++++ lib/Cron/Workers/StatsNotifications.php | 13 ++++++++++++- lib/Models/StatsNotification.php | 9 +++++++++ 4 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 lib/Models/StatsNotification.php diff --git a/lib/Config/Database.php b/lib/Config/Database.php index 152885ee91..d5c70d1d34 100644 --- a/lib/Config/Database.php +++ b/lib/Config/Database.php @@ -86,6 +86,7 @@ class Database { $statistics_forms = Env::$db_prefix . 'statistics_forms'; $mapping_to_external_entities = Env::$db_prefix . 'mapping_to_external_entities'; $log = Env::$db_prefix . 'log'; + $stats_notifications = Env::$db_prefix . 'stats_notifications'; define('MP_SETTINGS_TABLE', $settings); define('MP_SEGMENTS_TABLE', $segments); @@ -112,6 +113,7 @@ class Database { define('MP_STATISTICS_FORMS_TABLE', $statistics_forms); define('MP_MAPPING_TO_EXTERNAL_ENTITIES_TABLE', $mapping_to_external_entities); define('MP_LOG_TABLE', $log); + define('MP_STATS_NOTIFICATIONS_TABLE', $stats_notifications); } } } diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index 2f195f0ec8..86982d02e7 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -18,6 +18,7 @@ class Migrator { 'settings', 'custom_fields', 'scheduled_tasks', + 'stats_notifications', 'scheduled_task_subscribers', 'sending_queues', 'subscribers', @@ -126,6 +127,18 @@ class Migrator { return $this->sqlify(__FUNCTION__, $attributes); } + function statsNotifications() { + $attributes = array( + 'id int(11) unsigned NOT NULL AUTO_INCREMENT,', + 'newsletter_id int(11) unsigned NOT NULL,', + 'task_id int(11) unsigned NOT NULL,', + 'PRIMARY KEY (id),', + 'KEY newsletter_id (newsletter_id),', + 'KEY task_id (task_id)', + ); + return $this->sqlify(__FUNCTION__, $attributes); + } + function scheduledTaskSubscribers() { $attributes = array( 'task_id int(11) unsigned NOT NULL,', diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications.php index 03bf1aee55..7f6125b0cd 100644 --- a/lib/Cron/Workers/StatsNotifications.php +++ b/lib/Cron/Workers/StatsNotifications.php @@ -6,7 +6,18 @@ use Carbon\Carbon; use MailPoet\Models\SendingQueue; use MailPoet\Models\ScheduledTask; use MailPoet\Models\Setting; -use MailPoet\Tasks\Sending; + + +/** + * TODO: + * - finish stats_notifications table, test if it is working and the migration is creating the table + * - remove all the SendingQueue from here + * - in schedule method add a row to stats_notifications table + * - when sending of post notification or a standard newsletter is finished call schedule + * - add processing of this task to Daemon + * - check JIRA what to do next and how to send the newsletter + * - see \MailPoet\Subscribers\NewSubscriberNotificationMailer how to send an email, now with DI everything should be easy + */ class StatsNotifications { diff --git a/lib/Models/StatsNotification.php b/lib/Models/StatsNotification.php new file mode 100644 index 0000000000..0c2c49522c --- /dev/null +++ b/lib/Models/StatsNotification.php @@ -0,0 +1,9 @@ + Date: Tue, 8 Jan 2019 11:32:43 +0100 Subject: [PATCH 05/22] Use a separate table instead of sending_queue [MAILPOET-1571] --- lib/Config/Migrator.php | 2 ++ lib/Cron/Workers/StatsNotifications.php | 15 +++++++-------- .../Cron/Workers/StatsNotificationsTest.php | 7 ++++--- views/settings.html | 2 -- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index 86982d02e7..2b9ab3e579 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -132,6 +132,8 @@ class Migrator { 'id int(11) unsigned NOT NULL AUTO_INCREMENT,', 'newsletter_id int(11) unsigned NOT NULL,', 'task_id int(11) unsigned NOT NULL,', + 'created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,', + 'updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,', 'PRIMARY KEY (id),', 'KEY newsletter_id (newsletter_id),', 'KEY task_id (task_id)', diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications.php index 7f6125b0cd..44aa065663 100644 --- a/lib/Cron/Workers/StatsNotifications.php +++ b/lib/Cron/Workers/StatsNotifications.php @@ -6,13 +6,12 @@ use Carbon\Carbon; use MailPoet\Models\SendingQueue; use MailPoet\Models\ScheduledTask; use MailPoet\Models\Setting; +use MailPoet\Models\StatsNotification; /** * TODO: * - finish stats_notifications table, test if it is working and the migration is creating the table - * - remove all the SendingQueue from here - * - in schedule method add a row to stats_notifications table * - when sending of post notification or a standard newsletter is finished call schedule * - add processing of this task to Daemon * - check JIRA what to do next and how to send the newsletter @@ -41,10 +40,10 @@ class StatsNotifications { $task->scheduled_at = $this->getNextRunDate(); $task->save(); - $queue = SendingQueue::create(); - $queue->newsletter_id = $newsletter_id; - $queue->task_id = $task->id; - $queue->save(); + $stats_notifications = StatsNotification::create(); + $stats_notifications->newsletter_id = $newsletter_id; + $stats_notifications->task_id = $task->id; + $stats_notifications->save(); } function process() { @@ -80,9 +79,9 @@ class StatsNotifications { private function isTaskScheduled($newsletter_id) { $existing = ScheduledTask::table_alias('tasks') - ->join(SendingQueue::$_table, 'tasks.id = queues.task_id', 'queues') + ->join(StatsNotification::$_table, 'tasks.id = notification.task_id', 'notification') ->where('tasks.type', self::TASK_TYPE) - ->where('queues.newsletter_id', $newsletter_id) + ->where('notification.newsletter_id', $newsletter_id) ->findMany(); return (bool) $existing; } diff --git a/tests/integration/Cron/Workers/StatsNotificationsTest.php b/tests/integration/Cron/Workers/StatsNotificationsTest.php index d6f94b205b..1774e67238 100644 --- a/tests/integration/Cron/Workers/StatsNotificationsTest.php +++ b/tests/integration/Cron/Workers/StatsNotificationsTest.php @@ -7,6 +7,7 @@ use MailPoet\Cron\Workers\StatsNotifications; use MailPoet\Models\ScheduledTask; use MailPoet\Models\SendingQueue; use MailPoet\Models\Setting; +use MailPoet\Models\StatsNotification; class StatsNotificationsTest extends \MailPoetTest { @@ -24,9 +25,9 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldSchedule() { $newsletter_id = 5; $this->stats_notifications->schedule($newsletter_id); - $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); - expect($queue)->isInstanceOf(SendingQueue::class); - $task = ScheduledTask::where('id', $queue->task_id)->findOne(); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isInstanceOf(StatsNotification::class); + $task = ScheduledTask::where('id', $notification->task_id)->findOne(); expect($task)->isInstanceOf(ScheduledTask::class); } diff --git a/views/settings.html b/views/settings.html index 0f177877f1..ff8221c744 100644 --- a/views/settings.html +++ b/views/settings.html @@ -98,8 +98,6 @@ } else { $('#settings_subscriber_email_notification_error').hide(); } - console.log("whatever"); - // if new subscriber notification is enabled but sender is empty, show error var stats_notifications_enabled = $('input[name="stats_notifications[enabled]"]:checked').val(), stats_notifications_address = $('input[name="stats_notifications[address]"]').val().trim(); if (stats_notifications_enabled && stats_notifications_address == '') { From 1d34613b1782824a2516b9f705ff6c9f5618f1b6 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 8 Jan 2019 13:31:05 +0100 Subject: [PATCH 06/22] Schedule a notification [MAILPOET-1571] --- .../Workers/SendingQueue/SendingQueue.php | 8 ++++- lib/Cron/Workers/StatsNotifications.php | 14 ++++---- lib/DI/ContainerConfigurator.php | 2 ++ .../Workers/SendingQueue/SendingQueueTest.php | 36 +++++++++++++------ .../Cron/Workers/StatsNotificationsTest.php | 20 +++++++---- 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/lib/Cron/Workers/SendingQueue/SendingQueue.php b/lib/Cron/Workers/SendingQueue/SendingQueue.php index f01bf4197a..3ee60e1e18 100644 --- a/lib/Cron/Workers/SendingQueue/SendingQueue.php +++ b/lib/Cron/Workers/SendingQueue/SendingQueue.php @@ -5,6 +5,7 @@ use MailPoet\Cron\CronHelper; use MailPoet\Cron\Workers\SendingQueue\Tasks\Links; use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask; use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; +use MailPoet\Cron\Workers\StatsNotifications; use MailPoet\Logging\Logger; use MailPoet\Mailer\MailerError; use MailPoet\Mailer\MailerLog; @@ -25,11 +26,15 @@ class SendingQueue { const BATCH_SIZE = 20; const TASK_BATCH_SIZE = 5; + /** @var StatsNotifications */ + public $stats_notifications_worker; + /** @var SendingErrorHandler */ private $error_handler; - function __construct(SendingErrorHandler $error_handler, $timer = false, $mailer_task = false, $newsletter_task = false) { + function __construct(SendingErrorHandler $error_handler, StatsNotifications $stats_notifications_worker, $timer = false, $mailer_task = false, $newsletter_task = false) { $this->error_handler = $error_handler; + $this->stats_notifications_worker = $stats_notifications_worker; $this->mailer_task = ($mailer_task) ? $mailer_task : new MailerTask(); $this->newsletter_task = ($newsletter_task) ? $newsletter_task : new NewsletterTask(); $this->timer = ($timer) ? $timer : microtime(true); @@ -98,6 +103,7 @@ class SendingQueue { ); if($queue->status === ScheduledTaskModel::STATUS_COMPLETED) { $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); + $this->stats_notifications_worker->schedule($newsletter); } $this->enforceSendingAndExecutionLimits(); } diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications.php index 44aa065663..638ee9cde7 100644 --- a/lib/Cron/Workers/StatsNotifications.php +++ b/lib/Cron/Workers/StatsNotifications.php @@ -3,6 +3,7 @@ namespace MailPoet\Cron\Workers; use Carbon\Carbon; +use MailPoet\Models\Newsletter; use MailPoet\Models\SendingQueue; use MailPoet\Models\ScheduledTask; use MailPoet\Models\Setting; @@ -11,8 +12,7 @@ use MailPoet\Models\StatsNotification; /** * TODO: - * - finish stats_notifications table, test if it is working and the migration is creating the table - * - when sending of post notification or a standard newsletter is finished call schedule + * - only schedule for post notification and standard, need to do check here * - add processing of this task to Daemon * - check JIRA what to do next and how to send the newsletter * - see \MailPoet\Subscribers\NewSubscriberNotificationMailer how to send an email, now with DI everything should be easy @@ -29,8 +29,8 @@ class StatsNotifications { */ const HOURS_TO_SEND_AFTER_NEWSLETTER = 24; - function schedule($newsletter_id) { - if(!$this->shouldSchedule($newsletter_id)) { + function schedule(Newsletter $newsletter) { + if(!$this->shouldSchedule($newsletter)) { return false; } @@ -41,7 +41,7 @@ class StatsNotifications { $task->save(); $stats_notifications = StatsNotification::create(); - $stats_notifications->newsletter_id = $newsletter_id; + $stats_notifications->newsletter_id = $newsletter->id; $stats_notifications->task_id = $task->id; $stats_notifications->save(); } @@ -50,11 +50,11 @@ class StatsNotifications { } - private function shouldSchedule($newsletter_id) { + private function shouldSchedule(Newsletter $newsletter) { if($this->isDisabled()) { return false; } - if($this->isTaskScheduled($newsletter_id)) { + if($this->isTaskScheduled($newsletter->id)) { return false; } return true; diff --git a/lib/DI/ContainerConfigurator.php b/lib/DI/ContainerConfigurator.php index f865a2a4c8..974a31ab7e 100644 --- a/lib/DI/ContainerConfigurator.php +++ b/lib/DI/ContainerConfigurator.php @@ -54,6 +54,8 @@ class ContainerConfigurator implements IContainerConfigurator { // Cron $container->autowire(\MailPoet\Cron\Daemon::class)->setPublic(true); $container->autowire(\MailPoet\Cron\DaemonHttpRunner::class)->setPublic(true); + $container->autowire(\MailPoet\Cron\Workers\WorkersFactory::class)->setPublic(true); + $container->autowire(\MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler::class)->setPublic(true); // Listing $container->autowire(\MailPoet\Listing\BulkActionController::class)->setPublic(true); $container->autowire(\MailPoet\Listing\Handler::class)->setPublic(true); diff --git a/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php b/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php index 80cc27d515..a52e3f831e 100644 --- a/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php +++ b/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php @@ -12,6 +12,7 @@ use MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler; use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker; use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask; use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; +use MailPoet\Cron\Workers\StatsNotifications; use MailPoet\Mailer\MailerLog; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterLink; @@ -36,6 +37,9 @@ class SendingQueueTest extends \MailPoetTest { /** @var SendingErrorHandler */ private $sending_error_handler; + /** @var StatsNotifications */ + private $stats_notifications_worker; + function _before() { $wp_users = get_users(); wp_set_current_user($wp_users[0]->ID); @@ -76,7 +80,8 @@ class SendingQueueTest extends \MailPoetTest { $this->newsletter_link->hash = 'abcde'; $this->newsletter_link->save(); $this->sending_error_handler = new SendingErrorHandler(); - $this->sending_queue_worker = new SendingQueueWorker($this->sending_error_handler); + $this->stats_notifications_worker = new StatsNotifications(); + $this->sending_queue_worker = new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker); } private function getDirectUnsubscribeURL() { @@ -106,20 +111,20 @@ class SendingQueueTest extends \MailPoetTest { // constructor accepts timer argument $timer = microtime(true) - 5; - $sending_queue_worker = new SendingQueueWorker($this->sending_error_handler, $timer); + $sending_queue_worker = new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker, $timer); expect($sending_queue_worker->timer)->equals($timer); } function testItEnforcesExecutionLimitsBeforeQueueProcessing() { $sending_queue_worker = Stub::make( - new SendingQueueWorker($this->sending_error_handler), + new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker), array( 'processQueue' => Expected::never(), 'enforceSendingAndExecutionLimits' => Expected::exactly(1, function() { throw new \Exception(); }) ), $this); - $sending_queue_worker->__construct($this->sending_error_handler); + $sending_queue_worker->__construct($this->sending_error_handler, $this->stats_notifications_worker); try { $sending_queue_worker->process(); self::fail('Execution limits function was not called.'); @@ -130,12 +135,13 @@ class SendingQueueTest extends \MailPoetTest { function testItEnforcesExecutionLimitsAfterSendingWhenQueueStatusIsNotSetToComplete() { $sending_queue_worker = Stub::make( - new SendingQueueWorker($this->sending_error_handler), + new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker), array( 'enforceSendingAndExecutionLimits' => Expected::exactly(1) ), $this); $sending_queue_worker->__construct( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -164,12 +170,13 @@ class SendingQueueTest extends \MailPoetTest { $queue = $this->queue; $queue->status = SendingQueue::STATUS_COMPLETED; $sending_queue_worker = Stub::make( - new SendingQueueWorker($this->sending_error_handler), + new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker), array( 'enforceSendingAndExecutionLimits' => Expected::never() ), $this); $sending_queue_worker->__construct( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -193,7 +200,7 @@ class SendingQueueTest extends \MailPoetTest { function testItEnforcesExecutionLimitsAfterQueueProcessing() { $sending_queue_worker = Stub::make( - new SendingQueueWorker($this->sending_error_handler), + new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker), array( 'processQueue' => function() { // this function returns a queue object @@ -201,7 +208,7 @@ class SendingQueueTest extends \MailPoetTest { }, 'enforceSendingAndExecutionLimits' => Expected::exactly(2) ), $this); - $sending_queue_worker->__construct($this->sending_error_handler); + $sending_queue_worker->__construct($this->sending_error_handler, $this->stats_notifications_worker); $sending_queue_worker->process(); } @@ -225,6 +232,7 @@ class SendingQueueTest extends \MailPoetTest { $directUnsubscribeURL = $this->getDirectUnsubscribeURL(); $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -246,6 +254,7 @@ class SendingQueueTest extends \MailPoetTest { $trackedUnsubscribeURL = $this->getTrackedUnsubscribeURL(); $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -265,6 +274,7 @@ class SendingQueueTest extends \MailPoetTest { function testItCanProcessSubscribersOneByOne() { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -309,6 +319,7 @@ class SendingQueueTest extends \MailPoetTest { function testItCanProcessSubscribersInBulk() { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -356,6 +367,7 @@ class SendingQueueTest extends \MailPoetTest { function testItProcessesStandardNewsletters() { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -410,6 +422,7 @@ class SendingQueueTest extends \MailPoetTest { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::makeEmpty(new MailerTask(), array(), $this) ); @@ -425,6 +438,7 @@ class SendingQueueTest extends \MailPoetTest { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -591,9 +605,10 @@ class SendingQueueTest extends \MailPoetTest { 'updateProcessedSubscribers' => false )); $sending_task->id = 100; - $sending_queue_worker = Stub::make(new SendingQueueWorker($this->sending_error_handler)); + $sending_queue_worker = Stub::make(new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker)); $sending_queue_worker->__construct( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -627,6 +642,7 @@ class SendingQueueTest extends \MailPoetTest { function testItDoesNotUpdateNewsletterHashDuringSending() { $sending_queue_worker = new SendingQueueWorker( $this->sending_error_handler, + $this->stats_notifications_worker, $timer = false, Stub::make( new MailerTask(), @@ -650,7 +666,7 @@ class SendingQueueTest extends \MailPoetTest { return $custom_batch_size_value; }; Hooks::addFilter('mailpoet_cron_worker_sending_queue_batch_size', $filter); - $sending_queue_worker = new SendingQueueWorker($this->sending_error_handler); + $sending_queue_worker = new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker); expect($sending_queue_worker->batch_size)->equals($custom_batch_size_value); Hooks::removeFilter('mailpoet_cron_worker_sending_queue_batch_size', $filter); } diff --git a/tests/integration/Cron/Workers/StatsNotificationsTest.php b/tests/integration/Cron/Workers/StatsNotificationsTest.php index 1774e67238..d8376d2f88 100644 --- a/tests/integration/Cron/Workers/StatsNotificationsTest.php +++ b/tests/integration/Cron/Workers/StatsNotificationsTest.php @@ -2,8 +2,8 @@ namespace MailPoet\Test\Cron\Workers; -use Carbon\Carbon; use MailPoet\Cron\Workers\StatsNotifications; +use MailPoet\Models\Newsletter; use MailPoet\Models\ScheduledTask; use MailPoet\Models\SendingQueue; use MailPoet\Models\Setting; @@ -24,7 +24,8 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldSchedule() { $newsletter_id = 5; - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); expect($notification)->isInstanceOf(StatsNotification::class); $task = ScheduledTask::where('id', $notification->task_id)->findOne(); @@ -37,7 +38,8 @@ class StatsNotificationsTest extends \MailPoetTest { 'enabled' => false, 'address' => 'email@example.com' ]); - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); expect($queue)->isEmpty(); } @@ -45,7 +47,8 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfSettingsMissing() { $newsletter_id = 7; Setting::setValue(StatsNotifications::SETTINGS_KEY, []); - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); expect($queue)->isEmpty(); } @@ -55,7 +58,8 @@ class StatsNotificationsTest extends \MailPoetTest { Setting::setValue(StatsNotifications::SETTINGS_KEY, [ 'enabled' => true, ]); - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); expect($queue)->isEmpty(); } @@ -66,7 +70,8 @@ class StatsNotificationsTest extends \MailPoetTest { 'enabled' => true, 'address' => ' ' ]); - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); expect($queue)->isEmpty(); } @@ -82,7 +87,8 @@ class StatsNotificationsTest extends \MailPoetTest { 'newsletter_id' => $newsletter_id, 'task_id' => $existing_task->id, ]); - $this->stats_notifications->schedule($newsletter_id); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $this->stats_notifications->schedule($newsletter); $queues = SendingQueue::where('newsletter_id', $newsletter_id)->findMany(); expect($queues)->count(1); $tasks = ScheduledTask::where('id', $queues[0]->task_id)->findMany(); From 6452e834762a4c50d1a19ad2ff57539b356f449d Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 8 Jan 2019 15:53:47 +0100 Subject: [PATCH 07/22] Only schedule notification for standard newsletters [MAILPOET-1571] --- lib/Config/Migrator.php | 6 +-- lib/Cron/Workers/StatsNotifications.php | 7 +-- lib/Cron/Workers/WorkersFactory.php | 6 ++- lib/Models/StatsNotification.php | 24 ++++++++++ .../Cron/Workers/StatsNotificationsTest.php | 47 +++++++++++-------- 5 files changed, 63 insertions(+), 27 deletions(-) diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index 2b9ab3e579..9dab80e1bb 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -132,10 +132,10 @@ class Migrator { 'id int(11) unsigned NOT NULL AUTO_INCREMENT,', 'newsletter_id int(11) unsigned NOT NULL,', 'task_id int(11) unsigned NOT NULL,', - 'created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,', + 'created_at TIMESTAMP NULL,', 'updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,', - 'PRIMARY KEY (id),', - 'KEY newsletter_id (newsletter_id),', + 'PRIMARY KEY (id),', + 'UNIQUE KEY newsletter_id_task_id (newsletter_id, task_id),', 'KEY task_id (task_id)', ); return $this->sqlify(__FUNCTION__, $attributes); diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications.php index 638ee9cde7..c408c56246 100644 --- a/lib/Cron/Workers/StatsNotifications.php +++ b/lib/Cron/Workers/StatsNotifications.php @@ -9,10 +9,8 @@ use MailPoet\Models\ScheduledTask; use MailPoet\Models\Setting; use MailPoet\Models\StatsNotification; - /** * TODO: - * - only schedule for post notification and standard, need to do check here * - add processing of this task to Daemon * - check JIRA what to do next and how to send the newsletter * - see \MailPoet\Subscribers\NewSubscriberNotificationMailer how to send an email, now with DI everything should be easy @@ -57,6 +55,9 @@ class StatsNotifications { if($this->isTaskScheduled($newsletter->id)) { return false; } + if(($newsletter->type !== Newsletter::TYPE_NOTIFICATION) && ($newsletter->type !== Newsletter::TYPE_STANDARD)) { + return false; + } return true; } @@ -83,7 +84,7 @@ class StatsNotifications { ->where('tasks.type', self::TASK_TYPE) ->where('notification.newsletter_id', $newsletter_id) ->findMany(); - return (bool) $existing; + return (bool)$existing; } private function getNextRunDate() { diff --git a/lib/Cron/Workers/WorkersFactory.php b/lib/Cron/Workers/WorkersFactory.php index f1bf5949c7..84ec19e9cd 100644 --- a/lib/Cron/Workers/WorkersFactory.php +++ b/lib/Cron/Workers/WorkersFactory.php @@ -26,7 +26,11 @@ class WorkersFactory { /** @return SendingQueueWorker */ function createQueueWorker($timer) { - return new SendingQueueWorker($this->sending_error_handler, $timer); + return new SendingQueueWorker($this->sending_error_handler, $this->createStatsNotificationsWorker(), $timer); + } + + function createStatsNotificationsWorker() { + return new StatsNotifications(); } /** @return SendingServiceKeyCheckWorker */ diff --git a/lib/Models/StatsNotification.php b/lib/Models/StatsNotification.php index 0c2c49522c..8a162bf85f 100644 --- a/lib/Models/StatsNotification.php +++ b/lib/Models/StatsNotification.php @@ -5,5 +5,29 @@ namespace MailPoet\Models; class StatsNotification extends Model { public static $_table = MP_STATS_NOTIFICATIONS_TABLE; + /** @return StatsNotification */ + static function createOrUpdate($data = array()) { + $model = null; + + if(isset($data['id']) && (int)$data['id'] > 0) { + $model = static::findOne((int)$data['id']); + } + + if(!$model && isset($data['task_id']) && $data['newsletter_id']) { + $model = self::where('newsletter_id', $data['newsletter_id']) + ->where('task_id', $data['task_id']) + ->findOne(); + } + + if(!$model) { + $model = static::create(); + $model->hydrate($data); + } else { + unset($data['id']); + $model->set($data); + } + + return $model->save(); + } } diff --git a/tests/integration/Cron/Workers/StatsNotificationsTest.php b/tests/integration/Cron/Workers/StatsNotificationsTest.php index d8376d2f88..6642cafa97 100644 --- a/tests/integration/Cron/Workers/StatsNotificationsTest.php +++ b/tests/integration/Cron/Workers/StatsNotificationsTest.php @@ -5,7 +5,6 @@ namespace MailPoet\Test\Cron\Workers; use MailPoet\Cron\Workers\StatsNotifications; use MailPoet\Models\Newsletter; use MailPoet\Models\ScheduledTask; -use MailPoet\Models\SendingQueue; use MailPoet\Models\Setting; use MailPoet\Models\StatsNotification; @@ -24,7 +23,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldSchedule() { $newsletter_id = 5; - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); expect($notification)->isInstanceOf(StatsNotification::class); @@ -38,19 +37,19 @@ class StatsNotificationsTest extends \MailPoetTest { 'enabled' => false, 'address' => 'email@example.com' ]); - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); - $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); - expect($queue)->isEmpty(); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); } function testShouldNotScheduleIfSettingsMissing() { $newsletter_id = 7; Setting::setValue(StatsNotifications::SETTINGS_KEY, []); - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); - $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); - expect($queue)->isEmpty(); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); } function testShouldNotScheduleIfEmailIsMissing() { @@ -58,10 +57,10 @@ class StatsNotificationsTest extends \MailPoetTest { Setting::setValue(StatsNotifications::SETTINGS_KEY, [ 'enabled' => true, ]); - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); - $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); - expect($queue)->isEmpty(); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); } function testShouldNotScheduleIfEmailIsEmpty() { @@ -70,10 +69,10 @@ class StatsNotificationsTest extends \MailPoetTest { 'enabled' => true, 'address' => ' ' ]); - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); - $queue = SendingQueue::where('newsletter_id', $newsletter_id)->findOne(); - expect($queue)->isEmpty(); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); } function testShouldNotScheduleIfAlreadyScheduled() { @@ -83,18 +82,26 @@ class StatsNotificationsTest extends \MailPoetTest { 'status' => ScheduledTask::STATUS_SCHEDULED, 'scheduled_at' => '2017-01-02 12:13:14', ]); - $existing_queue = SendingQueue::createOrUpdate([ + $existing_notification = StatsNotification::createOrUpdate([ 'newsletter_id' => $newsletter_id, 'task_id' => $existing_task->id, ]); - $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id]); + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); - $queues = SendingQueue::where('newsletter_id', $newsletter_id)->findMany(); - expect($queues)->count(1); - $tasks = ScheduledTask::where('id', $queues[0]->task_id)->findMany(); + $notifications = StatsNotification::where('newsletter_id', $newsletter_id)->findMany(); + expect($notifications)->count(1); + $tasks = ScheduledTask::where('id', $notifications[0]->task_id)->findMany(); expect($tasks)->count(1); - expect($existing_queue->id)->equals($queues[0]->id); + expect($existing_notification->id)->equals($notifications[0]->id); expect($existing_task->id)->equals($tasks[0]->id); } + function testShouldNotScheduleIfInvalidType() { + $newsletter_id = 11; + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, Newsletter::TYPE_WELCOME]); + $this->stats_notifications->schedule($newsletter); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); + } + } From 96f2f79d48fc71b056c92353949253367ae55c59 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Wed, 9 Jan 2019 10:28:50 +0100 Subject: [PATCH 08/22] Refactor Stats notifications to spearate classes [MAILPOET-1571] --- lib/Cron/Daemon.php | 6 ++ .../Workers/SendingQueue/SendingQueue.php | 12 +-- .../Scheduler.php} | 25 ++---- .../Workers/StatsNotifications/Worker.php | 79 +++++++++++++++++++ lib/Cron/Workers/WorkersFactory.php | 14 +++- .../Workers/SendingQueue/SendingQueueTest.php | 6 +- .../SchedulerTest.php} | 21 +++-- 7 files changed, 120 insertions(+), 43 deletions(-) rename lib/Cron/Workers/{StatsNotifications.php => StatsNotifications/Scheduler.php} (76%) create mode 100644 lib/Cron/Workers/StatsNotifications/Worker.php rename tests/integration/Cron/Workers/{StatsNotificationsTest.php => StatsNotifications/SchedulerTest.php} (86%) diff --git a/lib/Cron/Daemon.php b/lib/Cron/Daemon.php index 99f82d7aa0..2c87b00169 100644 --- a/lib/Cron/Daemon.php +++ b/lib/Cron/Daemon.php @@ -23,6 +23,7 @@ class Daemon { $this->executeMigrationWorker(); $this->executeScheduleWorker(); $this->executeQueueWorker(); + $this->executeStatsNotificationsWorker(); $this->executeSendingServiceKeyCheckWorker(); $this->executePremiumKeyCheckWorker(); $this->executeBounceWorker(); @@ -43,6 +44,11 @@ class Daemon { return $queue->process(); } + function executeStatsNotificationsWorker() { + $worker = $this->workers_factory->createStatsNotificationsWorker($this->timer); + return $worker->process(); + } + function executeSendingServiceKeyCheckWorker() { $worker = $this->workers_factory->createSendingServiceKeyCheckWorker($this->timer); return $worker->process(); diff --git a/lib/Cron/Workers/SendingQueue/SendingQueue.php b/lib/Cron/Workers/SendingQueue/SendingQueue.php index 3ee60e1e18..39f5e43e87 100644 --- a/lib/Cron/Workers/SendingQueue/SendingQueue.php +++ b/lib/Cron/Workers/SendingQueue/SendingQueue.php @@ -5,7 +5,7 @@ use MailPoet\Cron\CronHelper; use MailPoet\Cron\Workers\SendingQueue\Tasks\Links; use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask; use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; -use MailPoet\Cron\Workers\StatsNotifications; +use MailPoet\Cron\Workers\StatsNotifications\Scheduler as StatsNotificationsScheduler; use MailPoet\Logging\Logger; use MailPoet\Mailer\MailerError; use MailPoet\Mailer\MailerLog; @@ -26,15 +26,15 @@ class SendingQueue { const BATCH_SIZE = 20; const TASK_BATCH_SIZE = 5; - /** @var StatsNotifications */ - public $stats_notifications_worker; + /** @var StatsNotificationsScheduler */ + public $stats_notifications_scheduler; /** @var SendingErrorHandler */ private $error_handler; - function __construct(SendingErrorHandler $error_handler, StatsNotifications $stats_notifications_worker, $timer = false, $mailer_task = false, $newsletter_task = false) { + function __construct(SendingErrorHandler $error_handler, StatsNotificationsScheduler $stats_notifications_scheduler, $timer = false, $mailer_task = false, $newsletter_task = false) { $this->error_handler = $error_handler; - $this->stats_notifications_worker = $stats_notifications_worker; + $this->stats_notifications_scheduler = $stats_notifications_scheduler; $this->mailer_task = ($mailer_task) ? $mailer_task : new MailerTask(); $this->newsletter_task = ($newsletter_task) ? $newsletter_task : new NewsletterTask(); $this->timer = ($timer) ? $timer : microtime(true); @@ -103,7 +103,7 @@ class SendingQueue { ); if($queue->status === ScheduledTaskModel::STATUS_COMPLETED) { $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); - $this->stats_notifications_worker->schedule($newsletter); + $this->stats_notifications_scheduler->schedule($newsletter); } $this->enforceSendingAndExecutionLimits(); } diff --git a/lib/Cron/Workers/StatsNotifications.php b/lib/Cron/Workers/StatsNotifications/Scheduler.php similarity index 76% rename from lib/Cron/Workers/StatsNotifications.php rename to lib/Cron/Workers/StatsNotifications/Scheduler.php index c408c56246..a04c5e441c 100644 --- a/lib/Cron/Workers/StatsNotifications.php +++ b/lib/Cron/Workers/StatsNotifications/Scheduler.php @@ -1,25 +1,14 @@ type = self::TASK_TYPE; + $task->type = Worker::TASK_TYPE; $task->status = ScheduledTask::STATUS_SCHEDULED; $task->scheduled_at = $this->getNextRunDate(); $task->save(); @@ -44,10 +33,6 @@ class StatsNotifications { $stats_notifications->save(); } - function process() { - - } - private function shouldSchedule(Newsletter $newsletter) { if($this->isDisabled()) { return false; @@ -62,7 +47,7 @@ class StatsNotifications { } private function isDisabled() { - $settings = Setting::getValue(self::SETTINGS_KEY); + $settings = Setting::getValue(Worker::SETTINGS_KEY); if(!is_array($settings)) { return true; } @@ -81,7 +66,7 @@ class StatsNotifications { private function isTaskScheduled($newsletter_id) { $existing = ScheduledTask::table_alias('tasks') ->join(StatsNotification::$_table, 'tasks.id = notification.task_id', 'notification') - ->where('tasks.type', self::TASK_TYPE) + ->where('tasks.type', Worker::TASK_TYPE) ->where('notification.newsletter_id', $newsletter_id) ->findMany(); return (bool)$existing; diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php new file mode 100644 index 0000000000..44a9152feb --- /dev/null +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -0,0 +1,79 @@ +timer = $timer ?: microtime(true); + $this->renderer = $renderer; + $this->mailer = $mailer; + } + + /** @throws \Exception */ + function process() { + $settings = Setting::getValue(self::SETTINGS_KEY); + try { + $this->mailer->getSenderNameAndAddress($this->constructSenderEmail()); + $this->mailer->send($this->constructNewsletter(), $settings['address']); + } catch(\Exception $e) { + if(WP_DEBUG) { + throw $e; + } + } + + CronHelper::enforceExecutionLimit($this->timer); + } + + private function constructSenderEmail() { + $url_parts = parse_url(home_url()); + $site_name = strtolower($url_parts['host']); + if(strpos($site_name, 'www.') === 0) { + $site_name = substr($site_name, 4); + } + return [ + 'address' => self::SENDER_EMAIL_PREFIX . $site_name, + 'name' => self::SENDER_EMAIL_PREFIX . $site_name, + ]; + } + + private function constructNewsletter() { + $context = [ + 'link_settings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings'), + 'link_premium' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-premium'), + ]; + return [ + 'subject' => sprintf(__('New subscriber to ', 'mailpoet')), + 'body' => [ + 'html' => $this->renderer->render('emails/newSubscriberNotification.html', $context), + 'text' => $this->renderer->render('emails/newSubscriberNotification.txt', $context), + ], + ]; + } + +} diff --git a/lib/Cron/Workers/WorkersFactory.php b/lib/Cron/Workers/WorkersFactory.php index 84ec19e9cd..d5dc4b3ba5 100644 --- a/lib/Cron/Workers/WorkersFactory.php +++ b/lib/Cron/Workers/WorkersFactory.php @@ -2,9 +2,11 @@ namespace MailPoet\Cron\Workers; +use MailPoet\Config\Renderer; use MailPoet\Cron\Workers\Scheduler as SchedulerWorker; use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker; use MailPoet\Cron\Workers\SendingQueue\Migration as MigrationWorker; +use MailPoet\Cron\Workers\StatsNotifications\Worker as StatsNotificationsWorker; use MailPoet\Cron\Workers\Bounce as BounceWorker; use MailPoet\Cron\Workers\KeyCheck\PremiumKeyCheck as PremiumKeyCheckWorker; use MailPoet\Cron\Workers\KeyCheck\SendingServiceKeyCheck as SendingServiceKeyCheckWorker; @@ -26,11 +28,17 @@ class WorkersFactory { /** @return SendingQueueWorker */ function createQueueWorker($timer) { - return new SendingQueueWorker($this->sending_error_handler, $this->createStatsNotificationsWorker(), $timer); + return new SendingQueueWorker($this->sending_error_handler, $this->createStatsNotificationsWorker($timer), $timer); } - function createStatsNotificationsWorker() { - return new StatsNotifications(); + function createStatsNotificationsWorker($timer) { + //TODO get those from DI + $caching = !WP_DEBUG; + $debugging = WP_DEBUG; + $this->renderer = new Renderer($caching, $debugging); + $this->mailer = new \MailPoet\Mailer\Mailer(); + + return new StatsNotificationsWorker($this->mailer, $this->renderer, $timer); } /** @return SendingServiceKeyCheckWorker */ diff --git a/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php b/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php index a52e3f831e..3756782b91 100644 --- a/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php +++ b/tests/integration/Cron/Workers/SendingQueue/SendingQueueTest.php @@ -12,7 +12,7 @@ use MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler; use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker; use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask; use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask; -use MailPoet\Cron\Workers\StatsNotifications; +use MailPoet\Cron\Workers\StatsNotifications\Scheduler as StatsNotificationsScheduler; use MailPoet\Mailer\MailerLog; use MailPoet\Models\Newsletter; use MailPoet\Models\NewsletterLink; @@ -37,7 +37,7 @@ class SendingQueueTest extends \MailPoetTest { /** @var SendingErrorHandler */ private $sending_error_handler; - /** @var StatsNotifications */ + /** @var Scheduler */ private $stats_notifications_worker; function _before() { @@ -80,7 +80,7 @@ class SendingQueueTest extends \MailPoetTest { $this->newsletter_link->hash = 'abcde'; $this->newsletter_link->save(); $this->sending_error_handler = new SendingErrorHandler(); - $this->stats_notifications_worker = new StatsNotifications(); + $this->stats_notifications_worker = new StatsNotificationsScheduler(); $this->sending_queue_worker = new SendingQueueWorker($this->sending_error_handler, $this->stats_notifications_worker); } diff --git a/tests/integration/Cron/Workers/StatsNotificationsTest.php b/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php similarity index 86% rename from tests/integration/Cron/Workers/StatsNotificationsTest.php rename to tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php index 6642cafa97..1482584f06 100644 --- a/tests/integration/Cron/Workers/StatsNotificationsTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php @@ -1,21 +1,20 @@ stats_notifications = new StatsNotifications(); - Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + $this->stats_notifications = new Scheduler(); + Setting::setValue(Worker::SETTINGS_KEY, [ 'enabled' => true, 'address' => 'email@example.com' ]); @@ -33,7 +32,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfDisabled() { $newsletter_id = 6; - Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + Setting::setValue(Worker::SETTINGS_KEY, [ 'enabled' => false, 'address' => 'email@example.com' ]); @@ -45,7 +44,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfSettingsMissing() { $newsletter_id = 7; - Setting::setValue(StatsNotifications::SETTINGS_KEY, []); + Setting::setValue(Worker::SETTINGS_KEY, []); $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); $this->stats_notifications->schedule($newsletter); $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); @@ -54,7 +53,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfEmailIsMissing() { $newsletter_id = 8; - Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + Setting::setValue(Worker::SETTINGS_KEY, [ 'enabled' => true, ]); $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); @@ -65,7 +64,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfEmailIsEmpty() { $newsletter_id = 9; - Setting::setValue(StatsNotifications::SETTINGS_KEY, [ + Setting::setValue(Worker::SETTINGS_KEY, [ 'enabled' => true, 'address' => ' ' ]); @@ -78,7 +77,7 @@ class StatsNotificationsTest extends \MailPoetTest { function testShouldNotScheduleIfAlreadyScheduled() { $newsletter_id = 10; $existing_task = ScheduledTask::createOrUpdate([ - 'type' => StatsNotifications::TASK_TYPE, + 'type' => Worker::TASK_TYPE, 'status' => ScheduledTask::STATUS_SCHEDULED, 'scheduled_at' => '2017-01-02 12:13:14', ]); From ef5eba31d110f485591c04245f044947c334a8ff Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Thu, 10 Jan 2019 11:04:51 +0100 Subject: [PATCH 09/22] Add test [MAILPOET-1571] --- lib/Cron/Daemon.php | 2 +- .../Workers/StatsNotifications/Worker.php | 90 +++- lib/Models/NewsletterLink.php | 15 + lib/Models/ScheduledTask.php | 9 + lib/Models/StatsNotification.php | 9 + .../Workers/StatsNotifications/WorkerTest.php | 186 ++++++++ views/emails/statsNotification.html | 421 ++++++++++++++++++ 7 files changed, 709 insertions(+), 23 deletions(-) create mode 100644 tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php create mode 100644 views/emails/statsNotification.html diff --git a/lib/Cron/Daemon.php b/lib/Cron/Daemon.php index 2c87b00169..a2ee283efe 100644 --- a/lib/Cron/Daemon.php +++ b/lib/Cron/Daemon.php @@ -21,9 +21,9 @@ class Daemon { CronHelper::saveDaemon($settings_daemon_data); try { $this->executeMigrationWorker(); + $this->executeStatsNotificationsWorker(); $this->executeScheduleWorker(); $this->executeQueueWorker(); - $this->executeStatsNotificationsWorker(); $this->executeSendingServiceKeyCheckWorker(); $this->executePremiumKeyCheckWorker(); $this->executeBounceWorker(); diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index 44a9152feb..a779721a42 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -2,17 +2,16 @@ namespace MailPoet\Cron\Workers\StatsNotifications; +use Carbon\Carbon; use MailPoet\Config\Renderer; use MailPoet\Cron\CronHelper; use MailPoet\Mailer\Mailer; +use MailPoet\Models\Newsletter; +use MailPoet\Models\NewsletterLink; +use MailPoet\Models\ScheduledTask; use MailPoet\Models\Setting; +use MailPoet\Tasks\Sending; -/** - * TODO: - * - add processing of this task to Daemon - * - check JIRA what to do next and how to send the newsletter - * - see \MailPoet\Subscribers\NewSubscriberNotificationMailer how to send an email, now with DI everything should be easy - */ class Worker { const TASK_TYPE = 'stats_notification'; @@ -38,16 +37,19 @@ class Worker { /** @throws \Exception */ function process() { $settings = Setting::getValue(self::SETTINGS_KEY); - try { - $this->mailer->getSenderNameAndAddress($this->constructSenderEmail()); - $this->mailer->send($this->constructNewsletter(), $settings['address']); - } catch(\Exception $e) { - if(WP_DEBUG) { - throw $e; + $this->mailer->sender = $this->mailer->getSenderNameAndAddress($this->constructSenderEmail()); + foreach($this->getTasks() as $task) { + try { + $this->mailer->send($this->constructNewsletter($task), $settings['address']); + } catch(\Exception $e) { + //if(WP_DEBUG) { + throw $e; + //} + } finally { + $this->markTaskAsFinished($task); } + CronHelper::enforceExecutionLimit($this->timer); } - - CronHelper::enforceExecutionLimit($this->timer); } private function constructSenderEmail() { @@ -62,18 +64,62 @@ class Worker { ]; } - private function constructNewsletter() { - $context = [ - 'link_settings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings'), - 'link_premium' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-premium'), - ]; + private function getTasks() { + $date = new Carbon(); + return ScheduledTask::orderByAsc('priority') + ->orderByAsc('updated_at') + ->whereNull('deleted_at') + ->where('status', ScheduledTask::STATUS_SCHEDULED) + ->whereLte('scheduled_at', $date) + ->where('type', self::TASK_TYPE) + ->limit(Sending::RESULT_BATCH_SIZE) + ->findMany(); + } + + private function constructNewsletter(ScheduledTask $task) { + $newsletter = $this->getNewsletter($task); + $link = NewsletterLink::findTopLinkForNewsletter($newsletter); + $context = $this->prepareContext($newsletter, $link); return [ - 'subject' => sprintf(__('New subscriber to ', 'mailpoet')), + 'subject' => sprintf(_x('Stats for email %s', 'title of an automatic email containing statistics (newsletter open rate, click rate, etc)', 'mailpoet'), $newsletter->subject), 'body' => [ - 'html' => $this->renderer->render('emails/newSubscriberNotification.html', $context), - 'text' => $this->renderer->render('emails/newSubscriberNotification.txt', $context), + 'html' => $this->renderer->render('emails/statsNotification.html', $context), ], ]; } + private function getNewsletter(ScheduledTask $task) { + $statsNotificationModel = $task->statsNotification()->findOne(); + return $statsNotificationModel + ->newsletter() + ->findOne() + ->withSendingQueue() + ->withTotalSent() + ->withStatistics(); + } + + private function prepareContext(Newsletter $newsletter, NewsletterLink $link) { + return [ + 'subject' => $newsletter->subject, + 'preheader' => sprintf(_x( + '%1$s%% opens, %2$s%% clicks, %3$s%% unsubscribes in a nutshell.', 'newsletter open rate, click rate and unsubscribe rate', 'mailpoet'), + number_format(($newsletter->statistics['clicked'] * 100) / $newsletter->total_sent, 2), + number_format(($newsletter->statistics['opened'] * 100) / $newsletter->total_sent,2), + number_format(($newsletter->statistics['unsubscribed'] * 100) / $newsletter->total_sent,2) + ), + 'topLinkClicks' => $link->clicksCount, + 'topLink' => $link->url, + 'linkSettings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings#basics'), + 'linkStats' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-newsletters#/stats/' . $newsletter->id()), + 'premiumPluginActive' => is_plugin_active('mailpoet-premium/mailpoet-premium.php'), + ]; + } + + private function markTaskAsFinished(ScheduledTask $task) { + $task->status = ScheduledTask::STATUS_COMPLETED; + $task->processed_at = new Carbon; + $task->scheduled_at = null; + $task->save(); + } + } diff --git a/lib/Models/NewsletterLink.php b/lib/Models/NewsletterLink.php index 661858455a..a9b92ea365 100644 --- a/lib/Models/NewsletterLink.php +++ b/lib/Models/NewsletterLink.php @@ -5,4 +5,19 @@ if(!defined('ABSPATH')) exit; class NewsletterLink extends Model { public static $_table = MP_NEWSLETTER_LINKS_TABLE; + + static function findTopLinkForNewsletter(Newsletter $newsletter) { + return self::selectExpr('links.*') + ->selectExpr('count(*)', 'clicksCount') + ->tableAlias('links') + ->innerJoin(StatisticsClicks::$_table, + array('clicks.link_id', '=', 'links.id'), + 'clicks') + ->where('newsletter_id', $newsletter->id()) + ->groupBy('links.id') + ->orderByDesc('clicksCount') + ->limit(1) + ->findOne(); + } + } diff --git a/lib/Models/ScheduledTask.php b/lib/Models/ScheduledTask.php index 5b6cffcea9..650180f190 100644 --- a/lib/Models/ScheduledTask.php +++ b/lib/Models/ScheduledTask.php @@ -32,6 +32,15 @@ class ScheduledTask extends Model { ); } + /** @return StatsNotification */ + function statsNotification() { + return $this->hasOne( + StatsNotification::class, + 'task_id', + 'id' + ); + } + function pause() { $this->set('status', self::STATUS_PAUSED); $this->save(); diff --git a/lib/Models/StatsNotification.php b/lib/Models/StatsNotification.php index 8a162bf85f..b02cb2720a 100644 --- a/lib/Models/StatsNotification.php +++ b/lib/Models/StatsNotification.php @@ -5,6 +5,15 @@ namespace MailPoet\Models; class StatsNotification extends Model { public static $_table = MP_STATS_NOTIFICATIONS_TABLE; + /** @return Newsletter */ + public function newsletter() { + return $this->hasOne( + Newsletter::class, + 'id', + 'newsletter_id' + ); + } + /** @return StatsNotification */ static function createOrUpdate($data = array()) { $model = null; diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php new file mode 100644 index 0000000000..0121ac0b20 --- /dev/null +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -0,0 +1,186 @@ +mailer = $this->createMock(Mailer::class); + $this->renderer = $this->createMock(Renderer::class); + $this->stats_notifications = new Worker($this->mailer, $this->renderer); + Setting::setValue(Worker::SETTINGS_KEY, [ + 'enabled' => true, + 'address' => 'email@example.com' + ]); + $newsletter = Newsletter::createOrUpdate([ + 'subject' => 'Email Subject1', + 'type' => Newsletter::TYPE_STANDARD, + ]); + $sending_task = ScheduledTask::createOrUpdate([ + 'type' => 'sending', + 'status' => ScheduledTask::STATUS_COMPLETED, + ]); + $stats_notifications_task = ScheduledTask::createOrUpdate([ + 'type' => Worker::TASK_TYPE, + 'status' => ScheduledTask::STATUS_SCHEDULED, + 'scheduled_at' => '2017-01-02 12:13:14', + 'processed_at' => null, + ]); + StatsNotification::createOrUpdate([ + 'newsletter_id' => $newsletter->id(), + 'task_id' => $stats_notifications_task->id(), + ]); + $queue = SendingQueue::createOrUpdate([ + 'newsletter_rendered_subject' => 'Email Subject', + 'task_id' => $sending_task->id(), + 'newsletter_id' => $newsletter->id(), + 'count_processed' => 5, + ]); + $link = NewsletterLink::createOrUpdate([ + 'url' => 'Link url', + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'hash' => 'xyz', + ]); + StatisticsClicks::createOrUpdate([ + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'subscriber_id' => '5', + 'link_id' => $link->id(), + 'count' => 5, + 'created_at' => '2018-01-02 15:16:17', + ]); + $link2 = NewsletterLink::createOrUpdate([ + 'url' => 'Link url2', + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'hash' => 'xyzd', + ]); + StatisticsClicks::createOrUpdate([ + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'subscriber_id' => '6', + 'link_id' => $link2->id(), + 'count' => 5, + 'created_at' => '2018-01-02 15:16:17', + ]); + StatisticsClicks::createOrUpdate([ + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'subscriber_id' => '7', + 'link_id' => $link2->id(), + 'count' => 5, + 'created_at' => '2018-01-02 15:16:17', + ]); + StatisticsOpens::createOrUpdate([ + 'subscriber_id' => '10', + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'created_at' => '2017-01-02 12:23:45', + ]); + StatisticsOpens::createOrUpdate([ + 'subscriber_id' => '11', + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'created_at' => '2017-01-02 21:23:45', + ]); + StatisticsUnsubscribes::createOrUpdate([ + 'subscriber_id' => '12', + 'newsletter_id' => $newsletter->id(), + 'queue_id' => $queue->id(), + 'created_at' => '2017-01-02 21:23:45', + ]); + } + + function testRendersTemplate() { + $this->renderer->expects($this->once()) + ->method('render') + ->with( + $this->stringContains('statsNotification.html'), + $this->callback(function($context){ + return is_array($context); + })); + + $this->stats_notifications->process(); + } + + function testAddsSubjectToContext() { + $this->renderer->expects($this->once()) + ->method('render') + ->with( + $this->stringContains('statsNotification.html'), + $this->callback(function($context){ + return $context['subject'] === 'Email Subject1'; + })); + + $this->stats_notifications->process(); + } + + function testAddsPreHeaderToContext() { + $this->renderer->expects($this->once()) + ->method('render') + ->with( + $this->stringContains('statsNotification.html'), + $this->callback(function($context){ + return $context['preheader'] === '60.00% opens, 40.00% clicks, 20.00% unsubscribes in a nutshell.'; + })); + + $this->stats_notifications->process(); + } + + function testAddsWPUrlsToContext() { + $this->renderer->expects($this->once()) + ->method('render') + ->with( + $this->stringContains('statsNotification.html'), + $this->callback(function($context){ + return strpos($context['linkSettings'], 'mailpoet-settings') + && strpos($context['linkStats'], 'mailpoet-newsletters#/stats'); + })); + + $this->stats_notifications->process(); + } + + function testAddsLinksToContext() { + $this->renderer->expects($this->once()) + ->method('render') + ->with( + $this->stringContains('statsNotification.html'), + $this->callback(function($context){ + return ($context['topLink'] === 'Link url2') + && ($context['topLinkClicks'] === '2'); + })); + + $this->stats_notifications->process(); + } + + function testSends() { + $this->mailer->expects($this->once()) + ->method('send'); + + $this->stats_notifications->process(); + } + +} diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html new file mode 100644 index 0000000000..a0f47b4f18 --- /dev/null +++ b/views/emails/statsNotification.html @@ -0,0 +1,421 @@ + + + + + + + <%= subject %> + + + + + + + + + + + + + +
+ + + + + + + + + + + + + + + + + + + + + +
+ + + + + + +
+ + + + + + + + + + + + + + + + + + + + + +
+ new_logo_orange +
+

+ <%= __('Your stats are in!') %> +

+
+

+ <%= subject %> +

+
+
+
+ + + + + + +
+ +
+ + + + + + + + + + + + +
+
+ + + + +
+ + GOOD +
+
+
+

+ + 29.8% + +

+
+ + + +
+ + <%= __('open rate') %> + +
+
+
+ +
+ + + + + + + + + + + + +
+
+ + + + +
+ + EXCELLENT +
+
+
+

+ + 3.1% + +

+
+ + + +
+ + <%= __('click rate') %> + +
+
+
+
+ + + + + + +
+ + + + + + +
+ + + + +
+
+
+
+
+ + + + + + +
+ + + + + + + + + +
+ + <%= __('Most clicked link') %> + +
+ + + +
+ + <%= topLink %> + +
+ + + +
+ + <%= __('%s unique clicks')|replace({'%s': topLinkClicks}) %> + +
+
+
+
+ + + + + + +
+ + + + + + + + + + + + + + + + + + + + + +
+ + + + +
+
+
+ +
+ + + +
+ <%= __('See more stats in the Premium version, like all the links that were clicked or which subscribers opened your emails. You can also create segments of subscribers by clicks and opens.') %> +
+
+ +
+
+
+ + + + + + +
+ + + + + + + + + +
+ new_logo_white +
+
+
+ + + From 3bd80aecd3bf2a26d4fa1c37b8e49b457788a65e Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 14 Jan 2019 14:05:26 +0100 Subject: [PATCH 10/22] Move dependencies creation to DI container [MAILPOET-1571] --- lib/Cron/Workers/WorkersFactory.php | 26 ++++++++++++++++++-------- lib/DI/ContainerConfigurator.php | 10 ++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/Cron/Workers/WorkersFactory.php b/lib/Cron/Workers/WorkersFactory.php index d5dc4b3ba5..88107246a7 100644 --- a/lib/Cron/Workers/WorkersFactory.php +++ b/lib/Cron/Workers/WorkersFactory.php @@ -3,6 +3,7 @@ namespace MailPoet\Cron\Workers; use MailPoet\Config\Renderer; +use MailPoet\Cron\Workers\StatsNotifications\Scheduler as StatsNotificationScheduler; use MailPoet\Cron\Workers\Scheduler as SchedulerWorker; use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker; use MailPoet\Cron\Workers\SendingQueue\Migration as MigrationWorker; @@ -11,14 +12,29 @@ use MailPoet\Cron\Workers\Bounce as BounceWorker; use MailPoet\Cron\Workers\KeyCheck\PremiumKeyCheck as PremiumKeyCheckWorker; use MailPoet\Cron\Workers\KeyCheck\SendingServiceKeyCheck as SendingServiceKeyCheckWorker; use MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler; +use MailPoet\Mailer\Mailer; class WorkersFactory { /** @var SendingErrorHandler */ private $sending_error_handler; - public function __construct(SendingErrorHandler $sending_error_handler) { + /** @var StatsNotificationScheduler */ + private $scheduler; + + /** @var Mailer */ + private $mailer; + + /** + * @var Renderer + */ + private $renderer; + + public function __construct(SendingErrorHandler $sending_error_handler, StatsNotificationScheduler $scheduler, Mailer $mailer, Renderer $renderer) { $this->sending_error_handler = $sending_error_handler; + $this->scheduler = $scheduler; + $this->mailer = $mailer; + $this->renderer = $renderer; } /** @return SchedulerWorker */ @@ -28,16 +44,10 @@ class WorkersFactory { /** @return SendingQueueWorker */ function createQueueWorker($timer) { - return new SendingQueueWorker($this->sending_error_handler, $this->createStatsNotificationsWorker($timer), $timer); + return new SendingQueueWorker($this->sending_error_handler, $this->scheduler, $timer); } function createStatsNotificationsWorker($timer) { - //TODO get those from DI - $caching = !WP_DEBUG; - $debugging = WP_DEBUG; - $this->renderer = new Renderer($caching, $debugging); - $this->mailer = new \MailPoet\Mailer\Mailer(); - return new StatsNotificationsWorker($this->mailer, $this->renderer, $timer); } diff --git a/lib/DI/ContainerConfigurator.php b/lib/DI/ContainerConfigurator.php index 974a31ab7e..681afb4b66 100644 --- a/lib/DI/ContainerConfigurator.php +++ b/lib/DI/ContainerConfigurator.php @@ -51,11 +51,13 @@ class ContainerConfigurator implements IContainerConfigurator { // Config $container->autowire(\MailPoet\Config\AccessControl::class)->setPublic(true); $container->autowire(\MailPoet\Config\Hooks::class)->setPublic(true); + $container->register(\MailPoet\Config\Renderer::class)->setFactory([__CLASS__, 'createRenderer']); // Cron $container->autowire(\MailPoet\Cron\Daemon::class)->setPublic(true); $container->autowire(\MailPoet\Cron\DaemonHttpRunner::class)->setPublic(true); $container->autowire(\MailPoet\Cron\Workers\WorkersFactory::class)->setPublic(true); $container->autowire(\MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler::class)->setPublic(true); + $container->autowire(\MailPoet\Cron\Workers\StatsNotifications\Scheduler::class); // Listing $container->autowire(\MailPoet\Listing\BulkActionController::class)->setPublic(true); $container->autowire(\MailPoet\Listing\Handler::class)->setPublic(true); @@ -64,6 +66,8 @@ class ContainerConfigurator implements IContainerConfigurator { $container->autowire(\MailPoet\Router\Endpoints\Subscription::class)->setPublic(true); $container->autowire(\MailPoet\Router\Endpoints\Track::class)->setPublic(true); $container->autowire(\MailPoet\Router\Endpoints\ViewInBrowser::class)->setPublic(true); + // Mailer + $container->autowire(\MailPoet\Mailer\Mailer::class); // Subscribers $container->autowire(\MailPoet\Subscribers\NewSubscriberNotificationMailer::class)->setPublic(true); $container->autowire(\MailPoet\Subscribers\ConfirmationEmailMailer::class)->setPublic(true); @@ -96,4 +100,10 @@ class ContainerConfigurator implements IContainerConfigurator { } return $container->get(IContainerConfigurator::PREMIUM_CONTAINER_SERVICE_SLUG)->get($id); } + + static function createRenderer() { + $caching = !WP_DEBUG; + $debugging = WP_DEBUG; + return new \MailPoet\Config\Renderer($caching, $debugging); + } } From 1db75f40fbfb8f0a41f915811bdf38c92c370083 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 14 Jan 2019 14:05:56 +0100 Subject: [PATCH 11/22] Add statistics to template [MAILPOET-1571] --- .../Workers/StatsNotifications/Worker.php | 17 +- .../Workers/StatsNotifications/WorkerTest.php | 2 +- views/emails/statsNotification.html | 263 ++++++++++-------- 3 files changed, 159 insertions(+), 123 deletions(-) diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index a779721a42..aab0c65534 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -42,9 +42,9 @@ class Worker { try { $this->mailer->send($this->constructNewsletter($task), $settings['address']); } catch(\Exception $e) { - //if(WP_DEBUG) { + if(WP_DEBUG) { throw $e; - //} + } } finally { $this->markTaskAsFinished($task); } @@ -99,19 +99,24 @@ class Worker { } private function prepareContext(Newsletter $newsletter, NewsletterLink $link) { + $clicked = ($newsletter->statistics['clicked'] * 100) / $newsletter->total_sent; + $opened = ($newsletter->statistics['opened'] * 100) / $newsletter->total_sent; + $unsubscribed = ($newsletter->statistics['unsubscribed'] * 100) / $newsletter->total_sent; return [ 'subject' => $newsletter->subject, 'preheader' => sprintf(_x( '%1$s%% opens, %2$s%% clicks, %3$s%% unsubscribes in a nutshell.', 'newsletter open rate, click rate and unsubscribe rate', 'mailpoet'), - number_format(($newsletter->statistics['clicked'] * 100) / $newsletter->total_sent, 2), - number_format(($newsletter->statistics['opened'] * 100) / $newsletter->total_sent,2), - number_format(($newsletter->statistics['unsubscribed'] * 100) / $newsletter->total_sent,2) + number_format($clicked, 2), + number_format($opened, 2), + number_format($unsubscribed, 2) ), - 'topLinkClicks' => $link->clicksCount, + 'topLinkClicks' => (int)$link->clicksCount, 'topLink' => $link->url, 'linkSettings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings#basics'), 'linkStats' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-newsletters#/stats/' . $newsletter->id()), 'premiumPluginActive' => is_plugin_active('mailpoet-premium/mailpoet-premium.php'), + 'clicked' => $clicked, + 'opened' => $opened, ]; } diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index 0121ac0b20..ff92b07467 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -170,7 +170,7 @@ class WorkerTest extends \MailPoetTest { $this->stringContains('statsNotification.html'), $this->callback(function($context){ return ($context['topLink'] === 'Link url2') - && ($context['topLinkClicks'] === '2'); + && ($context['topLinkClicks'] === 2); })); $this->stats_notifications->process(); diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html index a0f47b4f18..df361b6848 100644 --- a/views/emails/statsNotification.html +++ b/views/emails/statsNotification.html @@ -24,6 +24,20 @@ +<% if opened > 30 %> + <% set openedColor = '2993ab' %> +<% elseif opened > 10 %> + <% set openedColor = 'f0b849' %> +<% else %> + <% set openedColor = 'd54e21' %> +<% endif %> +<% if clicked > 3 %> + <% set clickedColor = '2993ab' %> +<% elseif clicked > 1 %> + <% set clickedColor = 'f0b849' %> +<% else %> + <% set clickedColor = 'd54e21' %> +<% endif %> @@ -106,8 +120,14 @@
- - GOOD + + <% if opened > 30 %> + <%= __('EXCELENT') %> + <% elseif opened > 10 %> + <%= __('GOOD') %> + <% else %> + <%= __('BAD') %> + <% endif %>
@@ -117,8 +137,8 @@

- - 29.8% + + <%= number_format_i18n(opened) %>%

@@ -128,7 +148,7 @@ @@ -151,8 +171,14 @@
- + <%= __('open rate') %>
- - EXCELLENT + + <% if clicked > 3 %> + <%= __('EXCELENT') %> + <% elseif clicked > 1 %> + <%= __('GOOD') %> + <% else %> + <%= __('BAD') %> + <% endif %>
@@ -162,8 +188,8 @@

- - 3.1% + + <%= number_format_i18n(clicked) %>%

@@ -173,7 +199,7 @@ @@ -193,75 +219,77 @@
- + <%= __('click rate') %>
- - - - - - - - -
- - - - - - -
- - - - -
-
-
-
- - - - - - - - - - -
- - - - - - - - - -
- - <%= __('Most clicked link') %> - -
- - - -
- - <%= topLink %> - -
- - - -
- - <%= __('%s unique clicks')|replace({'%s': topLinkClicks}) %> - -
-
-
- - + <% if topLinkClicks > 0 %> + + + + + + + + +
+ + + + + + +
+ + + + +
+
+
+
+ + + + + + + + + + +
+ + + + + + + + + +
+ + <%= __('Most clicked link') %> + +
+ + + +
+ + <%= topLink %> + +
+ + + +
+ + <%= __('%s unique clicks')|replace({'%s': topLinkClicks}) %> + +
+
+
+ + + <% endif %> @@ -283,44 +311,47 @@ - - + + + + + <% endif %> From b66c724c4a74987602e3d6b1d23811d4253e7688 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 15 Jan 2019 14:43:16 +0100 Subject: [PATCH 12/22] Fix tests [MAILPOET-1571] --- .../Workers/StatsNotifications/Worker.php | 8 +- .../NewSubscriberNotificationMailer.php | 14 +++- lib/WP/Functions.php | 4 + tests/integration/Cron/CronHelperTest.php | 4 + .../integration/Cron/DaemonHttpRunnerTest.php | 80 ++++++++++--------- tests/integration/Cron/DaemonTest.php | 19 +++-- .../Workers/StatsNotifications/WorkerTest.php | 5 +- .../NewSubscriberNotificationMailerTest.php | 11 ++- 8 files changed, 94 insertions(+), 51 deletions(-) diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index aab0c65534..d3a1725f90 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -90,9 +90,11 @@ class Worker { private function getNewsletter(ScheduledTask $task) { $statsNotificationModel = $task->statsNotification()->findOne(); - return $statsNotificationModel - ->newsletter() - ->findOne() + $newsletter = $statsNotificationModel->newsletter()->findOne(); + if(!$newsletter) { + throw new \Exception('Newsletter not found'); + } + return $newsletter ->withSendingQueue() ->withTotalSent() ->withStatistics(); diff --git a/lib/Subscribers/NewSubscriberNotificationMailer.php b/lib/Subscribers/NewSubscriberNotificationMailer.php index 71034eb9d0..f96870c5da 100644 --- a/lib/Subscribers/NewSubscriberNotificationMailer.php +++ b/lib/Subscribers/NewSubscriberNotificationMailer.php @@ -6,6 +6,7 @@ use MailPoet\Config\Renderer; use MailPoet\Models\Segment; use MailPoet\Models\Setting; use MailPoet\Models\Subscriber; +use MailPoet\WP\Functions; class NewSubscriberNotificationMailer { @@ -18,11 +19,15 @@ class NewSubscriberNotificationMailer { /** @var \MailPoet\Mailer\Mailer */ private $mailer; + /** @var Functions */ + private $wordpress_functions; + /** * @param \MailPoet\Mailer\Mailer|null $mailer * @param Renderer|null $renderer + * @param Functions|null $wordpress_functions */ - function __construct($mailer = null, $renderer = null) { + function __construct($mailer = null, $renderer = null, $wordpress_functions = null) { if($renderer) { $this->renderer = $renderer; } else { @@ -30,6 +35,11 @@ class NewSubscriberNotificationMailer { $debugging = WP_DEBUG; $this->renderer = new Renderer($caching, $debugging); } + if($wordpress_functions) { + $this->wordpress_functions = $wordpress_functions; + } else { + $this->wordpress_functions = new Functions(); + } if($mailer) { $this->mailer = $mailer; } else { @@ -75,7 +85,7 @@ class NewSubscriberNotificationMailer { } private function constructSenderEmail() { - $url_parts = parse_url(home_url()); + $url_parts = parse_url($this->wordpress_functions->homeUrl()); $site_name = strtolower($url_parts['host']); if(substr($site_name, 0, 4) === 'www.') { $site_name = substr($site_name, 4); diff --git a/lib/WP/Functions.php b/lib/WP/Functions.php index a98c359f44..9a39ff27e1 100644 --- a/lib/WP/Functions.php +++ b/lib/WP/Functions.php @@ -28,6 +28,10 @@ class Functions { return call_user_func_array('current_time', func_get_args()); } + function homeUrl() { + return call_user_func_array('home_url', func_get_args()); + } + function getImageInfo($id) { /* * In some cases wp_get_attachment_image_src ignore the second parameter diff --git a/tests/integration/Cron/CronHelperTest.php b/tests/integration/Cron/CronHelperTest.php index e3283b16ea..5b834b68de 100644 --- a/tests/integration/Cron/CronHelperTest.php +++ b/tests/integration/Cron/CronHelperTest.php @@ -17,6 +17,10 @@ class CronHelperTest extends \MailPoetTest { Setting::setValue('cron_trigger', array( 'method' => 'none' )); + Setting::setValue('sender', array( + 'name' => 'John Doe', + 'address' => 'john.doe@example.com' + )); } function testItDefinesConstants() { diff --git a/tests/integration/Cron/DaemonHttpRunnerTest.php b/tests/integration/Cron/DaemonHttpRunnerTest.php index 2efb6cb842..5a80a7546c 100644 --- a/tests/integration/Cron/DaemonHttpRunnerTest.php +++ b/tests/integration/Cron/DaemonHttpRunnerTest.php @@ -22,27 +22,25 @@ class DaemonHttpRunnerTest extends \MailPoetTest { } function testItDoesNotRunWithoutRequestData() { - $daemon = Stub::construct( - new DaemonHttpRunner(new Daemon(new WorkersFactory(new SendingErrorHandler()))), - array(), - array( + $daemon = Stub::make( + DaemonHttpRunner::class, + [ 'abortWithError' => function($message) { return $message; } - ) + ] ); expect($daemon->run(false))->equals('Invalid or missing request data.'); } function testItDoesNotRunWhenThereIsInvalidOrMissingToken() { - $daemon = Stub::construct( - new DaemonHttpRunner(new Daemon(new WorkersFactory(new SendingErrorHandler()))), - array(), - array( + $daemon = Stub::make( + DaemonHttpRunner::class, + [ 'abortWithError' => function($message) { return $message; } - ) + ] ); $daemon->settings_daemon_data = array( 'token' => 123 @@ -54,14 +52,19 @@ class DaemonHttpRunnerTest extends \MailPoetTest { $data = array( 'token' => 123 ); - $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( - 'executeScheduleWorker' => function() { - throw new \Exception('Message'); - }, - 'executeQueueWorker' => function() { - throw new \Exception(); - }, - ), $this); + $daemon = Stub::make( + Daemon::class, + [ + 'executeScheduleWorker' => function() { + throw new \Exception('Message'); + }, + 'executeQueueWorker' => function() { + throw new \Exception(); + }, + 'executeMigrationWorker' => null, + 'executeStatsNotificationsWorker' => null, + ] + ); $daemon_http_runner = Stub::make(new DaemonHttpRunner($daemon), array( 'pauseExecution' => null, 'callSelf' => null @@ -74,16 +77,14 @@ class DaemonHttpRunnerTest extends \MailPoetTest { } function testItCanPauseExecution() { - $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( - 'executeScheduleWorker' => null, - 'executeQueueWorker' => null, - ), $this); - $daemon_http_runner = Stub::make(new DaemonHttpRunner($daemon), array( + $daemon = Stub::makeEmpty(Daemon::class); + $daemon_http_runner = Stub::make(DaemonHttpRunner::class, array( 'pauseExecution' => Expected::exactly(1, function($pause_delay) { expect($pause_delay)->lessThan(CronHelper::DAEMON_EXECUTION_LIMIT); expect($pause_delay)->greaterThan(CronHelper::DAEMON_EXECUTION_LIMIT - 1); }), - 'callSelf' => null + 'callSelf' => null, + 'terminateRequest' => null, ), $this); $data = array( 'token' => 123 @@ -107,7 +108,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); + $daemon->__construct(Stub::makeEmpty(Daemon::class)); $daemon->run($data); } @@ -127,7 +128,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); + $daemon->__construct(Stub::makeEmpty(Daemon::class)); $daemon->run($data); $data_after_run = Setting::getValue(CronHelper::DAEMON_SETTING); expect($data_after_run['token'], 567); @@ -145,7 +146,7 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'status' => CronHelper::DAEMON_STATUS_INACTIVE, ]; Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); + $daemon->__construct(Stub::makeEmpty(Daemon::class)); $daemon->run($data); } @@ -154,25 +155,31 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'executeScheduleWorker' => null, 'executeQueueWorker' => null, 'pauseExecution' => null, - 'callSelf' => null + 'callSelf' => null, + 'terminateRequest' => null, ), $this); $data = array( 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon_http_runner->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); + $daemon_http_runner->__construct(Stub::makeEmptyExcept(Daemon::class, 'run')); $daemon_http_runner->run($data); $updated_daemon = Setting::getValue(CronHelper::DAEMON_SETTING); expect($updated_daemon['token'])->equals($daemon_http_runner->token); } function testItUpdatesTimestampsDuringExecution() { - $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( - 'executeScheduleWorker' => function() { - sleep(2); - }, - 'executeQueueWorker' => null, - ), $this); + $daemon = Stub::make(Daemon::class, [ + 'executeScheduleWorker' => function() { + sleep(2); + }, + 'executeQueueWorker' => function() { + throw new \Exception(); + }, + 'executeMigrationWorker' => null, + 'executeStatsNotificationsWorker' => null, + ] + ); $daemon_http_runner = Stub::make(new DaemonHttpRunner($daemon), array( 'pauseExecution' => null, 'callSelf' => null @@ -201,12 +208,13 @@ class DaemonHttpRunnerTest extends \MailPoetTest { 'executeQueueWorker' => Expected::exactly(1), // daemon should call itself 'callSelf' => Expected::exactly(1), + 'terminateRequest' => null, ), $this); $data = array( 'token' => 123 ); Setting::setValue(CronHelper::DAEMON_SETTING, $data); - $daemon->__construct(new Daemon(new WorkersFactory(new SendingErrorHandler()))); + $daemon->__construct(Stub::makeEmptyExcept(Daemon::class, 'run')); $daemon->run($data); expect(ignore_user_abort())->equals(1); } diff --git a/tests/integration/Cron/DaemonTest.php b/tests/integration/Cron/DaemonTest.php index 1dfc4433f0..0e0cee380c 100644 --- a/tests/integration/Cron/DaemonTest.php +++ b/tests/integration/Cron/DaemonTest.php @@ -13,11 +13,14 @@ use MailPoet\Models\Setting; class DaemonTest extends \MailPoetTest { function testItCanExecuteWorkers() { - $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( + $daemon = Stub::make(Daemon::class, array( 'executeScheduleWorker' => Expected::exactly(1), 'executeQueueWorker' => Expected::exactly(1), - 'pauseExecution' => null, - 'callSelf' => null + 'executeMigrationWorker' => null, + 'executeStatsNotificationsWorker' => null, + 'executeSendingServiceKeyCheckWorker' => null, + 'executePremiumKeyCheckWorker' => null, + 'executeBounceWorker' => null, ), $this); $data = array( 'token' => 123 @@ -27,13 +30,15 @@ class DaemonTest extends \MailPoetTest { } function testItCanRun() { - $daemon = Stub::construct(Daemon::class, [new WorkersFactory(new SendingErrorHandler())], array( - 'pauseExecution' => null, + $daemon = Stub::make(Daemon::class, array( // workers should be executed 'executeScheduleWorker' => Expected::exactly(1), 'executeQueueWorker' => Expected::exactly(1), - // daemon should call itself - 'callSelf' => Expected::exactly(1), + 'executeMigrationWorker' => Expected::exactly(1), + 'executeStatsNotificationsWorker' => Expected::exactly(1), + 'executeSendingServiceKeyCheckWorker' => Expected::exactly(1), + 'executePremiumKeyCheckWorker' => Expected::exactly(1), + 'executeBounceWorker' => Expected::exactly(1) ), $this); $data = array( 'token' => 123 diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index ff92b07467..ac249ea5cc 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -17,7 +17,7 @@ use PHPUnit\Framework\MockObject\MockObject; class WorkerTest extends \MailPoetTest { - /** @var Scheduler */ + /** @var Worker */ private $stats_notifications; /** @var MockObject */ @@ -27,6 +27,9 @@ class WorkerTest extends \MailPoetTest { private $renderer; function _before() { + \ORM::raw_execute('TRUNCATE ' . Newsletter::$_table); + \ORM::raw_execute('TRUNCATE ' . ScheduledTask::$_table); + \ORM::raw_execute('TRUNCATE ' . SendingQueue::$_table); $this->mailer = $this->createMock(Mailer::class); $this->renderer = $this->createMock(Renderer::class); $this->stats_notifications = new Worker($this->mailer, $this->renderer); diff --git a/tests/integration/Subscribers/NewSubscriberNotificationMailerTest.php b/tests/integration/Subscribers/NewSubscriberNotificationMailerTest.php index d3da5fa3b3..9d2cb369b7 100644 --- a/tests/integration/Subscribers/NewSubscriberNotificationMailerTest.php +++ b/tests/integration/Subscribers/NewSubscriberNotificationMailerTest.php @@ -8,6 +8,7 @@ use MailPoet\Mailer\Mailer; use MailPoet\Models\Segment; use MailPoet\Models\Setting; use MailPoet\Models\Subscriber; +use MailPoet\WP\Functions; class NewSubscriberNotificationMailerTest extends \MailPoetTest { @@ -85,7 +86,6 @@ class NewSubscriberNotificationMailerTest extends \MailPoetTest { function testItRemovesWwwFromSenderAddress() { Setting::setValue(NewSubscriberNotificationMailer::SETTINGS_KEY, ['enabled' => true,'address' => 'a@b.c']); - update_option( 'home', 'http://www.example.com/xyz' ); $mailer = Stub::makeEmpty(Mailer::class, [ 'getSenderNameAndAddress' => @@ -96,7 +96,14 @@ class NewSubscriberNotificationMailerTest extends \MailPoetTest { }), ], $this); - $service = new NewSubscriberNotificationMailer($mailer); + $functions = Stub::makeEmpty(Functions::class, [ + 'homeUrl' => + Expected::once(function() { + return 'http://www.example.com/xyz'; + }), + ], $this); + + $service = new NewSubscriberNotificationMailer($mailer, null, $functions); $service->send($this->subscriber, $this->segments); } } From 627088e43d5e6aa1c30a84d0f7a7feba6064fa15 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 09:26:50 +0100 Subject: [PATCH 13/22] Add condition to WordPress trigger method [MAILPOET-1571] --- lib/Cron/Triggers/WordPress.php | 4 ++++ lib/Cron/Workers/StatsNotifications/Worker.php | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Cron/Triggers/WordPress.php b/lib/Cron/Triggers/WordPress.php index 9bfc417db8..5f97591df2 100644 --- a/lib/Cron/Triggers/WordPress.php +++ b/lib/Cron/Triggers/WordPress.php @@ -8,6 +8,7 @@ use MailPoet\Cron\Workers\SendingQueue\SendingQueue as SendingQueueWorker; use MailPoet\Cron\Workers\Bounce as BounceWorker; use MailPoet\Cron\Workers\KeyCheck\PremiumKeyCheck as PremiumKeyCheckWorker; use MailPoet\Cron\Workers\KeyCheck\SendingServiceKeyCheck as SendingServiceKeyCheckWorker; +use MailPoet\Cron\Workers\StatsNotifications\Worker; use MailPoet\Mailer\MailerLog; use MailPoet\Models\Setting; use MailPoet\Services\Bridge; @@ -44,6 +45,8 @@ class WordPress { $premium_key_specified = Bridge::isPremiumKeySpecified(); $premium_keycheck_due_tasks = PremiumKeyCheckWorker::getDueTasks(); $premium_keycheck_future_tasks = PremiumKeyCheckWorker::getFutureTasks(); + // stats notifications + $stats_notifications_tasks = (bool)Worker::getDueTasks(); // check requirements for each worker $sending_queue_active = (($scheduled_queues || $running_queues) && !$sending_limit_reached && !$sending_is_paused); $bounce_sync_active = ($mp_sending_enabled && ($bounce_due_tasks || !$bounce_future_tasks)); @@ -57,6 +60,7 @@ class WordPress { || $bounce_sync_active || $sending_service_key_check_active || $premium_key_check_active + || $stats_notifications_tasks ); } diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index d3a1725f90..0ddb8793f3 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -38,7 +38,7 @@ class Worker { function process() { $settings = Setting::getValue(self::SETTINGS_KEY); $this->mailer->sender = $this->mailer->getSenderNameAndAddress($this->constructSenderEmail()); - foreach($this->getTasks() as $task) { + foreach(self::getDueTasks() as $task) { try { $this->mailer->send($this->constructNewsletter($task), $settings['address']); } catch(\Exception $e) { @@ -64,7 +64,7 @@ class Worker { ]; } - private function getTasks() { + public static function getDueTasks() { $date = new Carbon(); return ScheduledTask::orderByAsc('priority') ->orderByAsc('updated_at') From e76f8d515914609ac8eb11d51d351bc7e581ed90 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 10:47:10 +0100 Subject: [PATCH 14/22] Prevent sending stats newsletter if tracking is disabled [MAILPOET-1571] --- lib/Cron/Workers/StatsNotifications/Scheduler.php | 3 +++ .../Cron/Workers/StatsNotifications/SchedulerTest.php | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/Cron/Workers/StatsNotifications/Scheduler.php b/lib/Cron/Workers/StatsNotifications/Scheduler.php index a04c5e441c..ba094f17f4 100644 --- a/lib/Cron/Workers/StatsNotifications/Scheduler.php +++ b/lib/Cron/Workers/StatsNotifications/Scheduler.php @@ -60,6 +60,9 @@ class Scheduler { if(empty(trim($settings['address']))) { return true; } + if(!(bool)Setting::getValue('tracking.enabled')) { + return true; + } return !(bool)$settings['enabled']; } diff --git a/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php b/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php index 1482584f06..a4fc1af259 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/SchedulerTest.php @@ -18,6 +18,7 @@ class SchedulerTest extends \MailPoetTest { 'enabled' => true, 'address' => 'email@example.com' ]); + Setting::setValue('tracking.enabled', true); } function testShouldSchedule() { @@ -30,6 +31,15 @@ class SchedulerTest extends \MailPoetTest { expect($task)->isInstanceOf(ScheduledTask::class); } + function testShouldNotScheduleIfTrackingIsDisabled() { + Setting::setValue('tracking.enabled', false); + $newsletter_id = 13; + $newsletter = Newsletter::createOrUpdate(['id' => $newsletter_id, 'type' => Newsletter::TYPE_STANDARD]); + $this->stats_notifications->schedule($newsletter); + $notification = StatsNotification::where('newsletter_id', $newsletter_id)->findOne(); + expect($notification)->isEmpty(); + } + function testShouldNotScheduleIfDisabled() { $newsletter_id = 6; Setting::setValue(Worker::SETTINGS_KEY, [ From de106e88280e80489920c3b7aa10be3b4df14224 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 13:14:56 +0100 Subject: [PATCH 15/22] Fix typo [MAILPOET-1571] --- views/emails/statsNotification.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html index df361b6848..173154aeb2 100644 --- a/views/emails/statsNotification.html +++ b/views/emails/statsNotification.html @@ -122,7 +122,7 @@
-
- + <% if premiumPluginActive %> + + + + <% else %> + + - - - - - - - + +
+ +
+ - - -
- - <%= __('View all stats') %> -
- -
- - - -
- <%= __('See more stats in the Premium version, like all the links that were clicked or which subscribers opened your emails. You can also create segments of subscribers by clicks and opens.') %> -
-
- -
+ <%= __('See more stats in the Premium version, like all the links that were clicked or which subscribers opened your emails. You can also create segments of subscribers by clicks and opens.') %> +
+
+ +
<% if opened > 30 %> - <%= __('EXCELENT') %> + <%= __('EXCELLENT') %> <% elseif opened > 10 %> <%= __('GOOD') %> <% else %> @@ -173,7 +173,7 @@ <% if clicked > 3 %> - <%= __('EXCELENT') %> + <%= __('EXCELLENT') %> <% elseif clicked > 1 %> <%= __('GOOD') %> <% else %> From 9eeda50b0718ba6fa527f59fe4aa897adc66a5a2 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 13:15:20 +0100 Subject: [PATCH 16/22] Render newsletters without clicked links [MAILPOET-1571] --- .../Workers/StatsNotifications/Worker.php | 12 ++++--- lib/Models/NewsletterLink.php | 6 +++- .../Workers/StatsNotifications/WorkerTest.php | 32 +++++++++++++++++++ 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index 0ddb8793f3..2255042700 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -100,11 +100,11 @@ class Worker { ->withStatistics(); } - private function prepareContext(Newsletter $newsletter, NewsletterLink $link) { + private function prepareContext(Newsletter $newsletter, NewsletterLink $link = null) { $clicked = ($newsletter->statistics['clicked'] * 100) / $newsletter->total_sent; $opened = ($newsletter->statistics['opened'] * 100) / $newsletter->total_sent; $unsubscribed = ($newsletter->statistics['unsubscribed'] * 100) / $newsletter->total_sent; - return [ + $context = [ 'subject' => $newsletter->subject, 'preheader' => sprintf(_x( '%1$s%% opens, %2$s%% clicks, %3$s%% unsubscribes in a nutshell.', 'newsletter open rate, click rate and unsubscribe rate', 'mailpoet'), @@ -112,14 +112,18 @@ class Worker { number_format($opened, 2), number_format($unsubscribed, 2) ), - 'topLinkClicks' => (int)$link->clicksCount, - 'topLink' => $link->url, + $context['topLinkClicks'] = 0, 'linkSettings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings#basics'), 'linkStats' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-newsletters#/stats/' . $newsletter->id()), 'premiumPluginActive' => is_plugin_active('mailpoet-premium/mailpoet-premium.php'), 'clicked' => $clicked, 'opened' => $opened, ]; + if($link) { + $context['topLinkClicks'] = (int)$link->clicksCount; + $context['topLink'] = $link->url; + } + return $context; } private function markTaskAsFinished(ScheduledTask $task) { diff --git a/lib/Models/NewsletterLink.php b/lib/Models/NewsletterLink.php index a9b92ea365..b29f2f2586 100644 --- a/lib/Models/NewsletterLink.php +++ b/lib/Models/NewsletterLink.php @@ -7,7 +7,7 @@ class NewsletterLink extends Model { public static $_table = MP_NEWSLETTER_LINKS_TABLE; static function findTopLinkForNewsletter(Newsletter $newsletter) { - return self::selectExpr('links.*') + $link = self::selectExpr('links.*') ->selectExpr('count(*)', 'clicksCount') ->tableAlias('links') ->innerJoin(StatisticsClicks::$_table, @@ -18,6 +18,10 @@ class NewsletterLink extends Model { ->orderByDesc('clicksCount') ->limit(1) ->findOne(); + if(!$link) { + return null; + } + return $link; } } diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index ac249ea5cc..03d5df385e 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -186,4 +186,36 @@ class WorkerTest extends \MailPoetTest { $this->stats_notifications->process(); } + function testItWorksForNewsletterWithNoStats() { + $newsletter = Newsletter::createOrUpdate([ + 'subject' => 'Email Subject2', + 'type' => Newsletter::TYPE_STANDARD, + ]); + $sending_task = ScheduledTask::createOrUpdate([ + 'type' => 'sending', + 'status' => ScheduledTask::STATUS_COMPLETED, + ]); + $stats_notifications_task = ScheduledTask::createOrUpdate([ + 'type' => Worker::TASK_TYPE, + 'status' => ScheduledTask::STATUS_SCHEDULED, + 'scheduled_at' => '2016-01-02 12:13:14', + 'processed_at' => null, + ]); + StatsNotification::createOrUpdate([ + 'newsletter_id' => $newsletter->id(), + 'task_id' => $stats_notifications_task->id(), + ]); + SendingQueue::createOrUpdate([ + 'newsletter_rendered_subject' => 'Email Subject2', + 'task_id' => $sending_task->id(), + 'newsletter_id' => $newsletter->id(), + 'count_processed' => 15, + ]); + + $this->mailer->expects($this->once()) + ->method('send'); + + $this->stats_notifications->process(); + } + } From 06370ea245588bbeae5ec324c939784fcbf3565d Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 13:20:17 +0100 Subject: [PATCH 17/22] Fix links [MAILPOET-1571] --- lib/Cron/Workers/StatsNotifications/Worker.php | 3 ++- .../Cron/Workers/StatsNotifications/WorkerTest.php | 3 ++- views/emails/statsNotification.html | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index 2255042700..6204da7be4 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -112,9 +112,10 @@ class Worker { number_format($opened, 2), number_format($unsubscribed, 2) ), - $context['topLinkClicks'] = 0, + 'topLinkClicks' => 0, 'linkSettings' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-settings#basics'), 'linkStats' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-newsletters#/stats/' . $newsletter->id()), + 'premiumPage' => get_site_url(null, '/wp-admin/admin.php?page=mailpoet-premium'), 'premiumPluginActive' => is_plugin_active('mailpoet-premium/mailpoet-premium.php'), 'clicked' => $clicked, 'opened' => $opened, diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index 03d5df385e..177fbadbe3 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -30,6 +30,7 @@ class WorkerTest extends \MailPoetTest { \ORM::raw_execute('TRUNCATE ' . Newsletter::$_table); \ORM::raw_execute('TRUNCATE ' . ScheduledTask::$_table); \ORM::raw_execute('TRUNCATE ' . SendingQueue::$_table); + \ORM::raw_execute('TRUNCATE ' . StatsNotification::$_table); $this->mailer = $this->createMock(Mailer::class); $this->renderer = $this->createMock(Renderer::class); $this->stats_notifications = new Worker($this->mailer, $this->renderer); @@ -212,7 +213,7 @@ class WorkerTest extends \MailPoetTest { 'count_processed' => 15, ]); - $this->mailer->expects($this->once()) + $this->mailer->expects($this->exactly(2)) ->method('send'); $this->stats_notifications->process(); diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html index 173154aeb2..c90470353d 100644 --- a/views/emails/statsNotification.html +++ b/views/emails/statsNotification.html @@ -318,7 +318,7 @@ @@ -343,7 +343,7 @@
- + <%= __('View all stats') %>
From 7469c26d1a96a89be48a93e20086317f112054c2 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Tue, 22 Jan 2019 15:51:00 +0100 Subject: [PATCH 18/22] Prevent rendering shortcode links [MAILPOET-1571] --- views/emails/statsNotification.html | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html index c90470353d..8349ff1bde 100644 --- a/views/emails/statsNotification.html +++ b/views/emails/statsNotification.html @@ -266,9 +266,13 @@
- + <%= __('See Premium features') %>
- + <% if topLink starts with 'http' %> + + <%= topLink %> + + <% else %> <%= topLink %> - + <% endif %>
From 3eb640597bfc55ce35c06b87a6458220b0fc2d73 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 28 Jan 2019 10:55:43 +0100 Subject: [PATCH 19/22] Fix rates [MAILPOET-1571] --- lib/Cron/Workers/StatsNotifications/Worker.php | 2 +- .../integration/Cron/Workers/StatsNotifications/WorkerTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index 6204da7be4..cafded0bf2 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -108,8 +108,8 @@ class Worker { 'subject' => $newsletter->subject, 'preheader' => sprintf(_x( '%1$s%% opens, %2$s%% clicks, %3$s%% unsubscribes in a nutshell.', 'newsletter open rate, click rate and unsubscribe rate', 'mailpoet'), - number_format($clicked, 2), number_format($opened, 2), + number_format($clicked, 2), number_format($unsubscribed, 2) ), 'topLinkClicks' => 0, diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index 177fbadbe3..c34e1d22c9 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -148,7 +148,7 @@ class WorkerTest extends \MailPoetTest { ->with( $this->stringContains('statsNotification.html'), $this->callback(function($context){ - return $context['preheader'] === '60.00% opens, 40.00% clicks, 20.00% unsubscribes in a nutshell.'; + return $context['preheader'] === '40.00% opens, 60.00% clicks, 20.00% unsubscribes in a nutshell.'; })); $this->stats_notifications->process(); From d7db761f73d17775edacac310ba7552bd4ad9e6e Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 28 Jan 2019 14:16:43 +0100 Subject: [PATCH 20/22] Add text template [MAILPOET-1571] --- .../Workers/StatsNotifications/Worker.php | 1 + .../Workers/StatsNotifications/WorkerTest.php | 32 ++++++++------- views/emails/statsNotification.txt | 40 +++++++++++++++++++ 3 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 views/emails/statsNotification.txt diff --git a/lib/Cron/Workers/StatsNotifications/Worker.php b/lib/Cron/Workers/StatsNotifications/Worker.php index cafded0bf2..3a216dfff7 100644 --- a/lib/Cron/Workers/StatsNotifications/Worker.php +++ b/lib/Cron/Workers/StatsNotifications/Worker.php @@ -84,6 +84,7 @@ class Worker { 'subject' => sprintf(_x('Stats for email %s', 'title of an automatic email containing statistics (newsletter open rate, click rate, etc)', 'mailpoet'), $newsletter->subject), 'body' => [ 'html' => $this->renderer->render('emails/statsNotification.html', $context), + 'text' => $this->renderer->render('emails/statsNotification.txt', $context), ], ]; } diff --git a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php index c34e1d22c9..7adfb0f3ff 100644 --- a/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php +++ b/tests/integration/Cron/Workers/StatsNotifications/WorkerTest.php @@ -119,22 +119,24 @@ class WorkerTest extends \MailPoetTest { } function testRendersTemplate() { - $this->renderer->expects($this->once()) - ->method('render') - ->with( - $this->stringContains('statsNotification.html'), - $this->callback(function($context){ - return is_array($context); - })); + $this->renderer->expects($this->exactly(2)) + ->method('render'); + $this->renderer->expects($this->at(0)) + ->method('render') + ->with($this->equalTo('emails/statsNotification.html')); + + $this->renderer->expects($this->at(1)) + ->method('render') + ->with($this->equalTo('emails/statsNotification.txt')); $this->stats_notifications->process(); } function testAddsSubjectToContext() { - $this->renderer->expects($this->once()) + $this->renderer->expects($this->exactly(2)) // html + text template ->method('render') ->with( - $this->stringContains('statsNotification.html'), + $this->anything(), $this->callback(function($context){ return $context['subject'] === 'Email Subject1'; })); @@ -143,10 +145,10 @@ class WorkerTest extends \MailPoetTest { } function testAddsPreHeaderToContext() { - $this->renderer->expects($this->once()) + $this->renderer->expects($this->exactly(2)) // html + text template ->method('render') ->with( - $this->stringContains('statsNotification.html'), + $this->anything(), $this->callback(function($context){ return $context['preheader'] === '40.00% opens, 60.00% clicks, 20.00% unsubscribes in a nutshell.'; })); @@ -155,10 +157,10 @@ class WorkerTest extends \MailPoetTest { } function testAddsWPUrlsToContext() { - $this->renderer->expects($this->once()) + $this->renderer->expects($this->exactly(2)) // html + text template ->method('render') ->with( - $this->stringContains('statsNotification.html'), + $this->anything(), $this->callback(function($context){ return strpos($context['linkSettings'], 'mailpoet-settings') && strpos($context['linkStats'], 'mailpoet-newsletters#/stats'); @@ -168,10 +170,10 @@ class WorkerTest extends \MailPoetTest { } function testAddsLinksToContext() { - $this->renderer->expects($this->once()) + $this->renderer->expects($this->exactly(2)) // html + text template ->method('render') ->with( - $this->stringContains('statsNotification.html'), + $this->anything(), $this->callback(function($context){ return ($context['topLink'] === 'Link url2') && ($context['topLinkClicks'] === 2); diff --git a/views/emails/statsNotification.txt b/views/emails/statsNotification.txt new file mode 100644 index 0000000000..5a595a7596 --- /dev/null +++ b/views/emails/statsNotification.txt @@ -0,0 +1,40 @@ +<%= __('Your stats are in!') %> + +<%= subject %> + +<%= __('open rate') %>: <%= number_format_i18n(opened) %>% +<% if opened > 30 %> + <%= __('EXCELLENT') %> +<% elseif opened > 10 %> + <%= __('GOOD') %> +<% else %> + <%= __('BAD') %> +<% endif %> + +<%= __('click rate') %>: <%= number_format_i18n(clicked) %>% +<% if clicked > 3 %> + <%= __('EXCELLENT') %> +<% elseif clicked > 1 %> + <%= __('GOOD') %> +<% else %> + <%= __('BAD') %> +<% endif %> + +<% if topLinkClicks > 0 %> +<%= __('Most clicked link') %> + <%= topLink %> + + <%= __('%s unique clicks')|replace({'%s': topLinkClicks}) %> +<% endif %> + +<% if premiumPluginActive %> +<%= __('View all stats') %> + <%= linkStats %> +<% else %> +<%= __('See Premium features') %> + <%= premiumPage %> +<% endif %> + +<%= __('How to improve my open rate?') %> https://mailpoet.com/how-to-improve-open-rates +<%= __('And my click rate?') %> https://mailpoet.com/how-to-improve-click-rates +<%= __('Disable these emails') %> <%= linkSettings %> From 7a9154a5a06bd704445d9ec58c91d48de6433f6e Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 28 Jan 2019 14:17:09 +0100 Subject: [PATCH 21/22] Update type to fix an error [MAILPOET-1571] --- lib/Config/Migrator.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index 9dab80e1bb..d7d24cfc12 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -142,11 +142,11 @@ class Migrator { } function scheduledTaskSubscribers() { - $attributes = array( + $attributes = array ( 'task_id int(11) unsigned NOT NULL,', 'subscriber_id int(11) unsigned NOT NULL,', 'processed int(1) NOT NULL,', - 'failed int(1) NOT NULL DEFAULT 0,', + 'failed SMALLINT(1) NOT NULL DEFAULT 0,', 'error text NULL,', 'created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,', 'PRIMARY KEY (task_id, subscriber_id),', From f694247d97e8096bcd2945180e84910552c4fd97 Mon Sep 17 00:00:00 2001 From: Pavel Dohnal Date: Mon, 28 Jan 2019 15:08:34 +0100 Subject: [PATCH 22/22] Make text consistent [MAILPOET-1571] --- views/emails/statsNotification.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/views/emails/statsNotification.html b/views/emails/statsNotification.html index 8349ff1bde..a39438c01f 100644 --- a/views/emails/statsNotification.html +++ b/views/emails/statsNotification.html @@ -189,7 +189,7 @@

- <%= number_format_i18n(clicked) %>% + <%= number_format_i18n(clicked) %>%