Merge pull request #1418 from mailpoet/cron-ping-warning

Automatically test if cron can be pinged and display a notice [MAILPOET-801]

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

@@ -450,7 +450,45 @@ const MailerMixin = {
   },
 };
 
+const CronMixin = {
+  checkCronStatus: (state) => {
+    if (state.meta.cron_accessible !== false) {
+      MailPoet.Notice.hide('mailpoet_cron_error');
+      return;
+    }
+
+    const cronPingCheckNotice = ReactStringReplace(
+      MailPoet.I18n.t('cronNotAccessibleNotice'),
+      /\[link\](.*?)\[\/link\]/g,
+      match => (
+        <a
+          href="https://beta.docs.mailpoet.com/article/231-sending-does-not-work"
+          target="_blank"
+          rel="noopener noreferrer"
+          key="check-cron"
+        >
+          { match }
+        </a>
+      )
+    );
+
+    MailPoet.Notice.error(
+      '',
+      { static: true, id: 'mailpoet_cron_error' }
+    );
+
+    ReactDOM.render(
+      (
+        <div>
+          <p>{cronPingCheckNotice}</p>
+        </div>
+      ),
+      jQuery('[data-id="mailpoet_cron_error"]')[0]
+    );
+  },
+};
 
 export { QueueMixin };
 export { StatisticsMixin };
 export { MailerMixin };
+export { CronMixin };

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

@@ -5,7 +5,10 @@ import Listing from 'listing/listing.jsx';
 import ListingTabs from 'newsletters/listings/tabs.jsx';
 import ListingHeading from 'newsletters/listings/heading.jsx';
 
-import { MailerMixin } from 'newsletters/listings/mixins.jsx';
+import {
+  MailerMixin,
+  CronMixin,
+} from 'newsletters/listings/mixins.jsx';
 
 import classNames from 'classnames';
 import MailPoet from 'mailpoet';
@@ -155,7 +158,7 @@ const newsletterActions = [
 ];
 
 const NewsletterListNotification = React.createClass({
-  mixins: [MailerMixin],
+  mixins: [MailerMixin, CronMixin],
   updateStatus: function updateStatus(e) {
     // make the event persist so that we can still override the selected value
     // in the ajax callback
@@ -330,7 +333,7 @@ const NewsletterListNotification = React.createClass({
           auto_refresh
           sort_by="updated_at"
           sort_order="desc"
-          afterGetItems={this.checkMailerStatus}
+          afterGetItems={(state) => { this.checkMailerStatus(state); this.checkCronStatus(state); }}
         />
       </div>
     );

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

@@ -12,6 +12,7 @@ import {
   QueueMixin,
   StatisticsMixin,
   MailerMixin,
+  CronMixin,
 } from 'newsletters/listings/mixins.jsx';
 
 const mailpoetTrackingEnabled = (!!(window.mailpoet_tracking_enabled));
@@ -57,7 +58,7 @@ Hooks.addFilter('mailpoet_newsletters_listings_notification_history_actions', St
 newsletterActions = Hooks.applyFilters('mailpoet_newsletters_listings_notification_history_actions', newsletterActions);
 
 const NewsletterListNotificationHistory = React.createClass({
-  mixins: [QueueMixin, StatisticsMixin, MailerMixin],
+  mixins: [QueueMixin, StatisticsMixin, MailerMixin, CronMixin],
   renderItem: function renderItem(newsletter, actions, meta) {
     const rowClasses = classNames(
       'manage-column',
@@ -120,7 +121,7 @@ const NewsletterListNotificationHistory = React.createClass({
           auto_refresh
           sort_by="sent_at"
           sort_order="desc"
-          afterGetItems={this.checkMailerStatus}
+          afterGetItems={(state) => { this.checkMailerStatus(state); this.checkCronStatus(state); }}
         />
       </div>
     );

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

@@ -12,6 +12,7 @@ import {
   QueueMixin,
   StatisticsMixin,
   MailerMixin,
+  CronMixin,
 } from 'newsletters/listings/mixins.jsx';
 
 const mailpoetTrackingEnabled = (!!(window.mailpoet_tracking_enabled));
@@ -172,7 +173,7 @@ Hooks.addFilter('mailpoet_newsletters_listings_standard_actions', StatisticsMixi
 newsletterActions = Hooks.applyFilters('mailpoet_newsletters_listings_standard_actions', newsletterActions);
 
 const NewsletterListStandard = React.createClass({
-  mixins: [QueueMixin, StatisticsMixin, MailerMixin],
+  mixins: [QueueMixin, StatisticsMixin, MailerMixin, CronMixin],
   renderItem: function renderItem(newsletter, actions, meta) {
     const rowClasses = classNames(
       'manage-column',
@@ -233,7 +234,7 @@ const NewsletterListStandard = React.createClass({
           auto_refresh
           sort_by="sent_at"
           sort_order="desc"
-          afterGetItems={this.checkMailerStatus}
+          afterGetItems={(state) => { this.checkMailerStatus(state); this.checkCronStatus(state); }}
         />
       </div>
     );

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

@@ -4,7 +4,11 @@ import Listing from 'listing/listing.jsx';
 import ListingTabs from 'newsletters/listings/tabs.jsx';
 import ListingHeading from 'newsletters/listings/heading.jsx';
 
-import { StatisticsMixin, MailerMixin } from 'newsletters/listings/mixins.jsx';
+import {
+  StatisticsMixin,
+  MailerMixin,
+  CronMixin,
+} from 'newsletters/listings/mixins.jsx';
 
 import classNames from 'classnames';
 import MailPoet from 'mailpoet';
@@ -128,7 +132,7 @@ Hooks.addFilter('mailpoet_newsletters_listings_welcome_notification_actions', St
 newsletterActions = Hooks.applyFilters('mailpoet_newsletters_listings_welcome_notification_actions', newsletterActions);
 
 const NewsletterListWelcome = React.createClass({
-  mixins: [StatisticsMixin, MailerMixin],
+  mixins: [StatisticsMixin, MailerMixin, CronMixin],
   updateStatus: function updateStatus(e) {
     // make the event persist so that we can still override the selected value
     // in the ajax callback
@@ -312,7 +316,7 @@ const NewsletterListWelcome = React.createClass({
           auto_refresh
           sort_by="updated_at"
           sort_order="desc"
-          afterGetItems={this.checkMailerStatus}
+          afterGetItems={(state) => { this.checkMailerStatus(state); this.checkCronStatus(state); }}
         />
       </div>
     );

lib/API/JSON/v1/Newsletters.php

@@ -6,6 +6,7 @@ use Carbon\Carbon;
 use MailPoet\API\JSON\Endpoint as APIEndpoint;
 use MailPoet\API\JSON\Error as APIError;
 use MailPoet\Config\AccessControl;
+use MailPoet\Cron\CronHelper;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterQueueTask;
 use MailPoet\Listing;
 use MailPoet\Models\Newsletter;
@@ -420,6 +421,7 @@ class Newsletters extends APIEndpoint {
       'groups' => $listing_data['groups'],
       'mta_log' => Setting::getValue('mta_log'),
       'mta_method' => Setting::getValue('mta.method'),
+      'cron_accessible' => CronHelper::isDaemonAccessible(),
       'current_time' => WPFunctions::currentTime('mysql')
     ));
   }

lib/Cron/CronHelper.php

@@ -19,7 +19,11 @@ class CronHelper {
 
   static function createDaemon($token) {
     $daemon = array(
-      'token' => $token
+      'token' => $token,
+      'run_accessed_at' => null,
+      'run_started_at' => null,
+      'run_completed_at' => null,
+      'last_error' => null,
     );
     self::saveDaemon($daemon);
     return $daemon;
@@ -70,10 +74,36 @@ class CronHelper {
       CronDaemonEndpoint::ACTION_RUN,
       $data
     );
+    $daemon = self::getDaemon();
+    if(!$daemon) {
+      throw new \LogicException('Daemon does not exist.');
+    }
+    $daemon['run_accessed_at'] = time();
+    self::saveDaemon($daemon);
     $result = self::queryCronUrl($url);
     return WPFunctions::wpRemoteRetrieveBody($result);
   }
 
+  /**
+   * @return boolean|null
+   */
+  static function isDaemonAccessible() {
+    $daemon = self::getDaemon();
+    if(!$daemon || $daemon['run_accessed_at'] === null) {
+      return null;
+    }
+    if($daemon['run_accessed_at'] <= (int)$daemon['run_started_at']) {
+      return true;
+    }
+    if(
+      $daemon['run_accessed_at'] + self::DAEMON_REQUEST_TIMEOUT < time() &&
+      $daemon['run_accessed_at'] > (int)$daemon['run_started_at']
+    ) {
+        return false;
+    }
+    return null;
+  }
+
   static function queryCronUrl($url) {
     $args = WPHooks::applyFilters(
       'mailpoet_cron_request_args',

lib/Cron/Daemon.php

@@ -51,6 +51,7 @@ class Daemon {
     }
     $daemon = $this->daemon;
     $daemon['token'] = $this->token;
+    $daemon['run_started_at'] = time();
     CronHelper::saveDaemon($daemon);
     try {
       $this->executeMigrationWorker();
@@ -60,8 +61,12 @@ class Daemon {
       $this->executePremiumKeyCheckWorker();
       $this->executeBounceWorker();
     } catch(\Exception $e) {
-      // continue processing, no need to handle errors
+      $daemon['last_error'] = $e->getMessage();
+      CronHelper::saveDaemon($daemon);
     }
+    // Log successful execution
+    $daemon['run_completed_at'] = time();
+    CronHelper::saveDaemon($daemon);
     // if workers took less time to execute than the daemon execution limit,
     // pause daemon execution to ensure that daemon runs only once every X seconds
     $elapsed_time = microtime(true) - $this->timer;

tests/unit/Cron/CronHelperTest.php

@@ -33,7 +33,11 @@ class CronHelperTest extends \MailPoetTest {
     expect($daemon)->equals(
       array(
         'token' => $token,
-        'updated_at' => $time
+        'updated_at' => $time,
+        'run_accessed_at' => null,
+        'run_started_at' => null,
+        'run_completed_at' => null,
+        'last_error' => null,
       )
     );
   }
@@ -46,7 +50,11 @@ class CronHelperTest extends \MailPoetTest {
     expect($daemon)->equals(
       array(
         'token' => $token,
-        'updated_at' => $time
+        'updated_at' => $time,
+        'run_accessed_at' => null,
+        'run_started_at' => null,
+        'run_completed_at' => null,
+        'last_error' => null,
       )
     );
   }
@@ -54,7 +62,11 @@ class CronHelperTest extends \MailPoetTest {
   function testItLoadsDaemon() {
     $daemon = array(
       'token' => 'some_token',
-      'updated_at' => '12345678'
+      'updated_at' => '12345678',
+      'run_accessed_at' => null,
+      'run_started_at' => null,
+      'run_completed_at' => null,
+      'last_error' => null,
     );
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
@@ -67,7 +79,11 @@ class CronHelperTest extends \MailPoetTest {
     // when saving daemon, 'updated_at' value should change
     $daemon = array(
       'token' => 'some_token',
-      'updated_at' => '12345678'
+      'updated_at' => '12345678',
+      'run_accessed_at' => null,
+      'run_started_at' => null,
+      'run_completed_at' => null,
+      'last_error' => null,
     );
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
@@ -79,6 +95,107 @@ class CronHelperTest extends \MailPoetTest {
     expect(CronHelper::getDaemon())->equals($daemon);
   }
 
+  function testItUpdatesDaemonAccessedAt() {
+    $daemon = [
+      'token' => 'some_token',
+      'updated_at' => 12345678,
+      'run_accessed_at' => null,
+      'run_started_at' => null,
+      'run_completed_at' => null,
+      'last_error' => null,
+    ];
+    Setting::setValue(
+      CronHelper::DAEMON_SETTING,
+      $daemon
+    );
+    $time = time();
+    Mock::double('MailPoet\Cron\CronHelper', ['queryCronUrl' => []]);
+    CronHelper::accessDaemon('some_token');
+    $updated_daemon = CronHelper::getDaemon();
+    expect($updated_daemon['run_accessed_at'])->greaterOrEquals($time);
+    expect($updated_daemon['run_accessed_at'])->lessThan($time + 2);
+  }
+
+  function testItThrowsAnExceptionIfAccessingNonExistingDaemon() {
+    try {
+      CronHelper::accessDaemon('some_token');
+      $this->fail('An exception should have been thrown.');
+    } catch (\LogicException $e) {
+      expect($e->getMessage())->equals('Daemon does not exist.');
+    }
+  }
+
+  function testItDetectsNotAccessibleDaemon() {
+    $time = time();
+    $run_start_values = [null, $time - 20];
+    foreach($run_start_values as $run_start) {
+      $daemon = [
+        'token' => 'some_token',
+        'updated_at' => 12345678,
+        'run_accessed_at' => $time - 10,
+        'run_started_at' => $run_start,
+        'run_completed_at' => null,
+        'last_error' => null,
+      ];
+      Setting::setValue(
+        CronHelper::DAEMON_SETTING,
+        $daemon
+      );
+      expect(CronHelper::isDaemonAccessible())->false();
+    }
+  }
+
+  function testItDetectsAccessibleDaemon() {
+    $time = time();
+    $daemon = [
+      'token' => 'some_token',
+      'updated_at' => 12345678,
+      'run_accessed_at' => $time - 5,
+      'run_started_at' => $time - 4,
+      'run_completed_at' => null,
+      'last_error' => null,
+    ];
+    Setting::setValue(
+      CronHelper::DAEMON_SETTING,
+      $daemon
+    );
+    expect(CronHelper::isDaemonAccessible())->true();
+  }
+
+  function testItDetectsUnknownStateOfTheDaemon() {
+    $time = time();
+    $test_inputs = [
+      [
+        'run_access' => null,
+        'run_start' => null,
+      ],
+      [
+        'run_access' => $time - 4,
+        'run_start' => null,
+      ],
+      [
+        'run_access' => $time - 4,
+        'run_start' => $time - 10,
+      ],
+      null,
+    ];
+    foreach($test_inputs as $test_input) {
+      $daemon = [
+        'token' => 'some_token',
+        'updated_at' => 12345678,
+        'run_accessed_at' => $test_input['run_access'],
+        'run_started_at' => $test_input['run_start'],
+        'run_completed_at' => null,
+        'last_error' => null,
+      ];
+      Setting::setValue(
+        CronHelper::DAEMON_SETTING,
+        $daemon
+      );
+      expect(CronHelper::isDaemonAccessible())->null();
+    }
+  }
+
   function testItCreatesRandomToken() {
     // random token is a string of 5 characters
     $token1 = CronHelper::createToken();

tests/unit/Cron/DaemonTest.php

@@ -80,10 +80,10 @@ class DaemonTest extends \MailPoetTest {
     $daemon->run();
   }
 
-  function testItContinuesExecutionWhenWorkersThrowException() {
+  function testItStoresErrorMessageAndContinuesExecutionWhenWorkersThrowException() {
     $daemon = Stub::make(new Daemon(true), array(
       'executeScheduleWorker' => function() {
-        throw new \Exception();
+        throw new \Exception('Message');
       },
       'executeQueueWorker' => function() {
         throw new \Exception();
@@ -97,6 +97,8 @@ class DaemonTest extends \MailPoetTest {
     Setting::setValue(CronHelper::DAEMON_SETTING, $data);
     $daemon->__construct($data);
     $daemon->run();
+    $updated_daemon = Setting::getValue(CronHelper::DAEMON_SETTING);
+    expect($updated_daemon['last_error'])->greaterOrEquals('Message');
   }
 
   function testItCanPauseExecution() {
@@ -172,6 +174,29 @@ class DaemonTest extends \MailPoetTest {
     expect($updated_daemon['token'])->equals($daemon->token);
   }
 
+  function testItUpdatesTimestampsDuringExecution() {
+    $daemon = Stub::make(new Daemon(true), array(
+      'executeScheduleWorker' => function() {
+        sleep(2);
+      },
+      'executeQueueWorker' => null,
+      'pauseExecution' => null,
+      'callSelf' => null
+    ), $this);
+    $data = array(
+      'token' => 123,
+    );
+    $now = time();
+    Setting::setValue(CronHelper::DAEMON_SETTING, $data);
+    $daemon->__construct($data);
+    $daemon->run();
+    $updated_daemon = Setting::getValue(CronHelper::DAEMON_SETTING);
+    expect($updated_daemon['run_started_at'])->greaterOrEquals($now);
+    expect($updated_daemon['run_started_at'])->lessThan($now + 2);
+    expect($updated_daemon['run_completed_at'])->greaterOrEquals($now + 2);
+    expect($updated_daemon['run_completed_at'])->lessThan($now + 4);
+  }
+
   function testItCanRun() {
     ignore_user_abort(0);
     expect(ignore_user_abort())->equals(0);

views/newsletters.html

@@ -287,6 +287,7 @@
     'beta': __('Beta'),
 	'errorWhileTakingScreenshot': __('An error occured while saving the template in "Recently sent"'),
     'selectAutomaticEmailsEventsHeading': __('Select %1s events'),
+    'cronNotAccessibleNotice': __('Oops! There seems to be an issue with the sending on your website. [link]See our guide[/link] to solve this yourself.'),
   }) %>
 <% endblock %>