Unit tests fixed + models & routers update

lib/Config/Widget.php

@@ -12,7 +12,6 @@ class Widget {
     $this->registerWidget();
 
     if(!is_admin()) {
-      //$this->setupActions();
       $this->setupDependencies();
     } else {
       $this->setupAdminDependencies();
@@ -69,6 +68,9 @@ class Widget {
     }
   }
 
+  // TODO: extract this method into an Initializer
+  // - the "ajax" part might probably be useless
+  // - the "post" (non-ajax) part needs to be redone properly
   function setupActions() {
     // ajax requests
     add_action(

lib/Models/Model.php

@@ -39,7 +39,6 @@ class Model extends \Sudzy\ValidModel {
     $this->setTimestamp();
     try {
       parent::save();
-      return true;
     } catch(\Sudzy\ValidationException $e) {
       $this->setError($e->getValidationErrors());
     } catch(\PDOException $e) {
@@ -47,7 +46,7 @@ class Model extends \Sudzy\ValidModel {
     } catch(\Exception $e) {
       $this->setError($e->getMessage());
     }
-    return false;
+    return $this;
   }
 
   function trash() {

lib/Models/NewsletterTemplate.php

@@ -32,16 +32,7 @@ class NewsletterTemplate extends Model {
       $template->set($data);
     }
 
-    $saved = $template->save();
-
-    if($saved === true) {
-      return true;
-    } else {
-      $errors = $template->getValidationErrors();
-      if(!empty($errors)) {
-        return $errors;
-      }
-    }
-    return false;
+    $template->save();
+    return $template;
   }
 }

lib/Models/Segment.php

@@ -164,8 +164,7 @@ class Segment extends Model {
       $segment->set($data);
     }
 
-    $segment->save();
-    return $segment;
+    return $segment->save();
   }
 
   static function getPublic() {

lib/Models/Setting.php

@@ -10,8 +10,7 @@ class Setting extends Model {
     parent::__construct();
 
     $this->addValidations('name', array(
-      'required' => 'name_is_blank',
-      'isString' => 'name_is_not_string'
+      'required' => __('You need to specify a name.')
     ));
   }
 
@@ -55,10 +54,11 @@ class Setting extends Model {
         $value = serialize($value);
       }
 
-      return Setting::createOrUpdate(array(
+      $setting = Setting::createOrUpdate(array(
         'name' => $key,
         'value' => $value
       ));
+      return ($setting->id() > 0 && $setting->getErrors() === false);
     } else {
       $main_key = array_shift($keys);
 
@@ -101,8 +101,7 @@ class Setting extends Model {
   }
 
   public static function createOrUpdate($model) {
-    $exists = self::where('name', $model['name'])
-      ->find_one();
+    $exists = self::where('name', $model['name'])->findOne();
 
     if($exists === false) {
       $new_model = self::create();

lib/Router/Forms.php

@@ -202,37 +202,28 @@ class Forms {
   }
 
   function restore($id) {
-    $result = false;
-
     $form = Form::findOne($id);
     if($form !== false) {
-      $result = $form->restore();
+      $form->restore();
     }
-
-    return $result;
+    return ($form->getErrors() === false);
   }
 
   function trash($id) {
-    $result = false;
-
     $form = Form::findOne($id);
     if($form !== false) {
-      $result = $form->trash();
+      $form->trash();
     }
-
-    return $result;
+    return ($form->getErrors() === false);
   }
 
   function delete($id) {
-    $result = false;
-
     $form = Form::findOne($id);
     if($form !== false) {
       $form->delete();
-      $result = 1;
+      return 1;
     }
-
-    return $result;
+    return false;
   }
 
   function duplicate($id) {

lib/Router/Newsletters.php

@@ -85,17 +85,17 @@ class Newsletters {
         NewsletterOption::where('newsletter_id', $newsletter->id)
           ->deleteMany();
 
-        $optionFields = NewsletterOptionField::where(
+        $option_fields = NewsletterOptionField::where(
           'newsletter_type',
           $data['type']
         )->findArray();
 
-        foreach($optionFields as $optionField) {
-          if(isset($options[$optionField['name']])) {
+        foreach($option_fields as $option_field) {
+          if(isset($options[$option_field['name']])) {
             $relation = NewsletterOption::create();
             $relation->newsletter_id = $newsletter->id;
-            $relation->option_field_id = $optionField['id'];
-            $relation->value = $options[$optionField['name']];
+            $relation->option_field_id = $option_field['id'];
+            $relation->value = $options[$option_field['name']];
             $relation->save();
           }
         }
@@ -263,15 +263,14 @@ class Newsletters {
         $sender = false,
         $reply_to = false
       );
+      $result = $mailer->send($newsletter, $data['subscriber'])
 
-      wp_send_json(array(
-        'result' => $mailer->send($newsletter, $data['subscriber'])
-      ));
+      return array('result' => $result);
     } catch(\Exception $e) {
-      wp_send_json(array(
+      return array(
         'result' => false,
         'errors' => array($e->getMessage()),
-      ));
+      );
     }
   }
 
@@ -299,7 +298,7 @@ class Newsletters {
       $item['queue'] = ($queue !== false) ? $queue->asArray() : null;
     }
 
-    wp_send_json($listing_data);
+    return $listing_data;
   }
 
   function bulkAction($data = array()) {
@@ -307,7 +306,7 @@ class Newsletters {
       '\MailPoet\Models\Newsletter',
       $data
     );
-    wp_send_json($bulk_action->apply());
+    return $bulk_action->apply();
   }
 
   function create($data = array()) {
@@ -328,25 +327,28 @@ class Newsletters {
       unset($data['options']);
     }
 
-    $result = $newsletter->save();
-    if($result !== true) {
-      wp_send_json($newsletter->getValidationErrors());
+    $newsletter->save();
+    $errors = $newsletter->getErrors();
+    if(!empty($errors)) {
+      return $errors;
     } else {
       if(!empty($options)) {
-        $optionFields = NewsletterOptionField::where('newsletter_type', $newsletter->type)->findArray();
+        $option_fields = NewsletterOptionField::where(
+          'newsletter_type', $newsletter->type
+        )->findArray();
 
-        foreach($optionFields as $optionField) {
-          if(isset($options[$optionField['name']])) {
+        foreach($option_fields as $option_field) {
+          if(isset($options[$option_field['name']])) {
             $relation = NewsletterOption::create();
             $relation->newsletter_id = $newsletter->id;
-            $relation->option_field_id = $optionField['id'];
-            $relation->value = $options[$optionField['name']];
+            $relation->option_field_id = $option_field['id'];
+            $relation->value = $options[$option_field['name']];
             $relation->save();
           }
         }
       }
       $newsletter->body = json_decode($newsletter->body);
-      wp_send_json($newsletter->asArray());
+      return $newsletter->asArray();
     }
   }
 }

lib/Router/Segments.php

@@ -87,37 +87,28 @@ class Segments {
   }
 
   function restore($id) {
-    $result = false;
-
     $segment = Segment::findOne($id);
     if($segment !== false) {
-      $result = $segment->restore();
+      $segment->restore();
     }
-
-    return $result;
+    return ($segment->getErrors() === false);
   }
 
   function trash($id) {
-    $result = false;
-
     $segment = Segment::findOne($id);
     if($segment !== false) {
-      $result = $segment->trash();
+      $segment->trash();
     }
-
-    return $result;
+    return ($segment->getErrors() === false);
   }
 
   function delete($id) {
-    $result = false;
-
     $segment = Segment::findOne($id);
     if($segment !== false) {
       $segment->delete();
-      $result = 1;
+      return 1;
     }
-
-    return $result;
+    return false;
   }
 
   function duplicate($id) {

lib/Router/Subscribers.php

@@ -191,37 +191,28 @@ class Subscribers {
   }
 
   function restore($id) {
-    $result = false;
-
     $subscriber = Subscriber::findOne($id);
     if($subscriber !== false) {
-      $result = $subscriber->restore();
+      $subscriber->restore();
     }
-
-    return $result;
+    return ($subscriber->getErrors() === false);
   }
 
   function trash($id) {
-    $result = false;
-
     $subscriber = Subscriber::findOne($id);
     if($subscriber !== false) {
-      $result = $subscriber->trash();
+      $subscriber->trash();
     }
-
-    return $result;
+    return ($subscriber->getErrors() === false);
   }
 
   function delete($id) {
-    $result = false;
-
     $subscriber = Subscriber::findOne($id);
     if($subscriber !== false) {
       $subscriber->delete();
-      $result = 1;
+      return 1;
     }
-
-    return $result;
+    return false;
   }
 
   function bulkAction($data = array()) {

tests/unit/Models/CustomFieldCest.php

@@ -30,7 +30,8 @@ class CustomFieldCest {
   }
 
   function itCanBeCreated() {
-    expect($this->saved)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itCanHaveName() {
@@ -46,10 +47,14 @@ class CustomFieldCest {
   }
 
   function itHasToBeValid() {
-    $empty_model = CustomField::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(2);
+    $invalid_custom_field = CustomField::create();
+
+    $result = $invalid_custom_field->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
+    expect($errors[1])->equals('You need to specify a type.');
   }
 
   function itHasACreatedAtOnCreation() {

tests/unit/Models/FormCest.php

@@ -14,15 +14,17 @@ class FormCest {
   }
 
   function itCanBeCreated() {
-    expect($this->saved)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasToBeValid() {
-    expect($this->saved)->equals(true);
-    $empty_model = Form::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(1);
+    $invalid_form = Form::create();
+    $result = $invalid_form->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
   }
 
   function itHasACreatedAtOnCreation() {

tests/unit/Models/NewsletterCest.php

@@ -19,11 +19,12 @@ class NewsletterCest {
     $newsletter = Newsletter::create();
     $newsletter->hydrate($this->data);
     $this->newsletter = $newsletter;
-    $this->result = $newsletter->save();
+    $this->saved = $newsletter->save();
   }
 
   function itCanBeCreated() {
-    expect($this->result)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasSubject() {

tests/unit/Models/NewsletterOptionFieldCest.php

@@ -29,7 +29,8 @@ class NewsletterOptionFieldCest {
   }
 
   function itCanBeCreated() {
-    expect($this->saved)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasName() {
@@ -45,11 +46,13 @@ class NewsletterOptionFieldCest {
   }
 
   function itHasToBeValid() {
-    expect($this->saved)->equals(true);
-    $empty_model = NewsletterOptionField::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(2);
+    $invalid_newsletter_option = NewsletterOptionField::create();
+    $result = $invalid_newsletter_option->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
+    expect($errors[1])->equals('You need to specify a newsletter type.');
   }
 
   function itHasACreatedAtOnCreation() {

tests/unit/Models/NewsletterTemplateCest.php

@@ -13,18 +13,22 @@ class NewsletterTemplateCest {
 
     $template = NewsletterTemplate::create();
     $template->hydrate($this->data);
-    $this->result = $template->save();
+    $this->saved = $template->save();
   }
 
   function itCanBeCreated() {
-    expect($this->result)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasToBeValid() {
-    $empty_model = NewsletterTemplate::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(2);
+    $invalid_newsletter_template = NewsletterTemplate::create();
+    $result = $invalid_newsletter_template->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
+    expect($errors[1])->equals('Template body cannot be empty.');
   }
 
   function itHasName() {
@@ -46,25 +50,28 @@ class NewsletterTemplateCest {
   }
 
   function itCanCreateOrUpdate() {
-    $is_created = NewsletterTemplate::createOrUpdate(
+    $created_template = NewsletterTemplate::createOrUpdate(
       array(
         'name' => 'Another template',
         'description' => 'Another template description',
         'body' => '{content: {}, globalStyles: {}}',
       ));
-    expect($is_created)->equals(true);
+    expect($created_template->id() > 0)->true();
+    expect($created_template->getErrors())->false();
 
     $template = NewsletterTemplate::where('name', 'Another template')
       ->findOne();
     expect($template->name)->equals('Another template');
 
-    $is_updated = NewsletterTemplate::createOrUpdate(
+    $updated_template = NewsletterTemplate::createOrUpdate(
       array(
         'id' => $template->id,
         'name' => 'Another template updated',
         'body' => '{}'
       ));
-    expect($is_updated)->equals(true);
+    expect($updated_template->id() > 0)->true();
+    expect($updated_template->getErrors())->false();
+
     $template = NewsletterTemplate::findOne($template->id);
     expect($template->name)->equals('Another template updated');
   }

tests/unit/Models/SegmentCest.php

@@ -41,7 +41,8 @@ class SegmentCest {
   }
 
   function itCanBeCreated() {
-    expect($this->saved)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itCanHaveName() {
@@ -51,7 +52,9 @@ class SegmentCest {
   function nameMustBeUnique() {
     $segment = Segment::create();
     $segment->hydrate($this->data);
-    $errors = $segment->save();
+    $result = $segment->save();
+    $errors = $result->getErrors();
+
     expect(is_array($errors))->true();
     expect($errors[0])->contains('Duplicate');
   }
@@ -61,11 +64,13 @@ class SegmentCest {
   }
 
   function itHasToBeValid() {
-    expect($this->saved)->equals(true);
-    $empty_model = Segment::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(1);
+    $invalid_segment = Segment::create();
+
+    $result = $invalid_segment->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
   }
 
   function itHasACreatedAtOnCreation() {

tests/unit/Models/SettingCest.php

@@ -11,19 +11,21 @@ class SettingCest {
 
     $setting = Setting::create();
     $setting->hydrate($this->data);
-    $this->result = $setting->save();
+    $this->saved = $setting->save();
   }
 
   function itCanBeCreated() {
-   expect($this->result)->equals(true);
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasToBeValid() {
-    expect($this->result)->equals(true);
-    $empty_model = Setting::create();
-    expect($empty_model->save())->notEquals(true);
-    $validations = $empty_model->getValidationErrors();
-    expect(count($validations))->equals(2);
+    $invalid_setting = Setting::create();
+    $result = $invalid_setting->save();
+    $errors = $result->getErrors();
+
+    expect(is_array($errors))->true();
+    expect($errors[0])->equals('You need to specify a name.');
   }
 
   function itHasACreatedAtOnCreation() {
@@ -65,18 +67,20 @@ class SettingCest {
       'value' => 'data'
     );
 
-    $result = Setting::createOrUpdate($data);
-    expect($result)->equals(true);
-    $record = Setting::where('name', $data['name'])
-      ->find_one();
-    expect($record->value)->equals($data['value']);
+    $created_setting = Setting::createOrUpdate($data);
+    expect($created_setting->id() > 0)->true();
+    expect($created_setting->getErrors())->false();
+
+    $setting = Setting::where('name', $data['name'])->findOne();
+    expect($setting->value)->equals($data['value']);
 
     $data['value'] = 'new data';
-    $result = Setting::createOrUpdate($data);
-    expect($result)->equals(true);
-    $record = Setting::where('name', $data['name'])
-      ->find_one();
-    expect($record->value)->equals('new data');
+    $updated_setting = Setting::createOrUpdate($data);
+    expect($updated_setting->id() > 0)->true();
+    expect($updated_setting->getErrors())->false();
+
+    $setting = Setting::where('name', $data['name'])->findOne();
+    expect($setting->value)->equals('new data');
   }
 
   function itCanGetAndSetValue() {

tests/unit/Models/SubscriberCest.php

@@ -20,7 +20,8 @@ class SubscriberCest {
   }
 
   function itCanBeCreated() {
-    expect($this->saved)->true();
+    expect($this->saved->id() > 0)->true();
+    expect($this->saved->getErrors())->false();
   }
 
   function itHasFirstName() {
@@ -62,14 +63,13 @@ class SubscriberCest {
   }
 
   function itCanChangeStatus() {
-    $subscriber = Subscriber::where('email', $this->data['email'])
-      ->findOne();
+    $subscriber = Subscriber::where('email', $this->data['email'])->findOne();
     $subscriber->status = 'subscribed';
-    expect($subscriber->save())->equals(true);
-    $subscriber_updated = Subscriber::where(
-      'email',
-      $this->data['email']
-    )
+    $subscriber->save();
+
+    expect($subscriber->id() > 0)->true();
+    expect($subscriber->getErrors())->false();
+    $subscriber_updated = Subscriber::where('email', $this->data['email'])
       ->findOne();
     expect($subscriber_updated->status)->equals('subscribed');
   }
@@ -89,17 +89,17 @@ class SubscriberCest {
   function itHasGroupFilter() {
     $subscribers = Subscriber::filter('groupBy', 'unconfirmed')
       ->findMany();
-    foreach ($subscribers as $subscriber) {
+    foreach($subscribers as $subscriber) {
       expect($subscriber->status)->equals('unconfirmed');
     }
     $subscribers = Subscriber::filter('groupBy', 'subscribed')
       ->findMany();
-    foreach ($subscribers as $subscriber) {
+    foreach($subscribers as $subscriber) {
       expect($subscriber->status)->equals('subscribed');
     }
     $subscribers = Subscriber::filter('groupBy', 'unsubscribed')
       ->findMany();
-    foreach ($subscribers as $subscriber) {
+    foreach($subscribers as $subscriber) {
       expect($subscriber->status)->equals('unsubscribed');
     }
   }
@@ -150,7 +150,7 @@ class SubscriberCest {
         'type' => 'text',
       )
     );
-    foreach ($customFieldData as $data) {
+    foreach($customFieldData as $data) {
       $customField = CustomField::create();
       $customField->hydrate($data);
       $customField->save();
@@ -168,7 +168,7 @@ class SubscriberCest {
         'value' => 'France'
       )
     );
-    foreach ($subscriberCustomFieldData as $data) {
+    foreach($subscriberCustomFieldData as $data) {
       $association = SubscriberCustomField::create();
       $association->hydrate($data);
       $association->save();

tests/unit/Models/SubscriberCustomFieldCest.php

@@ -20,7 +20,9 @@ class SubscriberCustomFieldCest {
   function itCanBeCreated() {
     $subscriberCustomField = SubscriberCustomField::create();
     $subscriberCustomField->hydrate($this->data[0]);
-    expect($subscriberCustomField->save())->true();
+    $subscriberCustomField->save();
+    expect($subscriberCustomField->id() > 0)->true();
+    expect($subscriberCustomField->getErrors())->false();
   }
 
   function itCanCreateOrUpdateMultipleRecords() {