Segments & Subscribers endpoints

- unit tests

assets/js/src/listing/listing.jsx

@@ -469,14 +469,6 @@ const Listing = React.createClass({
             // redirect to default group
             this.handleGroup('all');
           }
-
-          // TODO: remove this....
-          if (this.props['onGetItems'] !== undefined) {
-            const count = (response.groups[0] !== undefined)
-              ? ~~(response.groups[0].count)
-              : 0;
-            this.props.onGetItems(count);
-          }
         });
       });
     }

assets/js/src/subscribers/list.jsx

@@ -330,16 +330,23 @@ const SubscriberList = React.createClass({
       </div>
     );
   },
-  onGetItems: function(count) {
-    jQuery('#mailpoet_export_button')[(count > 0) ? 'show' : 'hide']();
-  },
   render: function() {
     return (
       <div>
         <h1 className="title">
-          {MailPoet.I18n.t('pageTitle')} <Link className="page-title-action" to="/new">{MailPoet.I18n.t('new')}</Link>
+          {MailPoet.I18n.t('pageTitle')} <Link
-          <a className="page-title-action" href="?page=mailpoet-import#step1">{MailPoet.I18n.t('import')}</a>
+            className="page-title-action"
-          <a id="mailpoet_export_button" className="page-title-action" href="?page=mailpoet-export">{MailPoet.I18n.t('export')}</a>
+            to="/new"
+          >{MailPoet.I18n.t('new')}</Link>
+          <a
+            className="page-title-action"
+            href="?page=mailpoet-import#step1"
+          >{MailPoet.I18n.t('import')}</a>
+          <a
+            id="mailpoet_export_button"
+            className="page-title-action"
+            href="?page=mailpoet-export"
+          >{MailPoet.I18n.t('export')}</a>
         </h1>
 
         <Listing
@@ -352,7 +359,6 @@ const SubscriberList = React.createClass({
           bulk_actions={ bulk_actions }
           item_actions={ item_actions }
           messages={ messages }
-          onGetItems={ this.onGetItems }
           sort_by={ 'created_at' }
           sort_order={ 'desc' }
         />

lib/API/Endpoints/Segments.php

@@ -51,7 +51,7 @@ class Segments extends APIEndpoint {
     $errors = $segment->getErrors();
 
     if(!empty($errors)) {
-      return $this->errorResponse($errors);
+      return $this->badRequest($errors);
     } else {
       return $this->successResponse(
         Segment::findOne($segment->id)->asArray()

lib/API/Endpoints/Subscribers.php

@@ -1,5 +1,7 @@
 <?php
 namespace MailPoet\API\Endpoints;
+use \MailPoet\API\Endpoint as APIEndpoint;
+use \MailPoet\API\Error as APIError;
 
 use MailPoet\Listing;
 use MailPoet\Models\Subscriber;
@@ -13,19 +15,22 @@ use MailPoet\Util\Url;
 
 if(!defined('ABSPATH')) exit;
 
-class Subscribers {
+class Subscribers extends APIEndpoint {
-  function __construct() {
+  function get($data = array()) {
-  }
+    $id = (isset($data['id']) ? (int)$data['id'] : false);
-
-  function get($id = null) {
     $subscriber = Subscriber::findOne($id);
-    if($subscriber !== false) {
+    if($subscriber === false) {
-      $subscriber = $subscriber
+      return $this->errorResponse(array(
-        ->withCustomFields()
+        APIError::NOT_FOUND => __('This subscriber does not exist.')
-        ->withSubscriptions()
+      ));
-        ->asArray();
+    } else {
+      return $this->successResponse(
+        $subscriber
+          ->withCustomFields()
+          ->withSubscriptions()
+          ->asArray()
+      );
     }
-    return $subscriber;
   }
 
   function listing($data = array()) {
@@ -46,22 +51,6 @@ class Subscribers {
     return $listing_data;
   }
 
-  function save($data = array()) {
-    $subscriber = Subscriber::createOrUpdate($data);
-    $errors = $subscriber->getErrors();
-
-    if(!empty($errors)) {
-      return array(
-        'result' => false,
-        'errors' => $errors
-      );
-    } else {
-      return array(
-        'result' => true
-      );
-    }
-  }
-
   function subscribe($data = array()) {
     $doing_ajax = (bool)(defined('DOING_AJAX') && DOING_AJAX);
     $errors = array();
@@ -148,37 +137,76 @@ class Subscribers {
     }
   }
 
-  function restore($id) {
+  function save($data = array()) {
+    $subscriber = Subscriber::createOrUpdate($data);
+    $errors = $subscriber->getErrors();
+
+    if(!empty($errors)) {
+      return $this->badRequest($errors);
+    } else {
+      return $this->successResponse(
+        Subscriber::findOne($subscriber->id)->asArray()
+      );
+    }
+  }
+
+  function restore($data = array()) {
+    $id = (isset($data['id']) ? (int)$data['id'] : false);
     $subscriber = Subscriber::findOne($id);
-    if($subscriber !== false) {
+    if($subscriber === false) {
+      return $this->errorResponse(array(
+        APIError::NOT_FOUND => __('This subscriber does not exist.')
+      ));
+    } else {
       $subscriber->restore();
+      return $this->successResponse(
+        Subscriber::findOne($subscriber->id)->asArray(),
+        array('count' => 1)
+      );
     }
-    return ($subscriber->getErrors() === false);
   }
 
-  function trash($id) {
+  function trash($data = array()) {
+    $id = (isset($data['id']) ? (int)$data['id'] : false);
     $subscriber = Subscriber::findOne($id);
-    if($subscriber !== false) {
+    if($subscriber === false) {
+      return $this->errorResponse(array(
+        APIError::NOT_FOUND => __('This subscriber does not exist.')
+      ));
+    } else {
       $subscriber->trash();
+      return $this->successResponse(
+        Subscriber::findOne($subscriber->id)->asArray(),
+        array('count' => 1)
+      );
     }
-    return ($subscriber->getErrors() === false);
   }
 
-  function delete($id) {
+  function delete($data = array()) {
+    $id = (isset($data['id']) ? (int)$data['id'] : false);
     $subscriber = Subscriber::findOne($id);
-    if($subscriber !== false) {
+    if($subscriber === false) {
+      return $this->errorResponse(array(
+        APIError::NOT_FOUND => __('This subscriber does not exist.')
+      ));
+    } else {
       $subscriber->delete();
-      return 1;
+      return $this->successResponse(null, array('count' => 1));
     }
-    return false;
   }
 
   function bulkAction($data = array()) {
-    $bulk_action = new Listing\BulkAction(
+    try {
-      '\MailPoet\Models\Subscriber',
+      $bulk_action = new Listing\BulkAction(
-      $data
+        '\MailPoet\Models\Subscriber',
-    );
+        $data
-
+      );
-    return $bulk_action->apply();
+      $count = $bulk_action->apply();
+      return $this->successResponse(null, array('count' => $count));
+    } catch(\Exception $e) {
+      return $this->errorResponse(array(
+        $e->getCode() => $e->getMessage()
+      ));
+    }
   }
 }

lib/Listing/BulkAction.php

@@ -26,7 +26,7 @@ class BulkAction {
 
     if(!method_exists($this->model_class, $bulk_action_method)) {
       throw new \Exception(
-        $this->model_class. ' has not method "'.$bulk_action_method.'"'
+        $this->model_class. ' has no method "'.$bulk_action_method.'"'
       );
     }
 

lib/Models/Subscriber.php

@@ -23,12 +23,13 @@ class Subscriber extends Model {
     ));
   }
 
-  static function findOne($id = null) {
+  static function findOne($id = false) {
     if(is_int($id) || (string)(int)$id === $id) {
       return parent::findOne($id);
-    } else {
+    } else if(strlen(trim($id)) > 0) {
       return parent::where('email', $id)->findOne();
     }
+    return false;
   }
 
   function segments() {

tests/unit/API/SegmentsTest.php

@@ -1,5 +1,6 @@
 <?php
 use \MailPoet\API\Endpoints\Segments;
+use \MailPoet\API\Response as APIResponse;
 use \MailPoet\Models\Segment;
 
 class SegmentsTest extends MailPoetTest {
@@ -13,13 +14,18 @@ class SegmentsTest extends MailPoetTest {
     $router = new Segments();
 
     $response = $router->get(/* missing id */);
-    expect($response)->false();
+    expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
+    expect($response->errors[0]['message'])->equals('This list does not exist.');
 
-    $response = $router->get('not_an_id');
+    $response = $router->get(array('id' => 'not_an_id'));
-    expect($response)->false();
+    expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
+    expect($response->errors[0]['message'])->equals('This list does not exist.');
 
-    $response = $router->get($this->segment_1->id());
+    $response = $router->get(array('id' => $this->segment_1->id));
-    expect($response['name'])->equals($this->segment_1->name);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->data)->equals(
+      Segment::findOne($this->segment_1->id)->asArray()
+    );
   }
 
   function testItCanGetListingData() {
@@ -43,14 +49,14 @@ class SegmentsTest extends MailPoetTest {
 
     $router = new Segments();
     $response = $router->save(/* missing data */);
-    expect($response['result'])->false();
+    expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST);
-    expect($response['errors'][0])->equals('Please specify a name');
+    expect($response->errors[0]['message'])->equals('Please specify a name');
 
     $response = $router->save($segment_data);
-    expect($response['result'])->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $segment = Segment::where('name', 'New Segment')->findOne();
+      Segment::where('name', 'New Segment')->findOne()->asArray()
-    expect($segment->name)->equals($segment_data['name']);
+    );
   }
 
   function testItCannotSaveDuplicate() {
@@ -60,8 +66,8 @@ class SegmentsTest extends MailPoetTest {
 
     $router = new Segments();
     $response = $router->save($duplicate_entry);
-    expect($response['result'])->false();
+    expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST);
-    expect($response['errors'][0])->equals(
+    expect($response->errors[0]['message'])->equals(
       'Another record already exists. Please specify a different "name".'
     );
   }
@@ -69,64 +75,71 @@ class SegmentsTest extends MailPoetTest {
   function testItCanRestoreASegment() {
     $this->segment_1->trash();
 
-    $trashed_segment = Segment::findOne($this->segment_1->id());
+    $trashed_segment = Segment::findOne($this->segment_1->id);
     expect($trashed_segment->deleted_at)->notNull();
 
     $router = new Segments();
-    $response = $router->restore($this->segment_1->id());
+    $response = $router->restore(array('id' => $this->segment_1->id));
-    expect($response)->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $restored_segment = Segment::findOne($this->segment_1->id());
+      Segment::findOne($this->segment_1->id)->asArray()
-    expect($restored_segment->deleted_at)->null();
+    );
+    expect($response->data['deleted_at'])->null();
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanTrashASegment() {
     $router = new Segments();
-    $response = $router->trash($this->segment_2->id());
+    $response = $router->trash(array('id' => $this->segment_2->id));
-    expect($response)->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $trashed_segment = Segment::findOne($this->segment_2->id());
+      Segment::findOne($this->segment_2->id)->asArray()
-    expect($trashed_segment->deleted_at)->notNull();
+    );
+    expect($response->data['deleted_at'])->notNull();
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanDeleteASegment() {
     $router = new Segments();
-    $response = $router->delete($this->segment_3->id());
+    $response = $router->delete(array('id' => $this->segment_3->id));
-    expect($response)->equals(1);
+    expect($response->data)->isEmpty();
-
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-    $deleted_segment = Segment::findOne($this->segment_3->id());
+    expect($response->meta['count'])->equals(1);
-    expect($deleted_segment)->false();
   }
 
   function testItCanDuplicateASegment() {
     $router = new Segments();
-    $response = $router->duplicate($this->segment_1->id());
+    $response = $router->duplicate(array('id' => $this->segment_1->id));
-    expect($response['name'])->equals('Copy of '.$this->segment_1->name);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $duplicated_segment = Segment::findOne($response['id']);
+      Segment::where('name', 'Copy of Segment 1')->findOne()->asArray()
-    expect($duplicated_segment->name)->equals('Copy of '.$this->segment_1->name);
+    );
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanBulkDeleteSegments() {
-    expect(Segment::count())->equals(3);
+    $router = new Segments();
-
+    $response = $router->bulkAction(array(
-    $segments = Segment::findMany();
+      'action' => 'trash',
-    foreach($segments as $segment) {
+      'listing' => array('group' => 'all')
-      $segment->trash();
+    ));
-    }
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(3);
 
     $router = new Segments();
     $response = $router->bulkAction(array(
       'action' => 'delete',
       'listing' => array('group' => 'trash')
     ));
-    expect($response)->equals(3);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(3);
 
     $response = $router->bulkAction(array(
       'action' => 'delete',
       'listing' => array('group' => 'trash')
     ));
-    expect($response)->equals(0);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(0);
   }
 
   function _after() {

tests/unit/API/SubscribersTest.php

@@ -1,6 +1,7 @@
 <?php
 
 use \MailPoet\API\Endpoints\Subscribers;
+use \MailPoet\API\Response as APIResponse;
 use \MailPoet\Models\Subscriber;
 use \MailPoet\Models\Segment;
 
@@ -21,8 +22,8 @@ class SubscribersTest extends MailPoetTest {
       'last_name' => 'Doe',
       'status' => Subscriber::STATUS_SUBSCRIBED,
       'segments' => array(
-        $this->segment_1->id(),
+        $this->segment_1->id,
-        $this->segment_2->id()
+        $this->segment_2->id
       )
     ));
   }
@@ -30,17 +31,26 @@ class SubscribersTest extends MailPoetTest {
   function testItCanGetASubscriber() {
     $router = new Subscribers();
 
-    $response = $router->get($this->subscriber_1->id());
+    $response = $router->get(array('id' => 'not_an_id'));
-    expect($response['id'])->equals($this->subscriber_1->id());
+    expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
-    expect($response['email'])->equals($this->subscriber_1->email);
+    expect($response->errors[0]['message'])->equals(
-    expect($response['first_name'])->equals($this->subscriber_1->first_name);
+      'This subscriber does not exist.'
-    expect($response['last_name'])->equals($this->subscriber_1->last_name);
+    );
-
-    $response = $router->get('not_an_id');
-    expect($response)->false();
 
     $response = $router->get(/* missing argument */);
-    expect($response)->false();
+    expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
+    expect($response->errors[0]['message'])->equals(
+      'This subscriber does not exist.'
+    );
+
+    $response = $router->get(array('id' => $this->subscriber_1->id));
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->data)->equals(
+      Subscriber::findOne($this->subscriber_1->id)
+        ->withCustomFields()
+        ->withSubscriptions()
+        ->asArray()
+    );
   }
 
   function testItCanSaveANewSubscriber() {
@@ -49,15 +59,19 @@ class SubscribersTest extends MailPoetTest {
       'first_name' => 'Raul',
       'last_name' => 'Doe',
       'segments' => array(
-        $this->segment_1->id(),
+        $this->segment_1->id,
-        $this->segment_2->id()
+        $this->segment_2->id
       )
     );
 
     $router = new Subscribers();
     $response = $router->save($valid_data);
-    expect($response['result'])->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-    expect($response)->hasntKey('errors');
+    expect($response->data)->equals(
+      Subscriber::where('email', 'raul.doe@mailpoet.com')
+        ->findOne()
+        ->asArray()
+    );
 
     $subscriber = Subscriber::where('email', 'raul.doe@mailpoet.com')->findOne();
     $subscriber_segments = $subscriber->segments()->findMany();
@@ -66,60 +80,68 @@ class SubscribersTest extends MailPoetTest {
     expect($subscriber_segments[1]->name)->equals($this->segment_2->name);
 
     $response = $router->save(/* missing data */);
-    expect($response['result'])->false();
+    expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST);
-    expect($response['errors'][0])->equals('Please enter your email address');
+    expect($response->errors[0]['message'])
+      ->equals('Please enter your email address');
 
     $invalid_data = array(
       'email' => 'john.doe@invalid'
     );
 
     $response = $router->save($invalid_data);
-    expect($response['result'])->false();
+    expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST);
-    expect($response['errors'][0])->equals('Your email address is invalid!');
+    expect($response->errors[0]['message'])
+      ->equals('Your email address is invalid!');
   }
 
   function testItCanSaveAnExistingSubscriber() {
     $router = new Subscribers();
     $subscriber_data = $this->subscriber_2->asArray();
     unset($subscriber_data['created_at']);
-    $subscriber_data['segments'] = array($this->segment_1->id());
+    $subscriber_data['segments'] = array($this->segment_1->id);
     $subscriber_data['first_name'] = 'Super Jane';
 
     $response = $router->save($subscriber_data);
-    expect($response['result'])->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $updated_subscriber = Subscriber::findOne($this->subscriber_2->id());
+      Subscriber::findOne($this->subscriber_2->id)->asArray()
-    expect($updated_subscriber->email)->equals($this->subscriber_2->email);
+    );
-    expect($updated_subscriber->first_name)->equals('Super Jane');
+    expect($response->data['first_name'])->equals('Super Jane');
   }
 
   function testItCanRestoreASubscriber() {
     $this->subscriber_1->trash();
 
-    expect($this->subscriber_1->deleted_at)->notNull();
+    $trashed_subscriber = Subscriber::findOne($this->subscriber_1->id);
+    expect($trashed_subscriber->deleted_at)->notNull();
 
     $router = new Subscribers();
-    $router->restore($this->subscriber_1->id());
+    $response = $router->restore(array('id' => $this->subscriber_1->id));
-
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-    $restored_subscriber = Subscriber::findOne($this->subscriber_1->id());
+    expect($response->data)->equals(
-    expect($restored_subscriber->deleted_at)->null();
+      Subscriber::findOne($this->subscriber_1->id)->asArray()
+    );
+    expect($response->data['deleted_at'])->null();
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanTrashASubscriber() {
     $router = new Subscribers();
-    $response = $router->trash($this->subscriber_2->id());
+    $response = $router->trash(array('id' => $this->subscriber_2->id));
-    expect($response)->true();
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-
+    expect($response->data)->equals(
-    $trashed_subscriber = Subscriber::findOne($this->subscriber_2->id());
+      Subscriber::findOne($this->subscriber_2->id)->asArray()
-    expect($trashed_subscriber->deleted_at)->notNull();
+    );
+    expect($response->data['deleted_at'])->notNull();
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanDeleteASubscriber() {
     $router = new Subscribers();
-    $response = $router->delete($this->subscriber_1->id());
+    $response = $router->delete(array('id' => $this->subscriber_1->id));
-    expect($response)->equals(1);
+    expect($response->data)->isEmpty();
-
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-    expect(Subscriber::findOne($this->subscriber_1->id()))->false();
+    expect($response->meta['count'])->equals(1);
   }
 
   function testItCanFilterListing() {
@@ -282,8 +304,9 @@ class SubscribersTest extends MailPoetTest {
       ),
       'action' => 'delete'
     ));
-
+    expect($response->status)->equals(APIResponse::STATUS_OK);
-    expect($response)->equals(count($selection_ids));
+    expect($response->data)->isEmpty();
+    expect($response->meta['count'])->equals(count($selection_ids));
 
     $is_subscriber_1_deleted = (
       Subscriber::findOne($this->subscriber_1->id) === false
@@ -297,40 +320,38 @@ class SubscribersTest extends MailPoetTest {
   }
 
   function testItCanBulkDeleteSubscribers() {
-    expect(Subscriber::count())->equals(2);
+    $router = new Subscribers();
-
+    $response = $router->bulkAction(array(
-    $subscribers = Subscriber::findMany();
+      'action' => 'trash',
-    foreach($subscribers as $subscriber) {
+      'listing' => array('group' => 'all')
-      $subscriber->trash();
+    ));
-    }
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(2);
 
     $router = new Subscribers();
     $response = $router->bulkAction(array(
       'action' => 'delete',
       'listing' => array('group' => 'trash')
     ));
-    expect($response)->equals(2);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(2);
 
     $response = $router->bulkAction(array(
       'action' => 'delete',
       'listing' => array('group' => 'trash')
     ));
-    expect($response)->equals(0);
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    expect($response->meta['count'])->equals(0);
   }
 
   function testItCannotRunAnInvalidBulkAction() {
-    try {
+    $router = new Subscribers();
-      $router = new Subscribers();
+    $response = $router->bulkAction(array(
-      $response = $router->bulkAction(array(
+      'action' => 'invalidAction',
-        'action' => 'invalidAction',
+      'listing' => array()
-        'listing' => array()
+    ));
-      ));
+    expect($response->status)->equals(APIResponse::STATUS_NOT_FOUND);
-      $this->fail('Bulk Action class did not throw an exception');
+    expect($response->errors[0]['message'])->contains('has no method');
-    } catch(Exception $e) {
-      expect($e->getMessage())->equals(
-        '\MailPoet\Models\Subscriber has not method "bulkInvalidAction"'
-      );
-    }
   }
 
   function _after() {