bugfix + refactoring

lib/Models/Setting.php

@@ -6,6 +6,8 @@ if (!defined('ABSPATH')) exit;
 class Setting extends Model {
   public static $_table = MP_SETTINGS_TABLE;
 
+  public static $defaults = null;
+
   function __construct() {
     parent::__construct();
 
@@ -14,8 +16,26 @@ class Setting extends Model {
     ));
   }
 
+  public static function getDefaults() {
+    if(self::$defaults === null) {
+      self::loadDefaults();
+    }
+    return self::$defaults;
+  }
+
+  public static function loadDefaults() {
+    self::$defaults = array(
+      'signup_confirmation' => array(
+        'enabled' => true,
+        'subject' => sprintf(__('Confirm your subscription to %1$s'), get_option('blogname')),
+        'body' => __("Hello!\n\nHurray! You've subscribed to our site.\nWe need you to activate your subscription to the list(s): [lists_to_confirm] by clicking the link below: \n\n[activation_link]Click here to confirm your subscription.[/activation_link]\n\nThank you,\n\nThe team!")
+      )
+    );
+  }
+
   public static function getValue($key, $default = null) {
     $keys = explode('.', $key);
+    $defaults = self::getDefaults();
 
     if(count($keys) === 1) {
       $setting = Setting::where('name', $key)->findOne();
@@ -23,9 +43,14 @@ class Setting extends Model {
         return $default;
       } else {
         if(is_serialized($setting->value)) {
-          return unserialize($setting->value);
+          $value = unserialize($setting->value);
         } else {
-          return $setting->value;
+          $value = $setting->value;
+        }
+        if(is_array($value) && array_key_exists($key, $defaults)) {
+          return array_replace_recursive($defaults[$key], $value);
+        } else {
+          return $value;
         }
       }
     } else {
@@ -93,7 +118,7 @@ class Setting extends Model {
         $settings[$setting->name] = $value;
       }
     }
-    return $settings;
+    return array_replace_recursive(self::getDefaults(), $settings);
   }
 
   public static function createOrUpdate($data = array()) {

lib/Models/Subscriber.php

@@ -115,6 +115,7 @@ class Subscriber extends Model {
       $segment_names = array_map(function($segment) {
         return $segment->name;
       }, $segments);
+
       $body = nl2br($signup_confirmation['body']);
 
       // replace list of segments shortcode
@@ -152,14 +153,14 @@ class Subscriber extends Model {
       // set from
       $from = (
         !empty($signup_confirmation['from'])
-        && !empty($signup_confirmation['email'])
+        && !empty($signup_confirmation['from']['email'])
       ) ? $signup_confirmation['from']
         : false;
 
       // set reply to
       $reply_to = (
         !empty($signup_confirmation['reply_to'])
-        && !empty($signup_confirmation['reply_to'])
+        && !empty($signup_confirmation['reply_to']['email'])
       ) ? $signup_confirmation['reply_to']
         : false;
 
@@ -168,6 +169,7 @@ class Subscriber extends Model {
         $mailer = new Mailer(false, $from, $reply_to);
         return $mailer->send($email, $subscriber);
       } catch(\Exception $e) {
+        $this->setError($e->getMessage());
         return false;
       }
     }

lib/Router/Subscribers.php

@@ -88,18 +88,10 @@ class Subscribers {
     }
 
     $subscriber = Subscriber::subscribe($data, $segment_ids);
-
+    if($subscriber->getErrors() !== false) {
-    $result = false;
-    if($subscriber === false || !$subscriber->id()) {
-      $errors = array_merge($errors, $subscriber->getValidationErrors());
-    } else {
-      $result = true;
-    }
-
-    if(!empty($errors)) {
       return array(
         'result' => false,
-        'errors' => $errors
+        'errors' => $subscriber->getErrors()
       );
     }
 

lib/Subscription/Pages.php

@@ -8,6 +8,8 @@ use \MailPoet\Models\Segment;
 use \MailPoet\Util\Helpers;
 
 class Pages {
+  const DEMO_EMAIL = 'demo@mailpoet.com';
+
   function __construct() {
 
   }
@@ -24,7 +26,236 @@ class Pages {
     // TODO
   }
 
-  function getSubscriber() {
+  function setPageTitle($title = null) {
+    $subscriber = $this->getSubscriber();
+
+    switch($this->getAction()) {
+      case 'confirm':
+        return $this->getConfirmTitle($subscriber);
+      break;
+
+      case 'edit':
+        return $this->getEditTitle($subscriber);
+      break;
+
+      case 'unsubscribe':
+        if($subscriber !== false && $subscriber->email !== self::DEMO_EMAIL) {
+          if($subscriber->status !== Subscriber::STATUS_UNSUBSCRIBED) {
+            $subscriber->status = Subscriber::STATUS_UNSUBSCRIBED;
+            $subscriber->save();
+          }
+        }
+        return $this->getUnsubscribeTitle();
+      break;
+    }
+    return $title;
+  }
+
+  function setPageContent($page_content = '[mailpoet_page]') {
+    $content = '';
+    $subscriber = $this->getSubscriber();
+
+    switch($this->getAction()) {
+      case 'confirm':
+        $content = $this->getConfirmContent($subscriber);
+      break;
+      case 'edit':
+        $content = $this->getEditContent($subscriber);
+      break;
+      case 'unsubscribe':
+        $content = $this->getUnsubscribeContent($subscriber);
+      break;
+    }
+    return str_replace('[mailpoet_page]', $content, $page_content);
+  }
+
+  private function getConfirmTitle($subscriber) {
+    if($subscriber === false) {
+      $title = __('Your confirmation link expired, please subscribe again.');
+    } else {
+      if($subscriber->email === self::DEMO_EMAIL) {
+        $segment_names = array('demo 1', 'demo 2');
+      } else {
+        if($subscriber->status !== Subscriber::STATUS_SUBSCRIBED) {
+          $subscriber->status = Subscriber::STATUS_SUBSCRIBED;
+          $subscriber->save();
+        }
+
+        $segments = $subscriber->segments()->findMany();
+
+        $segment_names = array_map(function($segment) {
+          return $segment->name;
+        }, $segments);
+      }
+
+      if(empty($segment_names)) {
+        $title = __("You've subscribed!");
+      } else {
+        $title = sprintf(
+          __("You've subscribed to: %s"),
+          join(', ', $segment_names)
+        );
+      }
+    }
+    return $title;
+  }
+
+  private function getEditTitle($subscriber) {
+    if($subscriber !== false) {
+      return sprintf(
+        __('Edit your subscriber profile: %s'),
+        $subscriber->email
+      );
+    }
+  }
+
+  private function getUnsubscribeTitle() {
+    return __("You've unsubscribed!");
+  }
+
+
+  private function getConfirmContent($subscriber) {
+    if($subscriber !== false) {
+      return __("Yup, we've added you to our list. You'll hear from us shortly.");
+    }
+  }
+
+  private function getEditContent($subscriber) {
+    if($subscriber !== false) {
+      $subscriber = $subscriber
+      ->withCustomFields()
+      ->withSubscriptions();
+
+      $custom_fields = array_map(function($custom_field) use($subscriber) {
+        $custom_field->id = 'cf_'.$custom_field->id;
+        $custom_field = $custom_field->asArray();
+        $custom_field['params']['value'] = $subscriber->{$custom_field['id']};
+        return $custom_field;
+      }, CustomField::findMany());
+
+      $segment_ids = Setting::getValue('subscription.segments', array());
+      if(!empty($segment_ids)) {
+        $segments = Segment::getPublic()
+          ->whereIn('id', $segment_ids)
+          ->findMany();
+      } else {
+        $segments = Segment::getPublic()->findMany();
+      }
+
+      $subscribed_segment_ids = Helpers::arrayColumn(
+        $subscriber->subscriptions, 'id'
+      );
+
+      $segments = array_map(function($segment) use($subscribed_segment_ids) {
+        return array(
+          'id' => $segment->id,
+          'name' => $segment->name,
+          'is_checked' => in_array($segment->id, $subscribed_segment_ids)
+        );
+      }, $segments);
+
+      $fields = array(
+        array(
+          'id' => 'email',
+          'type' => 'text',
+          'params' => array(
+            'label' => __('Email'),
+            'required' => true,
+            'value' => $subscriber->email
+          )
+        ),
+        array(
+          'id' => 'first_name',
+          'type' => 'text',
+          'params' => array(
+            'label' => __('First name'),
+            'value' => $subscriber->first_name
+          )
+        ),
+        array(
+          'id' => 'last_name',
+          'type' => 'text',
+          'params' => array(
+            'label' => __('Last name'),
+            'value' => $subscriber->last_name
+          )
+        ),
+        array(
+          'id' => 'status',
+          'type' => 'select',
+          'params' => array(
+            'label' => __('Status'),
+            'values' => array(
+              array(
+                'value' => array(
+                  Subscriber::STATUS_SUBSCRIBED => __('Subscribed')
+                ),
+                'is_checked' => (
+                  $subscriber->status === Subscriber::STATUS_SUBSCRIBED
+                )
+              ),
+              array(
+                'value' => array(
+                  Subscriber::STATUS_UNSUBSCRIBED => __('Unsubscribed')
+                ),
+                'is_checked' => (
+                  $subscriber->status === Subscriber::STATUS_UNSUBSCRIBED
+                )
+              ),
+              array(
+                'value' => array(
+                  Subscriber::STATUS_UNCONFIRMED => __('Unconfirmed')
+                ),
+                'is_checked' => (
+                  $subscriber->status === Subscriber::STATUS_UNCONFIRMED
+                )
+              )
+            )
+          )
+        )
+      );
+
+      $form = array_merge(
+        $fields,
+        $custom_fields,
+        array(
+          array(
+            'id' => 'segment',
+            'type' => 'segment',
+            'params' => array(
+              'label' => __('Your lists'),
+              'values' => $segments
+            )
+          ),
+          array(
+            'id' => 'submit',
+            'type' => 'submit',
+            'params' => array(
+              'label' => __('Subscribe!')
+            )
+          )
+        )
+      );
+
+      $content = \MailPoet\Form\Renderer::renderBlocks($form);
+    }
+  }
+
+  private function getUnsubscribeContent($subscriber) {
+    $content = '<p>'.__("Great, you'll never hear from us again!").'</p>';
+    if($subscriber !== false) {
+      $content .= '<p><strong>'.
+        str_replace(
+          array('[link]', '[/link]'),
+          array('<a href="'.$subscriber->getConfirmationUrl().'">', '</a>'),
+          __('You made a mistake? [link]Undo unsubscribe.[/link]')
+        ).
+      '</strong></p>';
+    }
+    return $content;
+  }
+
+  private function getSubscriber() {
     $token = (isset($_GET['mailpoet_token']))
       ? $_GET['mailpoet_token']
       : null;
@@ -32,9 +263,9 @@ class Pages {
       ? $_GET['mailpoet_email']
       : null;
 
-    if(empty($token) || empty($email)) {
+    if(empty($token) || empty($email) || $email === self::DEMO_EMAIL) {
       $subscriber = Subscriber::create();
-      $subscriber->email = 'demo@mailpoet.com';
+      $subscriber->email = self::DEMO_EMAIL;
       return $subscriber;
     }
 
@@ -47,210 +278,6 @@ class Pages {
     return false;
   }
 
-  function setPageTitle($title = null) {
-    $subscriber = $this->getSubscriber();
-
-    switch($this->getAction()) {
-      case 'confirm':
-        if($subscriber === false) {
-          $title = __('Your confirmation link expired, please subscribe again.');
-        } else {
-          if($subscriber->email === 'demo@mailpoet.com') {
-            $segment_names = array('demo 1', 'demo 2');
-          } else {
-            if($subscriber->status !== Subscriber::STATUS_SUBSCRIBED) {
-              $subscriber->status = Subscriber::STATUS_SUBSCRIBED;
-              $subscriber->save();
-            }
-
-            $segments = $subscriber->segments()->findMany();
-
-            $segment_names = array_map(function($segment) {
-              return $segment->name;
-            }, $segments);
-          }
-
-          if(empty($segment_names)) {
-            $title = __("You've subscribed!");
-          } else {
-            $title = sprintf(
-              __("You've subscribed to: %s"),
-              join(', ', $segment_names)
-            );
-
-          }
-        }
-      break;
-      case 'edit':
-        if($subscriber !== false) {
-          $title = sprintf(
-            __('Edit your subscriber profile: %s'),
-            $subscriber->email
-          );
-        }
-      break;
-      case 'unsubscribe':
-        if($subscriber !== false) {
-          if($subscriber->status !== Subscriber::STATUS_UNSUBSCRIBED) {
-            $subscriber->status = Subscriber::STATUS_UNSUBSCRIBED;
-            $subscriber->save();
-          }
-        }
-        $title = __("You've unsubscribed!");
-      break;
-    }
-    return $title;
-  }
-
-  function setPageContent($page_content = '[mailpoet_page]') {
-    $content = '';
-    $subscriber = $this->getSubscriber();
-
-    switch($this->getAction()) {
-      case 'confirm':
-        if($subscriber !== false) {
-          $content = __(
-            "Yup, we've added you to our list. ".
-            "You'll hear from us shortly."
-          );
-        }
-      break;
-      case 'edit':
-        if($subscriber !== false) {
-          $subscriber = $subscriber
-          ->withCustomFields()
-          ->withSubscriptions();
-
-          $custom_fields = array_map(function($custom_field) use($subscriber) {
-            $custom_field->id = 'cf_'.$custom_field->id;
-            $custom_field = $custom_field->asArray();
-            $custom_field['params']['value'] = $subscriber->{$custom_field['id']};
-            return $custom_field;
-          }, CustomField::findMany());
-
-          $segment_ids = Setting::getValue('subscription.segments', array());
-          if(!empty($segment_ids)) {
-            $segments = Segment::getPublic()
-              ->whereIn('id', $segment_ids)
-              ->findMany();
-          } else {
-            $segments = Segment::getPublic()->findMany();
-          }
-
-          $subscribed_segment_ids = Helpers::arrayColumn(
-            $subscriber->subscriptions, 'id'
-          );
-
-          $segments = array_map(function($segment) use($subscribed_segment_ids) {
-            return array(
-              'id' => $segment->id,
-              'name' => $segment->name,
-              'is_checked' => in_array($segment->id, $subscribed_segment_ids)
-            );
-          }, $segments);
-
-          $fields = array(
-            array(
-              'id' => 'email',
-              'type' => 'text',
-              'params' => array(
-                'label' => __('Email'),
-                'required' => true,
-                'value' => $subscriber->email
-              )
-            ),
-            array(
-              'id' => 'first_name',
-              'type' => 'text',
-              'params' => array(
-                'label' => __('First name'),
-                'value' => $subscriber->first_name
-              )
-            ),
-            array(
-              'id' => 'last_name',
-              'type' => 'text',
-              'params' => array(
-                'label' => __('Last name'),
-                'value' => $subscriber->last_name
-              )
-            ),
-            array(
-              'id' => 'status',
-              'type' => 'select',
-              'params' => array(
-                'label' => __('Status'),
-                'values' => array(
-                  array(
-                    'value' => array(
-                      Subscriber::STATUS_SUBSCRIBED => __('Subscribed')
-                    ),
-                    'is_checked' => (
-                      $subscriber->status === Subscriber::STATUS_SUBSCRIBED
-                    )
-                  ),
-                  array(
-                    'value' => array(
-                      Subscriber::STATUS_UNSUBSCRIBED => __('Unsubscribed')
-                    ),
-                    'is_checked' => (
-                      $subscriber->status === Subscriber::STATUS_UNSUBSCRIBED
-                    )
-                  ),
-                  array(
-                    'value' => array(
-                      Subscriber::STATUS_UNCONFIRMED => __('Unconfirmed')
-                    ),
-                    'is_checked' => (
-                      $subscriber->status === Subscriber::STATUS_UNCONFIRMED
-                    )
-                  )
-                )
-              )
-            )
-          );
-
-          $form = array_merge(
-            $fields,
-            $custom_fields,
-            array(
-              array(
-                'id' => 'segment',
-                'type' => 'segment',
-                'params' => array(
-                  'label' => __('Your lists'),
-                  'values' => $segments
-                )
-              ),
-              array(
-                'id' => 'submit',
-                'type' => 'submit',
-                'params' => array(
-                  'label' => __('Subscribe!')
-                )
-              )
-            )
-          );
-
-          $content = \MailPoet\Form\Renderer::renderBlocks($form);
-        }
-      break;
-      case 'unsubscribe':
-        $content = '<p>'.__("Great, you'll never hear from us again!").'</p>';
-        if($subscriber !== false) {
-          $content .= '<p><strong>'.
-            str_replace(
-              array('[link]', '[/link]'),
-              array('<a href="'.$subscriber->getConfirmationUrl().'">', '</a>'),
-              __('You made a mistake? [link]Undo unsubscribe.[/link]')
-            ).
-          '</strong></p>';
-        }
-      break;
-    }
-    return str_replace('[mailpoet_page]', $content, $page_content);
-  }
-
   private function getAction() {
     return (isset($_GET['mailpoet_action']))
       ? $_GET['mailpoet_action']

tests/unit/Models/SettingCest.php

@@ -19,7 +19,13 @@ class SettingCest {
     expect($errors[0])->equals('You need to specify a name.');
   }
 
-  function itCanGetAllSettings() {
+  function itHasDefaultSettings() {
+    $default_settings = Setting::getDefaults();
+    expect($default_settings)->notEmpty();
+    expect($default_settings['signup_confirmation']['enabled'])->true();
+  }
+
+  function itCanGetAllSettingsIncludingDefaults() {
     Setting::setValue('key_1', 'value_1');
     Setting::setValue('key_2', 'value_2');
     Setting::setValue('key_3', array(
@@ -28,13 +34,17 @@ class SettingCest {
     ));
 
     $settings = Setting::getAll();
-    expect(array_keys($settings))->count(3);
     expect($settings['key_1'])->equals('value_1');
     expect($settings['key_2'])->equals('value_2');
     expect($settings['key_3'])->equals(array(
       'subkey_1' => 'subvalue_1',
       'subkey_2' => 'subvalue_2'
     ));
+
+    // default settings
+    $default_settings = Setting::getDefaults();
+    expect($settings['signup_confirmation'])
+      ->equals($default_settings['signup_confirmation']);
   }
 
   function itReturnsDefaultValueIfNotSet() {

tests/unit/Router/SettingsCest.php

@@ -16,7 +16,7 @@ class SettingsCest {
 
     Setting::deleteMany();
     $settings = $router->get();
-    expect($settings)->isEmpty();
+    expect($settings)->equals(Setting::getDefaults());
   }
 
   function itCanSetSettings() {

views/settings/signup.html

@@ -112,11 +112,6 @@
           name="signup_confirmation[subject]"
           <% if(settings.signup_confirmation.subject) %>
             value="<%= settings.signup_confirmation.subject %>"
-          <% else %>
-            value="<%=
-              __('Confirm your subscription to %1$s')
-              | format(get_option('blogname'))
-            %>"
           <% endif %>
         />
       </td>
@@ -139,8 +134,6 @@
           name="signup_confirmation[body]"
         ><% if(settings.signup_confirmation.body) %>
             <%=- settings.signup_confirmation.body -%>
-          <% else %>
-            <%=- __("Hello!\n\nHurray! You've subscribed to our site.\nWe need you to activate your subscription to the list(s): [lists_to_confirm] by clicking the link below: \n\n[activation_link]Click here to confirm your subscription.[/activation_link]\n\nThank you,\n\nThe team!") -%>
           <% endif %></textarea>
       </td>
     </tr>