From 2a48d0846d0ffddfb5dcef9923afb9fe99c5ba95 Mon Sep 17 00:00:00 2001
From: wxa <wxa@users.noreply.github.com>
Date: Mon, 30 Sep 2019 07:59:33 +0300
Subject: [PATCH] Move exception handling from concrete workers into
 processTask() [MAILPOET-2385]

---
 lib/Cron/Workers/InactiveSubscribers.php      | 41 ++++++++-----------
 .../Workers/SingleInstanceSimpleWorker.php    |  9 +++-
 lib/Cron/Workers/WooCommerceSync.php          |  7 +---
 .../Cron/Workers/InactiveSubscribersTest.php  | 21 ----------
 .../SingleInstanceSimpleWorkerTest.php        |  4 +-
 .../Cron/Workers/WooCommerceSyncTest.php      | 15 -------
 6 files changed, 29 insertions(+), 68 deletions(-)

diff --git a/lib/Cron/Workers/InactiveSubscribers.php b/lib/Cron/Workers/InactiveSubscribers.php
index 62f03c08ff..50d17862a4 100644
--- a/lib/Cron/Workers/InactiveSubscribers.php
+++ b/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');
-      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);
-      };
+    $tracking_enabled = (bool)$this->settings->get('tracking.enabled');
+    if (!$tracking_enabled) {
       self::schedule();
-    } catch (\Exception $e) {
-      $this->stopProgress($task);
-      throw $e;
+      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();
     return true;
   }
 }
diff --git a/lib/Cron/Workers/SingleInstanceSimpleWorker.php b/lib/Cron/Workers/SingleInstanceSimpleWorker.php
index 9f6fd39a28..481aaed3c0 100644
--- a/lib/Cron/Workers/SingleInstanceSimpleWorker.php
+++ b/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);
     }
diff --git a/lib/Cron/Workers/WooCommerceSync.php b/lib/Cron/Workers/WooCommerceSync.php
index 6d09af8be3..5b665f786b 100644
--- a/lib/Cron/Workers/WooCommerceSync.php
+++ b/lib/Cron/Workers/WooCommerceSync.php
@@ -27,12 +27,7 @@ class WooCommerceSync extends SingleInstanceSimpleWorker {
   }
 
   function processTaskStrategy(ScheduledTask $task) {
-    try {
-      $this->woocommerce_segment->synchronizeCustomers();
-    } catch (\Exception $e) {
-      $this->stopProgress($task);
-      throw $e;
-    }
+    $this->woocommerce_segment->synchronizeCustomers();
     return true;
   }
 
diff --git a/tests/integration/Cron/Workers/InactiveSubscribersTest.php b/tests/integration/Cron/Workers/InactiveSubscribersTest.php
index decf9d5ca1..4318bf3ebe 100644
--- a/tests/integration/Cron/Workers/InactiveSubscribersTest.php
+++ b/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]);
-    }
-  }
 }
diff --git a/tests/integration/Cron/Workers/SingleInstanceSimpleWorkerTest.php b/tests/integration/Cron/Workers/SingleInstanceSimpleWorkerTest.php
index bae6473081..99efe7de8b 100644
--- a/tests/integration/Cron/Workers/SingleInstanceSimpleWorkerTest.php
+++ b/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);
     }
   }
 
diff --git a/tests/integration/Cron/Workers/WooCommerceSyncTest.php b/tests/integration/Cron/Workers/WooCommerceSyncTest.php
index 4d81bfcbde..e2b34a7fa9 100644
--- a/tests/integration/Cron/Workers/WooCommerceSyncTest.php
+++ b/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;