Separate checking and storing of the keys [MAILPOET-939]

lib/API/JSON/v1/Services.php

@@ -30,6 +30,7 @@ class Services extends APIEndpoint {
 
     try {
       $result = $this->bridge->checkMSSKey($key);
+      $this->bridge->storeMSSKeyAndState($key, $result);
     } catch(\Exception $e) {
       return $this->errorResponse(array(
         $e->getCode() => $e->getMessage()
@@ -79,6 +80,7 @@ class Services extends APIEndpoint {
 
     try {
       $result = $this->bridge->checkPremiumKey($key);
+      $this->bridge->storePremiumKeyAndState($key, $result);
     } catch(\Exception $e) {
       return $this->errorResponse(array(
         $e->getCode() => $e->getMessage()

lib/Cron/Workers/KeyCheck/PremiumKeyCheck.php

@@ -16,6 +16,7 @@ class PremiumKeyCheck extends KeyCheckWorker {
   function checkKey() {
     $premium_key = Setting::getValue(Bridge::PREMIUM_KEY_SETTING_NAME);
     $result = $this->bridge->checkPremiumKey($premium_key);
+    $this->bridge->storePremiumKeyAndState($premium_key, $result);
     return $result;
   }
 }

lib/Cron/Workers/KeyCheck/SendingServiceKeyCheck.php

@@ -15,7 +15,9 @@ class SendingServiceKeyCheck extends KeyCheckWorker {
 
   function checkKey() {
     $mailer_config = Mailer::getMailerConfig();
-    $result = $this->bridge->checkMSSKey($mailer_config['mailpoet_api_key']);
+    $mss_key = $mailer_config['mailpoet_api_key'];
+    $result = $this->bridge->checkMSSKey($mss_key);
+    $this->bridge->storeMSSKeyAndState($mss_key, $result);
     $this->bridge->updateSubscriberCount($result);
     return $result;
   }

lib/Services/Bridge.php

@@ -63,38 +63,45 @@ class Bridge {
   function checkMSSKey($api_key) {
     $this->initApi($api_key);
     $result = $this->api->checkMSSKey();
-    return $this->processAPIKeyCheckResult($result);
+    return $this->processMSSKeyCheckResult($result);
   }
 
-  private function processAPIKeyCheckResult(array $result) {
+  private function processMSSKeyCheckResult(array $result) {
     $state_map = array(
       200 => self::MAILPOET_KEY_VALID,
       401 => self::MAILPOET_KEY_INVALID,
       402 => self::MAILPOET_KEY_EXPIRING
     );
 
-    $update_settings = false;
-
     if(!empty($result['code']) && isset($state_map[$result['code']])) {
       $key_state = $state_map[$result['code']];
-      $update_settings = true;
     } else {
       $key_state = self::MAILPOET_KEY_CHECK_ERROR;
     }
 
-    // store the key itself
+    return $this->buildKeyState(
-    if($update_settings) {
+      $key_state,
-      Setting::setValue(
+      $result
-        self::API_KEY_SETTING_NAME,
-        $this->api->getKey()
     );
   }
 
-    return $this->buildKeyState(
+  function storeMSSKeyAndState($key, $state) {
-      $key_state,
+    if(empty($state['state'])
-      $result,
+      || $state['state'] === self::MAILPOET_KEY_CHECK_ERROR
+    ) {
+      return false;
+    }
+
+    // store the key itself
+    Setting::setValue(
+      self::API_KEY_SETTING_NAME,
+      $key
+    );
+
+    // store the key state
+    Setting::setValue(
       self::API_KEY_STATE_SETTING_NAME,
-      $update_settings
+      $state
     );
   }
 
@@ -111,8 +118,6 @@ class Bridge {
       402 => self::PREMIUM_KEY_ALREADY_USED
     );
 
-    $update_settings = false;
-
     if(!empty($result['code']) && isset($state_map[$result['code']])) {
       if($state_map[$result['code']] == self::PREMIUM_KEY_VALID
         && !empty($result['data']['expire_at'])
@@ -121,41 +126,43 @@ class Bridge {
       } else {
         $key_state = $state_map[$result['code']];
       }
-      $update_settings = true;
     } else {
       $key_state = self::PREMIUM_KEY_CHECK_ERROR;
     }
 
-    // store the key itself
-    if($update_settings) {
-      Setting::setValue(
-        self::PREMIUM_KEY_SETTING_NAME,
-        $this->api->getKey()
-      );
-    }
-
     return $this->buildKeyState(
       $key_state,
-      $result,
+      $result
-      self::PREMIUM_KEY_STATE_SETTING_NAME,
-      $update_settings
     );
   }
 
-  private function buildKeyState($key_state, $result, $setting_name, $update_settings = false) {
+  function storePremiumKeyAndState($key, $state) {
+    if(empty($state['state'])
+      || $state['state'] === self::PREMIUM_KEY_CHECK_ERROR
+    ) {
+      return false;
+    }
+
+    // store the key itself
+    Setting::setValue(
+      self::PREMIUM_KEY_SETTING_NAME,
+      $key
+    );
+
+    // store the key state
+    Setting::setValue(
+      self::PREMIUM_KEY_STATE_SETTING_NAME,
+      $state
+    );
+  }
+
+  private function buildKeyState($key_state, $result) {
     $state = array(
       'state' => $key_state,
       'data' => !empty($result['data']) ? $result['data'] : null,
       'code' => !empty($result['code']) ? $result['code'] : self::CHECK_ERROR_UNKNOWN
     );
 
-    if($update_settings) {
-      Setting::setValue(
-        $setting_name,
-        $state
-      );
-    }
-
     return $state;
   }
 

tests/unit/API/JSON/v1/ServicesTest.php

@@ -21,7 +21,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithSuccessIfMSSKeyIsValid() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkMSSKey' => array('state' => Bridge::MAILPOET_KEY_VALID)),
+      array(
+        'checkMSSKey' => array('state' => Bridge::MAILPOET_KEY_VALID),
+        'storeMSSKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkMSSKey($this->data);
@@ -31,7 +34,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfMSSKeyIsInvalid() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkMSSKey' => array('state' => Bridge::MAILPOET_KEY_INVALID)),
+      array(
+        'checkMSSKey' => array('state' => Bridge::MAILPOET_KEY_INVALID),
+        'storeMSSKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkMSSKey($this->data);
@@ -42,10 +48,13 @@ class ServicesTest extends MailPoetTest {
     $date = new DateTime;
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkMSSKey' => array(
+      array(
+        'checkMSSKey' => array(
           'state' => Bridge::MAILPOET_KEY_EXPIRING,
           'data' => array('expire_at' => $date->format('c'))
-      )),
+        ),
+        'storeMSSKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkMSSKey($this->data);
@@ -57,7 +66,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfServiceIsUnavailableDuringMSSCheck() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkMSSKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE)),
+      array(
+        'checkMSSKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE),
+        'storeMSSKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkMSSKey($this->data);
@@ -68,7 +80,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfMSSCheckThrowsAnException() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkMSSKey' => function() { throw new \Exception('test'); }),
+      array(
+        'checkMSSKey' => function() { throw new \Exception('test'); },
+        'storeMSSKeyAndState' => Stub::never()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkMSSKey($this->data);
@@ -85,7 +100,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithSuccessIfPremiumKeyIsValid() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_VALID)),
+      array(
+        'checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_VALID),
+        'storePremiumKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);
@@ -98,7 +116,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfPremiumKeyIsInvalid() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_INVALID)),
+      array(
+        'checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_INVALID),
+        'storePremiumKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);
@@ -108,7 +129,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfPremiumKeyIsUsed() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_ALREADY_USED)),
+      array(
+        'checkPremiumKey' => array('state' => Bridge::PREMIUM_KEY_ALREADY_USED),
+        'storePremiumKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);
@@ -119,10 +143,13 @@ class ServicesTest extends MailPoetTest {
     $date = new DateTime;
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => array(
+      array(
+        'checkPremiumKey' => array(
           'state' => Bridge::PREMIUM_KEY_EXPIRING,
           'data' => array('expire_at' => $date->format('c'))
-      )),
+        ),
+        'storePremiumKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);
@@ -134,7 +161,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfServiceIsUnavailableDuringPremiumCheck() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE)),
+      array(
+        'checkPremiumKey' => array('code' => Bridge::CHECK_ERROR_UNAVAILABLE),
+        'storePremiumKeyAndState' => Stub::once()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);
@@ -145,7 +175,10 @@ class ServicesTest extends MailPoetTest {
   function testItRespondsWithErrorIfPremiumCheckThrowsAnException() {
     $this->services_endpoint->bridge = Stub::make(
       new Bridge(),
-      array('checkPremiumKey' => function() { throw new \Exception('test'); }),
+      array(
+        'checkPremiumKey' => function() { throw new \Exception('test'); },
+        'storePremiumKeyAndState' => Stub::never()
+      ),
       $this
     );
     $response = $this->services_endpoint->checkPremiumKey($this->data);

tests/unit/Cron/Workers/KeyCheck/PremiumKeyCheckTest.php

@@ -21,12 +21,21 @@ class PremiumKeyCheckTest extends MailPoetTest {
     $response = array('code' => Bridge::PREMIUM_KEY_VALID);
     $this->worker->bridge = Stub::make(
       new Bridge,
-      array('checkPremiumKey' => $response),
+      array(
+        'checkPremiumKey' => $response,
+        'storePremiumKeyAndState' => null
+      ),
       $this
     );
     $this->worker->bridge->expects($this->once())
       ->method('checkPremiumKey')
       ->with($this->equalTo($this->premium_key));
+    $this->worker->bridge->expects($this->once())
+      ->method('storePremiumKeyAndState')
+      ->with(
+        $this->equalTo($this->premium_key),
+        $this->equalTo($response)
+      );
     $this->fillPremiumKey();
     expect($this->worker->checkKey())->equals($response);
   }

tests/unit/Cron/Workers/KeyCheck/SendingServiceKeyCheckTest.php

@@ -8,6 +8,7 @@ use MailPoet\Services\Bridge;
 
 class SendingServiceKeyCheckTest extends MailPoetTest {
   function _before() {
+    $this->mss_key = 'some_key';
     $this->worker = new SendingServiceKeyCheck(microtime(true));
   }
 
@@ -23,10 +24,20 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
       new Bridge,
       array(
         'checkMSSKey' => $response,
+        'storeMSSKeyAndState' => null,
         'updateSubscriberCount' => Stub::once()
       ),
       $this
     );
+    $this->worker->bridge->expects($this->once())
+      ->method('checkMSSKey')
+      ->with($this->equalTo($this->mss_key));
+    $this->worker->bridge->expects($this->once())
+      ->method('storeMSSKeyAndState')
+      ->with(
+        $this->equalTo($this->mss_key),
+        $this->equalTo($response)
+      );
     $this->setMailPoetSendingMethod();
     expect($this->worker->checkKey())->equals($response);
   }
@@ -36,7 +47,7 @@ class SendingServiceKeyCheckTest extends MailPoetTest {
       Mailer::MAILER_CONFIG_SETTING_NAME,
       array(
         'method' => 'MailPoet',
-        'mailpoet_api_key' => 'some_key',
+        'mailpoet_api_key' => $this->mss_key,
       )
     );
   }

tests/unit/Services/BridgeTest.php

@@ -50,14 +50,12 @@ class BridgeTest extends MailPoetTest {
     $result = $this->bridge->checkMSSKey($this->valid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::MAILPOET_KEY_VALID);
-    expect($this->getMSSKey())->equals($this->valid_key);
   }
 
   function testItChecksInvalidMSSKey() {
     $result = $this->bridge->checkMSSKey($this->invalid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::MAILPOET_KEY_INVALID);
-    expect($this->getMSSKey())->equals($this->invalid_key);
   }
 
   function testItChecksExpiringMSSKey() {
@@ -65,7 +63,6 @@ class BridgeTest extends MailPoetTest {
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::MAILPOET_KEY_EXPIRING);
     expect($result['data']['expire_at'])->notEmpty();
-    expect($this->getMSSKey())->equals($this->expiring_key);
   }
 
   function testItReturnsErrorStateOnEmptyAPIResponseCodeDuringMSSCheck() {
@@ -74,28 +71,50 @@ class BridgeTest extends MailPoetTest {
     $result = $this->bridge->checkMSSKey($this->valid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::MAILPOET_KEY_CHECK_ERROR);
+  }
+
+  function testItStoresExpectedMSSKeyStates() {
+    $states = array(
+      Bridge::MAILPOET_KEY_VALID => $this->valid_key,
+      Bridge::MAILPOET_KEY_INVALID => $this->invalid_key,
+      Bridge::MAILPOET_KEY_EXPIRING => $this->expiring_key
+    );
+    foreach($states as $state => $key) {
+      $state = array('state' => $state);
+      $this->bridge->storeMSSKeyAndState($key, $state);
+      expect($this->getMSSKey())->equals($key);
+      expect($this->getMSSKeyState())->equals($state);
+    }
+  }
+
+  function testItDoesNotStoreErroneousOrUnexpectedMSSKeyStates() {
+    $states = array(
+      array('state' => Bridge::MAILPOET_KEY_CHECK_ERROR),
+      array()
+    );
+    foreach($states as $state) {
+      $this->bridge->storeMSSKeyAndState($this->valid_key, $state);
       expect($this->getMSSKey())->notEquals($this->valid_key);
+      expect($this->getMSSKeyState())->notEquals($state);
+    }
   }
 
   function testItChecksValidPremiumKey() {
     $result = $this->bridge->checkPremiumKey($this->valid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::PREMIUM_KEY_VALID);
-    expect($this->getPremiumKey())->equals($this->valid_key);
   }
 
   function testItChecksInvalidPremiumKey() {
     $result = $this->bridge->checkPremiumKey($this->invalid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::PREMIUM_KEY_INVALID);
-    expect($this->getPremiumKey())->equals($this->invalid_key);
   }
 
   function testItChecksAlreadyUsedPremiumKey() {
     $result = $this->bridge->checkPremiumKey($this->used_premium_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::PREMIUM_KEY_ALREADY_USED);
-    expect($this->getPremiumKey())->equals($this->used_premium_key);
   }
 
   function testItChecksExpiringPremiumKey() {
@@ -103,7 +122,6 @@ class BridgeTest extends MailPoetTest {
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::PREMIUM_KEY_EXPIRING);
     expect($result['data']['expire_at'])->notEmpty();
-    expect($this->getPremiumKey())->equals($this->expiring_premium_key);
   }
 
   function testItReturnsErrorStateOnEmptyAPIResponseCodeDuringPremiumCheck() {
@@ -112,7 +130,33 @@ class BridgeTest extends MailPoetTest {
     $result = $this->bridge->checkPremiumKey($this->valid_key);
     expect($result)->notEmpty();
     expect($result['state'])->equals(Bridge::PREMIUM_KEY_CHECK_ERROR);
+  }
+
+  function testItStoresExpectedPremiumKeyStates() {
+    $states = array(
+      Bridge::PREMIUM_KEY_VALID => $this->valid_key,
+      Bridge::PREMIUM_KEY_INVALID => $this->invalid_key,
+      Bridge::PREMIUM_KEY_ALREADY_USED => $this->used_premium_key,
+      Bridge::PREMIUM_KEY_EXPIRING => $this->expiring_key
+    );
+    foreach($states as $state => $key) {
+      $state = array('state' => $state);
+      $this->bridge->storePremiumKeyAndState($key, $state);
+      expect($this->getPremiumKey())->equals($key);
+      expect($this->getPremiumKeyState())->equals($state);
+    }
+  }
+
+  function testItDoesNotStoreErroneousOrUnexpectedPremiumKeyStates() {
+    $states = array(
+      array('state' => Bridge::PREMIUM_KEY_CHECK_ERROR),
+      array()
+    );
+    foreach($states as $state) {
+      $this->bridge->storePremiumKeyAndState($this->valid_key, $state);
       expect($this->getPremiumKey())->notEquals($this->valid_key);
+      expect($this->getPremiumKeyState())->notEquals($state);
+    }
   }
 
   function testItUpdatesSubscriberCount() {
@@ -136,8 +180,7 @@ class BridgeTest extends MailPoetTest {
       array('state' => Bridge::MAILPOET_KEY_VALID)
     );
     Bridge::invalidateKey();
-    $value = Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
+    expect($this->getMSSKeyState())->equals(array('state' => Bridge::MAILPOET_KEY_INVALID));
-    expect($value)->equals(array('state' => Bridge::MAILPOET_KEY_INVALID));
   }
 
   function testItChecksKeysOnSettingsSave() {
@@ -173,6 +216,10 @@ class BridgeTest extends MailPoetTest {
     return Setting::getValue(Bridge::API_KEY_SETTING_NAME);
   }
 
+  private function getMSSKeyState() {
+    return Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
+  }
+
   private function fillPremiumKey() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_SETTING_NAME,
@@ -184,6 +231,10 @@ class BridgeTest extends MailPoetTest {
     return Setting::getValue(Bridge::PREMIUM_KEY_SETTING_NAME);
   }
 
+  private function getPremiumKeyState() {
+    return Setting::getValue(Bridge::PREMIUM_KEY_STATE_SETTING_NAME);
+  }
+
   function _after() {
     ORM::raw_execute('TRUNCATE ' . Setting::$_table);
   }