Make the Premium key check stricter, split a unit test into more granular ones [PREMIUM-4]

lib/Config/ServicesChecker.php

@@ -49,23 +49,20 @@ class ServicesChecker {
   }
 
   function isPremiumKeyValid($display_error_notice = true) {
-    if(!Bridge::isPremiumKeySpecified()) {
+    $premium_key_specified = Bridge::isPremiumKeySpecified();
-      return null;
-    }
-
     $premium_plugin_active = License::getLicense();
     $premium_key = Setting::getValue(Bridge::PREMIUM_KEY_STATE_SETTING_NAME);
-    if(empty($premium_key['state'])) {
+
-      return false;
+    if (!$premium_plugin_active) {
-    }
+      $display_error_notice = false;
-    if($premium_key['state'] == Bridge::PREMIUM_KEY_VALID) {
-      return true;
     }
 
-    if($premium_key['state'] == Bridge::PREMIUM_KEY_INVALID
+    if(!$premium_key_specified
-      || $premium_key['state'] == Bridge::PREMIUM_KEY_ALREADY_USED
+      || empty($premium_key['state'])
+      || $premium_key['state'] === Bridge::PREMIUM_KEY_INVALID
+      || $premium_key['state'] === Bridge::PREMIUM_KEY_ALREADY_USED
     ) {
-      if($premium_plugin_active && $display_error_notice) {
+      if($display_error_notice) {
         $error = Helpers::replaceLinkTags(
           __('Warning! Your License Key is either invalid or expired. [link]Renew your License now[/link] to enjoy automatic updates and Premium support.', 'mailpoet'),
           'https://account.mailpoet.com'
@@ -73,10 +70,10 @@ class ServicesChecker {
         WPNotice::displayError($error);
       }
       return false;
-    } elseif($premium_key['state'] == Bridge::PREMIUM_KEY_EXPIRING
+    } elseif($premium_key['state'] === Bridge::PREMIUM_KEY_EXPIRING
       && !empty($premium_key['data']['expire_at'])
     ) {
-      if($premium_plugin_active && $display_error_notice) {
+      if($display_error_notice) {
         $date = date('Y-m-d', strtotime($premium_key['data']['expire_at']));
         $error = Helpers::replaceLinkTags(
           __('Your License Key is expiring! Don\'t forget to [link]renew your license[/link] by %s to keep enjoying automatic updates and Premium support.', 'mailpoet'),
@@ -86,6 +83,8 @@ class ServicesChecker {
         WPNotice::displayWarning($error);
       }
       return true;
+    } elseif($premium_key['state'] === Bridge::PREMIUM_KEY_VALID) {
+      return true;
     }
 
     return false;

tests/unit/Config/ServicesCheckerTest.php

@@ -6,28 +6,36 @@ use MailPoet\Models\Setting;
 use MailPoet\Services\Bridge;
 
 class ServicesCheckerTest extends MailPoetTest {
+  function _before() {
+    $this->setMailPoetSendingMethod();
+    $this->fillPremiumKey();
+  }
+
   function testItDoesNotCheckMSSKeyIfMPSendingServiceIsDisabled() {
     $this->disableMailPoetSendingMethod();
     $result = ServicesChecker::isMailPoetAPIKeyValid();
     expect($result)->null();
   }
 
-  function testItChecksMSSKeyValidity() {
+  function testItReturnsTrueIfMSSKeyIsValid() {
-    $this->setMailPoetSendingMethod();
     Setting::setValue(
       Bridge::API_KEY_STATE_SETTING_NAME,
       array('state' => Bridge::MAILPOET_KEY_VALID)
     );
     $result = ServicesChecker::isMailPoetAPIKeyValid();
     expect($result)->true();
+  }
 
+  function testItReturnsFalseIfMSSKeyIsInvalid() {
     Setting::setValue(
       Bridge::API_KEY_STATE_SETTING_NAME,
       array('state' => Bridge::MAILPOET_KEY_INVALID)
     );
     $result = ServicesChecker::isMailPoetAPIKeyValid();
     expect($result)->false();
+  }
 
+  function testItReturnsTrueIfMSSKeyIsExpiring() {
     Setting::setValue(
       Bridge::API_KEY_STATE_SETTING_NAME,
       array(
@@ -37,8 +45,9 @@ class ServicesCheckerTest extends MailPoetTest {
     );
     $result = ServicesChecker::isMailPoetAPIKeyValid();
     expect($result)->true();
+  }
 
-    // unexpected state should be treated as valid
+  function testItReturnsTrueIfMSSKeyStateIsUnexpected() {
     Setting::setValue(
       Bridge::API_KEY_STATE_SETTING_NAME,
       array(
@@ -49,35 +58,40 @@ class ServicesCheckerTest extends MailPoetTest {
     expect($result)->true();
   }
 
-  function testItDoesNotCheckPremiumKeyIfPremiumKeyIsNotSpecified() {
+  function testItReturnsFalseIfPremiumKeyIsNotSpecified() {
     $this->clearPremiumKey();
     $result = ServicesChecker::isPremiumKeyValid();
-    expect($result)->null();
+    expect($result)->false();
   }
 
-  function testItChecksPremiumKeyValidity() {
+  function testItReturnsTrueIfPremiumKeyIsValid() {
-    $this->fillPremiumKey();
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array('state' => Bridge::PREMIUM_KEY_VALID)
     );
     $result = ServicesChecker::isPremiumKeyValid();
     expect($result)->true();
+  }
 
+  function testItReturnsFalseIfPremiumKeyIsInvalid() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array('state' => Bridge::PREMIUM_KEY_INVALID)
     );
     $result = ServicesChecker::isPremiumKeyValid();
     expect($result)->false();
+  }
 
+  function testItReturnsFalseIfPremiumKeyIsAlreadyUsed() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array('state' => Bridge::PREMIUM_KEY_ALREADY_USED)
     );
     $result = ServicesChecker::isPremiumKeyValid();
     expect($result)->false();
+  }
 
+  function testItReturnsTrueIfPremiumKeyIsExpiring() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array(
@@ -87,8 +101,9 @@ class ServicesCheckerTest extends MailPoetTest {
     );
     $result = ServicesChecker::isPremiumKeyValid();
     expect($result)->true();
+  }
 
-    // unexpected state should be treated as invalid
+  function testItReturnsFalseIfPremiumKeyStateIsUnexpected() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array(
@@ -97,8 +112,9 @@ class ServicesCheckerTest extends MailPoetTest {
     );
     $result = ServicesChecker::isPremiumKeyValid();
     expect($result)->false();
+  }
 
-    // empty state should be treated as invalid
+  function testItReturnsFalseIfPremiumKeyStateIsEmpty() {
     Setting::setValue(
       Bridge::PREMIUM_KEY_STATE_SETTING_NAME,
       array(