Merge pull request #980 from mailpoet/newsletter_listing_update

Sorts standard/history notification records by sent_at date in listings [MAILPOET-932]

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')
-      ? ( <abbr>{ MailPoet.Date.format(newsletter.updated_at) }</abbr> )
-      : 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) }
           </td>
         ) : null }
-        <td className="column-date" data-colname={ MailPoet.I18n.t('lastModifiedOn') }>
+        <td className="column-date" data-colname={ MailPoet.I18n.t('sentOn') }>
-          { this.renderSentDate(newsletter) }
+          { (newsletter.sent_at) ? MailPoet.Date.format(newsletter.sent_at) : MailPoet.I18n.t('notSentYet') }
         </td>
       </div>
     );
@@ -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 }
         />

assets/js/src/newsletters/listings/standard.jsx

@@ -83,8 +83,8 @@ const columns = [
     display: mailpoet_tracking_enabled
   },
   {
-    name: 'updated_at',
+    name: 'sent_at',
-    label: MailPoet.I18n.t('lastModifiedOn'),
+    label: MailPoet.I18n.t('sentOn'),
     sortable: true
   }
 ];
@@ -188,8 +188,8 @@ const NewsletterListStandard = React.createClass({
             { this.renderStatistics(newsletter, undefined, meta.current_time) }
           </td>
         ) : null }
-        <td className="column-date" data-colname={ MailPoet.I18n.t('lastModifiedOn') }>
+        <td className="column-date" data-colname={ MailPoet.I18n.t('sentOn') }>
-          <abbr>{ MailPoet.Date.format(newsletter.updated_at) }</abbr>
+          <abbr>{ (newsletter.sent_at) ? MailPoet.Date.format(newsletter.sent_at) : MailPoet.I18n.t('notSentYet') }</abbr>
         </td>
       </div>
     );
@@ -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 }
         />

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,',

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",
+      'NewsletterBlank1Column',
-      "NewsletterBlank12Column",
+      'NewsletterBlank12Column',
-      "NewsletterBlank121Column",
+      'NewsletterBlank121Column',
-      "NewsletterBlank13Column",
+      'NewsletterBlank13Column',
-      "PostNotificationsBlank1Column",
+      'PostNotificationsBlank1Column',
-      "WelcomeBlank1Column",
+      'WelcomeBlank1Column',
-      "WelcomeBlank12Column",
+      'WelcomeBlank12Column',
-      "SimpleText",
+      'SimpleText',
-      "BurgerJoint",
+      'BurgerJoint',
-      "AppWelcome",
+      'AppWelcome',
-      "WorldCup",
+      'WorldCup',
-      "FoodBox",
+      'FoodBox',
-      "Discount",
+      'Discount',
-      "KickOff",
+      'KickOff',
-      "TakeAHike",
+      'TakeAHike',
-      "FestivalEvent",
+      'FestivalEvent',
-      "PieceOfCake",
+      'PieceOfCake',
-      "Shoes",
+      'Shoes',
-      "ScienceWeekly",
+      'ScienceWeekly',
-      "ChocolateStore",
+      'ChocolateStore',
-      "Faith",
+      'Faith',
-      "TravelNomads",
+      'TravelNomads',
-      "CoffeeShop",
+      'CoffeeShop',
-      "NewsDay",
+      'NewsDay',
-      "YogaStudio",
+      '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', '<=')) {
+    // perform once for versions below 3.0.0-beta.36.2.1
-      // Data conversion should only be performed only once, when migrating from
+    if(version_compare(get_option('mailpoet_db_version'), '3.0.0-beta.36.2.1', '>=')) {
-      // older version
       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
+    ));
+  }
 }

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();
       }

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();
     }
   }
 

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']);

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();
 

tests/unit/Config/PopulatorTest.php

@@ -0,0 +1,33 @@
+<?php
+
+use MailPoet\Config\Populator;
+use MailPoet\Models\Newsletter;
+use MailPoet\Models\SendingQueue;
+
+class PopulatorTest extends MailPoetTest {
+  function testItPopulatesNewslettersTableSentAtColumn() {
+    // TODO: remove in final release
+    $newsletters = array();
+    for($i = 1; $i <= 3; $i++) {
+      $newsletters[$i] = Newsletter::create();
+      $newsletters[$i]->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);
+  }
+}

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();

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);
   }

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'),