Move exception handling from concrete workers into processTask() [MAILPOET-2385]

lib/Cron/Workers/InactiveSubscribers.php

@@ -28,31 +28,26 @@ class InactiveSubscribers extends SingleInstanceSimpleWorker {
 
 
   function processTaskStrategy(ScheduledTask $task) {
-    try {
+    $tracking_enabled = (bool)$this->settings->get('tracking.enabled');
-      $tracking_enabled = (bool)$this->settings->get('tracking.enabled');
+    if (!$tracking_enabled) {
-      if (!$tracking_enabled) {
-        self::schedule();
-        return true;
-      }
-      $days_to_inactive = (int)$this->settings->get('deactivate_subscriber_after_inactive_days');
-      // Activate all inactive subscribers in case the feature is turned off
-      if ($days_to_inactive === 0) {
-        $this->inactive_subscribers_controller->reactivateInactiveSubscribers();
-        self::schedule();
-        return true;
-      }
-      // Handle activation/deactivation within interval
-      while ($this->inactive_subscribers_controller->markInactiveSubscribers($days_to_inactive, self::BATCH_SIZE) === self::BATCH_SIZE) {
-        CronHelper::enforceExecutionLimit($this->timer);
-      };
-      while ($this->inactive_subscribers_controller->markActiveSubscribers($days_to_inactive, self::BATCH_SIZE) === self::BATCH_SIZE) {
-        CronHelper::enforceExecutionLimit($this->timer);
-      };
       self::schedule();
-    } catch (\Exception $e) {
+      return true;
-      $this->stopProgress($task);
-      throw $e;
     }
+    $days_to_inactive = (int)$this->settings->get('deactivate_subscriber_after_inactive_days');
+    // Activate all inactive subscribers in case the feature is turned off
+    if ($days_to_inactive === 0) {
+      $this->inactive_subscribers_controller->reactivateInactiveSubscribers();
+      self::schedule();
+      return true;
+    }
+    // Handle activation/deactivation within interval
+    while ($this->inactive_subscribers_controller->markInactiveSubscribers($days_to_inactive, self::BATCH_SIZE) === self::BATCH_SIZE) {
+      CronHelper::enforceExecutionLimit($this->timer);
+    };
+    while ($this->inactive_subscribers_controller->markActiveSubscribers($days_to_inactive, self::BATCH_SIZE) === self::BATCH_SIZE) {
+      CronHelper::enforceExecutionLimit($this->timer);
+    };
+    self::schedule();
     return true;
   }
 }

lib/Cron/Workers/SingleInstanceSimpleWorker.php

@@ -23,7 +23,14 @@ abstract class SingleInstanceSimpleWorker extends SimpleWorker {
     }
 
     $this->startProgress($task);
-    $completed = $this->processTaskStrategy($task);
+
+    try {
+      $completed = $this->processTaskStrategy($task);
+    } catch (\Exception $e) {
+      $this->stopProgress($task);
+      throw $e;
+    }
+
     if ($completed) {
       $this->complete($task);
     }

lib/Cron/Workers/WooCommerceSync.php

@@ -27,12 +27,7 @@ class WooCommerceSync extends SingleInstanceSimpleWorker {
   }
 
   function processTaskStrategy(ScheduledTask $task) {
-    try {
+    $this->woocommerce_segment->synchronizeCustomers();
-      $this->woocommerce_segment->synchronizeCustomers();
-    } catch (\Exception $e) {
-      $this->stopProgress($task);
-      throw $e;
-    }
     return true;
   }
 

tests/integration/Cron/Workers/InactiveSubscribersTest.php

@@ -100,25 +100,4 @@ class InactiveSubscribersTest extends \MailPoetTest {
     $this->setExpectedException(\Exception::class, 'Maximum execution time has been reached.');
     $worker->processTaskStrategy(ScheduledTask::createOrUpdate([]));
   }
-
-  function testItWillResetTheInProgressFlagOnFail() {
-    $this->settings->set('deactivate_subscriber_after_inactive_days', 5);
-    $controller_mock = $this->createMock(InactiveSubscribersController::class);
-
-    $task = ScheduledTask::createOrUpdate([]);
-
-    $controller_mock->expects($this->once())
-      ->method('markInactiveSubscribers')
-      ->willThrowException(new \Exception('test error'));
-
-    try {
-      $worker = new InactiveSubscribers($controller_mock, $this->settings);
-      $worker->startProgress($task);
-      $worker->processTaskStrategy($task);
-      $this->fail('An exception should be thrown');
-    } catch (\Exception $e) {
-      expect($e->getMessage())->equals('test error');
-      expect($task->getMeta())->equals(['in_progress' => null]);
-    }
-  }
 }

tests/integration/Cron/Workers/SingleInstanceSimpleWorkerTest.php

@@ -26,7 +26,7 @@ class SingleInstanceSimpleWorkerTest extends \MailPoetTest {
     expect($this->worker->processTask($task))->equals(false);
   }
 
-  function testItWillKeepTheInProgressFlagOnFail() {
+  function testItWillResetTheInProgressFlagOnFail() {
     $task = $this->createScheduledTask();
     $this->worker->expects($this->once())
       ->method('processTaskStrategy')
@@ -36,7 +36,7 @@ class SingleInstanceSimpleWorkerTest extends \MailPoetTest {
       $this->fail('An exception should be thrown');
     } catch (\Exception $e) {
       expect($e->getMessage())->equals('test error');
-      expect(empty($task->getMeta()['in_progress']))->equals(false);
+      expect(empty($task->getMeta()['in_progress']))->equals(true);
     }
   }
 

tests/integration/Cron/Workers/WooCommerceSyncTest.php

@@ -33,21 +33,6 @@ class WooCommerceSyncTest extends \MailPoetTest {
     expect($this->worker->processTaskStrategy($task))->equals(true);
   }
 
-  function testItWillResetTheInProgressFlagOnFail() {
-    $task = $this->createScheduledTask();
-    $this->worker->startProgress($task);
-    $this->woocommerce_segment->expects($this->once())
-      ->method('synchronizeCustomers')
-      ->willThrowException(new \Exception('test error'));
-    try {
-      $this->worker->processTaskStrategy($task);
-      $this->fail('An exception should be thrown');
-    } catch (\Exception $e) {
-      expect($e->getMessage())->equals('test error');
-      expect($task->getMeta())->equals(['in_progress' => null]);
-    }
-  }
-
   private function createScheduledTask() {
     $task = ScheduledTask::create();
     $task->type = WooCommerceSync::TASK_TYPE;