diff --git a/assets/js/src/newsletters/listings/notification_history.jsx b/assets/js/src/newsletters/listings/notification_history.jsx index f157d41c67..5612660e55 100644 --- a/assets/js/src/newsletters/listings/notification_history.jsx +++ b/assets/js/src/newsletters/listings/notification_history.jsx @@ -34,7 +34,7 @@ const columns = [ display: mailpoet_tracking_enabled }, { - name: 'processed_at', + name: 'sent_at', label: MailPoet.I18n.t('sentOn'), } ]; @@ -56,11 +56,6 @@ newsletter_actions = Hooks.applyFilters('mailpoet_newsletters_listings_notificat const NewsletterListNotificationHistory = React.createClass({ mixins: [ QueueMixin, StatisticsMixin, MailerMixin ], - renderSentDate: function (newsletter) { - return (newsletter.queue.status === 'completed') - ? ( { MailPoet.Date.format(newsletter.updated_at) } ) - : MailPoet.I18n.t('notSentYet'); - }, renderItem: function (newsletter, actions, meta) { const rowClasses = classNames( 'manage-column', @@ -94,8 +89,8 @@ const NewsletterListNotificationHistory = React.createClass({ { this.renderStatistics(newsletter, undefined, meta.current_time) } ) : null } - - { this.renderSentDate(newsletter) } + + { (newsletter.sent_at) ? MailPoet.Date.format(newsletter.sent_at) : MailPoet.I18n.t('notSentYet') } ); @@ -125,7 +120,7 @@ const NewsletterListNotificationHistory = React.createClass({ columns={columns} item_actions={ newsletter_actions } auto_refresh={ true } - sort_by="updated_at" + sort_by="sent_at" sort_order="desc" afterGetItems={ this.checkMailerStatus } /> diff --git a/assets/js/src/newsletters/listings/standard.jsx b/assets/js/src/newsletters/listings/standard.jsx index f780f70832..797cc64418 100644 --- a/assets/js/src/newsletters/listings/standard.jsx +++ b/assets/js/src/newsletters/listings/standard.jsx @@ -83,8 +83,8 @@ const columns = [ display: mailpoet_tracking_enabled }, { - name: 'updated_at', - label: MailPoet.I18n.t('lastModifiedOn'), + name: 'sent_at', + label: MailPoet.I18n.t('sentOn'), sortable: true } ]; @@ -188,8 +188,8 @@ const NewsletterListStandard = React.createClass({ { this.renderStatistics(newsletter, undefined, meta.current_time) } ) : null } - - { MailPoet.Date.format(newsletter.updated_at) } + + { (newsletter.sent_at) ? MailPoet.Date.format(newsletter.sent_at) : MailPoet.I18n.t('notSentYet') } ); @@ -216,7 +216,7 @@ const NewsletterListStandard = React.createClass({ item_actions={ newsletter_actions } messages={ messages } auto_refresh={ true } - sort_by="updated_at" + sort_by="sent_at" sort_order="desc" afterGetItems={ this.checkMailerStatus } /> diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index 7a3cd51eca..841626c710 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -211,6 +211,7 @@ class Migrator { 'reply_to_name varchar(150) NOT NULL DEFAULT "",', 'preheader varchar(250) NOT NULL DEFAULT "",', 'body longtext,', + 'sent_at TIMESTAMP NULL,', 'created_at TIMESTAMP NULL,', 'updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,', 'deleted_at TIMESTAMP NULL,', diff --git a/lib/Config/Populator.php b/lib/Config/Populator.php index e1df2d84e4..569003abf3 100644 --- a/lib/Config/Populator.php +++ b/lib/Config/Populator.php @@ -3,7 +3,9 @@ namespace MailPoet\Config; use MailPoet\Cron\CronTrigger; use MailPoet\Mailer\MailerLog; +use MailPoet\Models\Newsletter; use MailPoet\Models\Segment; +use MailPoet\Models\SendingQueue; use MailPoet\Segments\WP; use MailPoet\Models\Setting; use MailPoet\Settings\Pages; @@ -26,37 +28,38 @@ class Populator { 'newsletter_templates', ); $this->templates = array( - "NewsletterBlank1Column", - "NewsletterBlank12Column", - "NewsletterBlank121Column", - "NewsletterBlank13Column", - "PostNotificationsBlank1Column", - "WelcomeBlank1Column", - "WelcomeBlank12Column", - "SimpleText", - "BurgerJoint", - "AppWelcome", - "WorldCup", - "FoodBox", - "Discount", - "KickOff", - "TakeAHike", - "FestivalEvent", - "PieceOfCake", - "Shoes", - "ScienceWeekly", - "ChocolateStore", - "Faith", - "TravelNomads", - "CoffeeShop", - "NewsDay", - "YogaStudio", + 'NewsletterBlank1Column', + 'NewsletterBlank12Column', + 'NewsletterBlank121Column', + 'NewsletterBlank13Column', + 'PostNotificationsBlank1Column', + 'WelcomeBlank1Column', + 'WelcomeBlank12Column', + 'SimpleText', + 'BurgerJoint', + 'AppWelcome', + 'WorldCup', + 'FoodBox', + 'Discount', + 'KickOff', + 'TakeAHike', + 'FestivalEvent', + 'PieceOfCake', + 'Shoes', + 'ScienceWeekly', + 'ChocolateStore', + 'Faith', + 'TravelNomads', + 'CoffeeShop', + 'NewsDay', + 'YogaStudio', ); } function up() { $this->convertExistingDataToUTF8(); $this->migrateSimpleScheduledTasks(); + $this->populateNewsletterSentAtField(); array_map(array($this, 'populate'), $this->models); @@ -328,8 +331,10 @@ class Populator { * character set, which usually defaults to latin1, but stored UTF-8 data. * This method converts existing incorrectly stored data that uses the * default character set, into a new character set that is used by WordPress. + * + * TODO: remove in final release */ - public function convertExistingDataToUTF8() { + function convertExistingDataToUTF8() { global $wpdb; if(!version_compare(get_option('mailpoet_db_version'), '3.0.0-beta.32', '<=')) { @@ -361,7 +366,6 @@ class Populator { 'forms' => array('name', 'body', 'settings', 'styles'), ); - foreach($tables as $table => $columns) { $query = "UPDATE `%s` SET %s WHERE %s"; $columns_query = array(); @@ -387,15 +391,16 @@ class Populator { } } - /** + /* * This migrates simple scheduled tasks from sending queues table to scheduled tasks table + * + * TODO: remove in final release */ public function migrateSimpleScheduledTasks() { global $wpdb; - if(!version_compare(get_option('mailpoet_db_version'), '3.0.0-beta.36.2.0', '<=')) { - // Data conversion should only be performed only once, when migrating from - // older version + // perform once for versions below 3.0.0-beta.36.2.1 + if(version_compare(get_option('mailpoet_db_version'), '3.0.0-beta.36.2.1', '>=')) { return; } @@ -436,4 +441,25 @@ class Populator { )); } + /* + * This populates existing newsletters' sent_at field with processed_at field data from + * corresponding sending queue. + * + * TODO: remove in final release + */ + function populateNewsletterSentAtField() { + global $wpdb; + + // perform once for versions below 3.0.0-beta.36.2.1 + if(version_compare(get_option('mailpoet_db_version'), '3.0.0-beta.36.2.1', '>=')) { + return; + } + + $query = "UPDATE `%s` newsletters JOIN `%s` queues ON newsletters.id = queues.newsletter_id SET newsletters.sent_at = queues.processed_at"; + $wpdb->query(sprintf( + $query, + Newsletter::$_table, + SendingQueue::$_table + )); + } } diff --git a/lib/Cron/Workers/SendingQueue/SendingQueue.php b/lib/Cron/Workers/SendingQueue/SendingQueue.php index 1e0520ab64..9aafb7ecbd 100644 --- a/lib/Cron/Workers/SendingQueue/SendingQueue.php +++ b/lib/Cron/Workers/SendingQueue/SendingQueue.php @@ -70,7 +70,7 @@ class SendingQueue { ); $queue->removeNonexistentSubscribers($subscibers_to_remove); if(!count($queue->subscribers['to_process'])) { - $this->newsletter_task->markNewsletterAsSent($newsletter); + $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); continue; } } @@ -80,7 +80,7 @@ class SendingQueue { $found_subscribers ); if($queue->status === SendingQueueModel::STATUS_COMPLETED) { - $this->newsletter_task->markNewsletterAsSent($newsletter); + $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); } $this->enforceSendingAndExecutionLimits(); } diff --git a/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php b/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php index e2ff187a8c..cbf1d17031 100644 --- a/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php +++ b/lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php @@ -125,12 +125,14 @@ class Newsletter { ); } - function markNewsletterAsSent($newsletter) { + function markNewsletterAsSent($newsletter, $queue) { // if it's a standard or notification history newsletter, update its status if($newsletter->type === NewsletterModel::TYPE_STANDARD || $newsletter->type === NewsletterModel::TYPE_NOTIFICATION_HISTORY ) { - $newsletter->setStatus(NewsletterModel::STATUS_SENT); + $newsletter->status = NewsletterModel::STATUS_SENT; + $newsletter->sent_at = $queue->processed_at; + $newsletter->save(); } } diff --git a/lib/Models/Newsletter.php b/lib/Models/Newsletter.php index bed0b8e1d0..c7d8b30cda 100644 --- a/lib/Models/Newsletter.php +++ b/lib/Models/Newsletter.php @@ -772,15 +772,22 @@ class Newsletter extends Model { } static function listingQuery($data = array()) { - return self::select(array( + $query = self::select( + array( 'id', 'subject', 'hash', 'type', 'status', + 'sent_at', 'updated_at', 'deleted_at' - )) + ) + ); + if($data['sort_by'] === 'sent_at') { + $query = $query->orderByExpr('ISNULL(sent_at) DESC'); + } + return $query ->filter('filterBy', $data) ->filter('groupBy', $data) ->filter('search', $data['search']); diff --git a/tests/unit/API/JSON/v1/NewslettersTest.php b/tests/unit/API/JSON/v1/NewslettersTest.php index 09400b4fba..5c6b6d7d0c 100644 --- a/tests/unit/API/JSON/v1/NewslettersTest.php +++ b/tests/unit/API/JSON/v1/NewslettersTest.php @@ -31,6 +31,51 @@ class NewslettersTest extends MailPoetTest { )); } + function testItKeepsUnsentNewslettersAtTheTopWhenSortingBySentAtDate() { + $router = new Newsletters(); + + $sent_newsletters = array(); + for($i = 1; $i <= 3; $i++) { + $sent_newsletters[$i] = Newsletter::create(); + $sent_newsletters[$i]->type = Newsletter::TYPE_STANDARD; + $sent_newsletters[$i]->subject = 'Sent newsletter ' . $i; + $sent_newsletters[$i]->sent_at = '2017-01-0' . $i . ' 01:01:01'; + $sent_newsletters[$i]->save(); + }; + + // sorting by ASC order retains unsent newsletters at the top + $response = $router->listing( + array( + 'params' => array( + 'type' => 'standard' + ), + 'sort_by' => 'sent_at', + 'sort_order' => 'asc' + ) + ); + expect($response->status)->equals(APIResponse::STATUS_OK); + expect($response->data[0]['id'])->equals($this->newsletter->id); + expect($response->data[1]['id'])->equals($sent_newsletters[1]->id); + expect($response->data[2]['id'])->equals($sent_newsletters[2]->id); + expect($response->data[3]['id'])->equals($sent_newsletters[3]->id); + + // sorting by DESC order retains unsent newsletters at the top + $response = $router->listing( + array( + 'params' => array( + 'type' => 'standard' + ), + 'sort_by' => 'sent_at', + 'sort_order' => 'desc' + ) + ); + expect($response->status)->equals(APIResponse::STATUS_OK); + expect($response->data[0]['id'])->equals($this->newsletter->id); + expect($response->data[1]['id'])->equals($sent_newsletters[3]->id); + expect($response->data[2]['id'])->equals($sent_newsletters[2]->id); + expect($response->data[3]['id'])->equals($sent_newsletters[1]->id); + } + function testItCanGetANewsletter() { $router = new Newsletters(); diff --git a/tests/unit/Config/PopulatorTest.php b/tests/unit/Config/PopulatorTest.php new file mode 100644 index 0000000000..e41101b96d --- /dev/null +++ b/tests/unit/Config/PopulatorTest.php @@ -0,0 +1,33 @@ +type = Newsletter::TYPE_STANDARD; + $newsletters[$i]->save(); + } + expect(Newsletter::whereNull('sent_at')->findMany())->count(3); + + $sending_queue = SendingQueue::create(); + $sending_queue->newsletter_id = $newsletters[1]->id; + $sending_queue->processed_at = date( 'Y-m-d H:i:s'); + $sending_queue->save(); + + $populator = new Populator(); + $populator->populateNewsletterSentAtField(); + expect(Newsletter::whereNull('sent_at')->findMany())->count(2); + expect(Newsletter::whereNotNull('sent_at')->findMany())->count(1); + } + + function _after() { + ORM::raw_execute('TRUNCATE ' . Newsletter::$_table); + ORM::raw_execute('TRUNCATE ' . SendingQueue::$_table); + } +} \ No newline at end of file diff --git a/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php b/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php index dd3d7554c7..859293a7b9 100644 --- a/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php +++ b/tests/unit/Cron/Workers/SendingQueue/SendingQueueTest.php @@ -343,6 +343,53 @@ class SendingQueueTest extends MailPoetTest { expect($statistics)->notEquals(false); } + function testItProcessesStandardNewsletters() { + $sending_queue_worker = new SendingQueueWorker( + $timer = false, + Stub::make( + new MailerTask(), + array( + 'send' => Stub::exactly(1, function($newsletter, $subscriber) { + // newsletter body should not be empty + expect(!empty($newsletter['body']['html']))->true(); + expect(!empty($newsletter['body']['text']))->true(); + return true; + }) + ), + $this + ) + ); + $sending_queue_worker->process(); + + // queue status is set to completed + $updated_queue = SendingQueue::findOne($this->queue->id); + expect($updated_queue->status)->equals(SendingQueue::STATUS_COMPLETED); + + // newsletter status is set to sent and sent_at date is populated + $updated_newsletter = Newsletter::findOne($this->newsletter->id); + expect($updated_newsletter->status)->equals(Newsletter::STATUS_SENT); + expect($updated_newsletter->sent_at)->equals($updated_queue->processed_at); + + // queue subscriber processed/to process count is updated + $updated_queue->subscribers = $updated_queue->getSubscribers(); + expect($updated_queue->subscribers)->equals( + array( + 'to_process' => array(), + 'processed' => array($this->subscriber->id) + ) + ); + expect($updated_queue->count_total)->equals(1); + expect($updated_queue->count_processed)->equals(1); + expect($updated_queue->count_to_process)->equals(0); + + // statistics entry should be created + $statistics = StatisticsNewsletters::where('newsletter_id', $this->newsletter->id) + ->where('subscriber_id', $this->subscriber->id) + ->where('queue_id', $this->queue->id) + ->findOne(); + expect($statistics)->notEquals(false); + } + function testItCanProcessWelcomeNewsletters() { $this->newsletter->type = Newsletter::TYPE_WELCOME; $this->newsletter_segment->delete(); diff --git a/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php b/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php index 50be5e051e..e8bfa04577 100644 --- a/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php +++ b/tests/unit/Cron/Workers/SendingQueue/Tasks/NewsletterTest.php @@ -183,30 +183,34 @@ class NewsletterTaskTest extends MailPoetTest { expect($newsletter_post->post_id)->equals('10'); } - function testItUpdatesStatusToSentOnlyForStandardAndPostNotificationNewsletters() { + function testItUpdatesStatusAndSetsSentAtDateOnlyForStandardAndPostNotificationNewsletters() { $newsletter = $this->newsletter; + $queue = new stdClass(); + $queue->processed_at = date('Y-m-d H:i:s'); // newsletter type is 'standard' $newsletter->type = Newsletter::TYPE_STANDARD; $newsletter->status = 'not_sent'; $newsletter->save(); - $this->newsletter_task->markNewsletterAsSent($newsletter); + $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); $updated_newsletter = Newsletter::findOne($newsletter->id); expect($updated_newsletter->status)->equals(Newsletter::STATUS_SENT); + expect($updated_newsletter->sent_at)->equals($queue->processed_at); // newsletter type is 'notification history' $newsletter->type = Newsletter::TYPE_NOTIFICATION_HISTORY; $newsletter->status = 'not_sent'; $newsletter->save(); - $this->newsletter_task->markNewsletterAsSent($newsletter); + $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); $updated_newsletter = Newsletter::findOne($newsletter->id); expect($updated_newsletter->status)->equals(Newsletter::STATUS_SENT); + expect($updated_newsletter->sent_at)->equals($queue->processed_at); // all other newsletter types $newsletter->type = Newsletter::TYPE_WELCOME; $newsletter->status = 'not_sent'; $newsletter->save(); - $this->newsletter_task->markNewsletterAsSent($newsletter); + $this->newsletter_task->markNewsletterAsSent($newsletter, $queue); $updated_newsletter = Newsletter::findOne($newsletter->id); expect($updated_newsletter->status)->notEquals(Newsletter::STATUS_SENT); } diff --git a/views/newsletters.html b/views/newsletters.html index f3f045cd30..c0f087fe8a 100644 --- a/views/newsletters.html +++ b/views/newsletters.html @@ -67,6 +67,7 @@ 'viewHistory': __('View history'), 'createdOn': __('Created on'), 'lastModifiedOn': __('Last modified on'), + 'sentOn': __('Sent on'), 'oneNewsletterTrashed': __('1 email was moved to the trash.'), 'multipleNewslettersTrashed': __('%$1d emails were moved to the trash.'), 'oneNewsletterDeleted': __('1 email was permanently deleted.'), @@ -247,7 +248,6 @@ 'sendingToSegmentsNotSpecified': __('You need to select a list to send to.'), 'backToPostNotifications': __('Back to Post notifications'), - 'sentOn': __('Sent on'), 'noSubscribers': __('No subscribers!'), 'mailerSendErrorNotice': __('Sending is paused because %$1s prevents MailPoet from delivering emails with the following error: %$2s'),