Fix ServicesChecker behavior, make MSS check stricter [MAILPOET-890]

lib/Config/Menu.php

@@ -305,14 +305,18 @@ class Menu {
     $settings = Setting::getAll();
     $flags = $this->_getFlags();
 
+    // force MSS key check even if the method isn't active
+    $checker = new ServicesChecker();
+    $mp_api_key_valid = $checker->isMailPoetAPIKeyValid(false, true);
+
     $data = array(
       'settings' => $settings,
       'segments' => Segment::getSegmentsWithSubscriberCount(),
       'cron_trigger' => CronTrigger::getAvailableMethods(),
       'total_subscribers' => Subscriber::getTotalSubscribers(),
       'premium_plugin_active' => License::getLicense(),
-      'premium_key_valid' => isset($this->premium_key_valid) ? $this->premium_key_valid : null,
-      'mss_key_valid' => isset($this->mp_api_key_valid) ? $this->mp_api_key_valid : null,
+      'premium_key_valid' => !empty($this->premium_key_valid),
+      'mss_key_valid' => !empty($mp_api_key_valid),
       'pages' => Pages::getAll(),
       'flags' => $flags,
       'current_user' => wp_get_current_user(),

lib/Config/ServicesChecker.php

@@ -12,17 +12,18 @@ use MailPoet\WP\Notice as WPNotice;
 if(!defined('ABSPATH')) exit;
 
 class ServicesChecker {
-  function isMailPoetAPIKeyValid($display_error_notice = true) {
-    if(!Bridge::isMPSendingServiceEnabled()) {
-      $display_error_notice = false;
+  function isMailPoetAPIKeyValid($display_error_notice = true, $force_check = false) {
+    if(!$force_check && !Bridge::isMPSendingServiceEnabled()) {
+      return null;
     }
 
+    $mss_key_specified = Bridge::isMSSKeySpecified();
     $mss_key = Setting::getValue(Bridge::API_KEY_STATE_SETTING_NAME);
-    if(empty($mss_key['state']) || $mss_key['state'] == Bridge::MAILPOET_KEY_VALID) {
-      return true;
-    }
 
-    if($mss_key['state'] == Bridge::MAILPOET_KEY_INVALID) {
+    if(!$mss_key_specified
+      || empty($mss_key['state'])
+      || $mss_key['state'] == Bridge::MAILPOET_KEY_INVALID
+    ) {
       if($display_error_notice) {
         $error = Helpers::replaceLinkTags(
           __('All sending is currently paused! Your key to send with MailPoet is invalid. [link]Visit MailPoet.com to purchase a key[/link]', 'mailpoet'),
@@ -45,9 +46,11 @@ class ServicesChecker {
         WPNotice::displayWarning($error);
       }
       return true;
+    } elseif($mss_key['state'] == Bridge::MAILPOET_KEY_VALID) {
+      return true;
     }
 
-    return true;
+    return false;
   }
 
   function isPremiumKeyValid($display_error_notice = true) {

lib/Services/Bridge.php

@@ -42,6 +42,11 @@ class Bridge {
     }
   }
 
+  static function isMSSKeySpecified() {
+    $key = Setting::getValue(self::API_KEY_SETTING_NAME);
+    return !empty($key);
+  }
+
   static function isPremiumKeySpecified() {
     $key = Setting::getValue(self::PREMIUM_KEY_SETTING_NAME);
     return !empty($key);

tests/unit/Config/ServicesCheckerTest.php

@@ -11,10 +11,22 @@ class ServicesCheckerTest extends MailPoetTest {
     $this->fillPremiumKey();
   }
 
-  function testItChecksMSSKeyIfMPSendingServiceIsDisabled() {
+  function testItDoesNotCheckMSSKeyIfMPSendingServiceIsDisabled() {
     $this->disableMailPoetSendingMethod();
     $result = ServicesChecker::isMailPoetAPIKeyValid();
-    expect($result)->true();
+    expect($result)->null();
+  }
+
+  function testItForciblyChecksMSSKeyIfMPSendingServiceIsDisabled() {
+    $this->disableMailPoetSendingMethod();
+    $result = ServicesChecker::isMailPoetAPIKeyValid(false, true);
+    expect($result)->false();
+  }
+
+  function testItReturnsFalseIfMSSKeyIsNotSpecified() {
+    Setting::setValue(Bridge::API_KEY_SETTING_NAME, '');
+    $result = ServicesChecker::isMailPoetAPIKeyValid();
+    expect($result)->false();
   }
 
   function testItReturnsTrueIfMSSKeyIsValid() {
@@ -47,7 +59,7 @@ class ServicesCheckerTest extends MailPoetTest {
     expect($result)->true();
   }
 
-  function testItReturnsTrueIfMSSKeyStateIsUnexpected() {
+  function testItReturnsFalseIfMSSKeyStateIsUnexpected() {
     Setting::setValue(
       Bridge::API_KEY_STATE_SETTING_NAME,
       array(
@@ -55,7 +67,18 @@ class ServicesCheckerTest extends MailPoetTest {
       )
     );
     $result = ServicesChecker::isMailPoetAPIKeyValid();
-    expect($result)->true();
+    expect($result)->false();
+  }
+
+  function testItReturnsFalseIfMSSKeyStateIsEmpty() {
+    Setting::setValue(
+      Bridge::API_KEY_STATE_SETTING_NAME,
+      array(
+        'state' => ''
+      )
+    );
+    $result = ServicesChecker::isMailPoetAPIKeyValid();
+    expect($result)->false();
   }
 
   function testItReturnsFalseIfPremiumKeyIsNotSpecified() {