Fix overriding cron token

lib/Cron/CronHelper.php

@@ -40,6 +40,22 @@ class CronHelper {
     return Setting::getValue(self::DAEMON_SETTING);
   }
 
+  static function saveDaemonLastError($error) {
+    $daemon = self::getDaemon();
+    if($daemon) {
+      $daemon['last_error'] = $error;
+      self::saveDaemon($daemon);
+    }
+  }
+
+  static function saveDaemonRunCompleted($run_completed_at) {
+    $daemon = self::getDaemon();
+    if($daemon) {
+      $daemon['run_completed_at'] = $run_completed_at;
+      self::saveDaemon($daemon);
+    }
+  }
+
   static function saveDaemon($daemon) {
     $daemon['updated_at'] = time();
     return Setting::setValue(

lib/Cron/Daemon.php

@@ -61,12 +61,10 @@ class Daemon {
       $this->executePremiumKeyCheckWorker();
       $this->executeBounceWorker();
     } catch(\Exception $e) {
-      $daemon['last_error'] = $e->getMessage();
+      CronHelper::saveDaemonLastError($e->getMessage());
-      CronHelper::saveDaemon($daemon);
     }
     // Log successful execution
-    $daemon['run_completed_at'] = time();
+    CronHelper::saveDaemonRunCompleted(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

@@ -62,15 +62,7 @@ class CronHelperTest extends \MailPoetTest {
   }
 
   function testItLoadsDaemon() {
-    $daemon = array(
+    $daemon = $this->getDeamonTestData();
-      'token' => 'some_token',
-      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
-      'updated_at' => '12345678',
-      'run_accessed_at' => null,
-      'run_started_at' => null,
-      'run_completed_at' => null,
-      'last_error' => null,
-    );
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
       $daemon
@@ -80,15 +72,7 @@ class CronHelperTest extends \MailPoetTest {
 
   function testItSavesDaemon() {
     // when saving daemon, 'updated_at' value should change
-    $daemon = array(
+    $daemon = $this->getDeamonTestData();
-      'token' => 'some_token',
-      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
-      'updated_at' => '12345678',
-      'run_accessed_at' => null,
-      'run_started_at' => null,
-      'run_completed_at' => null,
-      'last_error' => null,
-    );
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
       $daemon
@@ -100,15 +84,7 @@ class CronHelperTest extends \MailPoetTest {
   }
 
   function testItUpdatesDaemonAccessedAt() {
-    $daemon = [
+    $daemon = $this->getDeamonTestData();
-      'token' => 'some_token',
-      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
-      'updated_at' => 12345678,
-      'run_accessed_at' => null,
-      'run_started_at' => null,
-      'run_completed_at' => null,
-      'last_error' => null,
-    ];
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
       $daemon
@@ -134,15 +110,9 @@ class CronHelperTest extends \MailPoetTest {
     $time = time();
     $run_start_values = [null, $time - 20];
     foreach($run_start_values as $run_start) {
-      $daemon = [
+      $daemon = $this->getDeamonTestData();
-        'token' => 'some_token',
+      $daemon['run_accessed_at'] = $time - 10;
-        'status' => CronHelper::DAEMON_STATUS_ACTIVE,
+      $daemon['run_started_at'] = $run_start;
-        '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
@@ -153,15 +123,9 @@ class CronHelperTest extends \MailPoetTest {
 
   function testItDetectsAccessibleDaemon() {
     $time = time();
-    $daemon = [
+    $daemon = $this->getDeamonTestData();
-      'token' => 'some_token',
+    $daemon['run_accessed_at'] = $time - 5;
-      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
+    $daemon['run_started_at'] = $time - 4;
-      '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
@@ -187,15 +151,9 @@ class CronHelperTest extends \MailPoetTest {
       null,
     ];
     foreach($test_inputs as $test_input) {
-      $daemon = [
+      $daemon = $this->getDeamonTestData();
-        'token' => 'some_token',
+      $daemon['run_accessed_at'] = $test_input['run_access'];
-        'status' => CronHelper::DAEMON_STATUS_ACTIVE,
+      $daemon['run_started_at'] = $test_input['run_start'];
-        '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
@@ -205,15 +163,7 @@ class CronHelperTest extends \MailPoetTest {
   }
 
   function testItDeactivatesDaemon() {
-    $daemon = [
+    $daemon = $this->getDeamonTestData();
-      'token' => 'some_token',
-      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
-      'updated_at' => '12345678',
-      'run_accessed_at' => null,
-      'run_started_at' => null,
-      'run_completed_at' => null,
-      'last_error' => null,
-    ];
     Setting::setValue(
       CronHelper::DAEMON_SETTING,
       $daemon
@@ -224,6 +174,31 @@ class CronHelperTest extends \MailPoetTest {
     expect($daemon['status'])->equals(CronHelper::DAEMON_STATUS_INACTIVE);
   }
 
+  function testItSavesLastError() {
+    $daemon = $this->getDeamonTestData();
+    Setting::setValue(
+      CronHelper::DAEMON_SETTING,
+      $daemon
+    );
+
+    CronHelper::saveDaemonLastError('error');
+    $daemon = CronHelper::getDaemon();
+    expect($daemon['last_error'])->equals('error');
+  }
+
+
+  function testItSavesRunCompletedAt() {
+    $daemon = $this->getDeamonTestData();
+    Setting::setValue(
+      CronHelper::DAEMON_SETTING,
+      $daemon
+    );
+
+    CronHelper::saveDaemonRunCompleted(123);
+    $daemon = CronHelper::getDaemon();
+    expect($daemon['run_completed_at'])->equals(123);
+  }
+
   function testItCreatesRandomToken() {
     // random token is a string of 5 characters
     $token1 = CronHelper::createToken();
@@ -329,4 +304,16 @@ class CronHelperTest extends \MailPoetTest {
     Mock::clean();
     \ORM::raw_execute('TRUNCATE ' . Setting::$_table);
   }
+
+  private function getDeamonTestData() {
+    return [
+      'token' => 'some_token',
+      'status' => CronHelper::DAEMON_STATUS_ACTIVE,
+      'updated_at' => 12345678,
+      'run_accessed_at' => null,
+      'run_started_at' => null,
+      'run_completed_at' => null,
+      'last_error' => null,
+    ];
+  }
 }

tests/unit/Cron/DaemonTest.php

@@ -137,7 +137,7 @@ class DaemonTest extends \MailPoetTest {
     $daemon->run();
   }
 
-  function testItTerminatesExecutionWhenDaemonTokenChanges() {
+  function testItTerminatesExecutionWhenDaemonTokenChangesAndKeepsChangedToken() {
     $daemon = Stub::make(new Daemon(true), array(
       'executeScheduleWorker' => function() {
         Setting::setValue(
@@ -155,6 +155,8 @@ class DaemonTest extends \MailPoetTest {
     Setting::setValue(CronHelper::DAEMON_SETTING, $data);
     $daemon->__construct($data);
     $daemon->run();
+    $data_after_run = Setting::getValue(CronHelper::DAEMON_SETTING);
+    expect($data_after_run['token'], 567);
   }
 
   function testItTerminatesExecutionWhenDaemonIsDeactivated() {