Fix fubscription form failing when some fields are absent or don't exist [MAILPOET-764]

lib/API/Endpoints/Subscribers.php

@@ -65,6 +65,12 @@ class Subscribers extends APIEndpoint {
     $form = Form::findOne($form_id);
     unset($data['form_id']);
 
+    if(!$form) {
+      return $this->badRequest(array(
+        APIError::BAD_REQUEST => __('Please specify a valid form ID.', 'mailpoet')
+      ));
+    }
+
     $segment_ids = (!empty($data['segments'])
       ? (array)$data['segments']
       : array()
@@ -77,6 +83,10 @@ class Subscribers extends APIEndpoint {
       ));
     }
 
+    // only accept fields defined in the form
+    $form_fields = $form->getFieldList();
+    $data = array_intersect_key($data, array_flip($form_fields));
+
     $subscriber = Subscriber::subscribe($data, $segment_ids);
     $errors = $subscriber->getErrors();
 

lib/Models/Form.php

@@ -39,6 +39,32 @@ class Form extends Model {
     return parent::save();
   }
 
+  function getFieldList() {
+    $form = $this->asArray();
+    if(empty($form['body'])) {
+      return false;
+    }
+
+    $skipped_types = array('html', 'divider', 'submit');
+    $fields = array();
+
+    foreach((array)$form['body'] as $field) {
+      if(empty($field['id'])
+        || empty($field['type'])
+        || in_array($field['type'], $skipped_types)
+      ) {
+        continue;
+      }
+      if($field['id'] > 0) {
+        $fields[] = 'cf_' . $field['id'];
+      } else {
+        $fields[] = $field['id'];
+      }
+    }
+
+    return $fields ?: false;
+  }
+
   static function search($orm, $search = '') {
     return $orm->whereLike('name', '%'.$search.'%');
   }

tests/unit/API/Endpoints/SubscribersTest.php

@@ -1,7 +1,9 @@
 <?php
 
+use Codeception\Util\Fixtures;
 use \MailPoet\API\Endpoints\Subscribers;
 use \MailPoet\API\Response as APIResponse;
+use \MailPoet\Models\Form;
 use \MailPoet\Models\Subscriber;
 use \MailPoet\Models\Segment;
 use \MailPoet\Models\Setting;
@@ -28,6 +30,11 @@ class SubscribersTest extends MailPoetTest {
       )
     ));
 
+    $this->form = Form::createOrUpdate(array(
+      'name' => 'My Form',
+      'body' => Fixtures::get('form_body_template')
+    ));
+
     // setup mailer
     Setting::setValue('sender', array(
       'address' => 'sender@mailpoet.com',
@@ -375,10 +382,22 @@ class SubscribersTest extends MailPoetTest {
     expect($response->errors[0]['message'])->contains('has no method');
   }
 
-  function testItCannotSubscribeWithoutSegments() {
+  function testItCannotSubscribeWithoutFormID() {
     $router = new Subscribers();
     $response = $router->subscribe(array(
       'email' => 'toto@mailpoet.com'
+      // no form ID specified
+    ));
+
+    expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST);
+    expect($response->errors[0]['message'])->equals('Please specify a valid form ID.');
+  }
+
+  function testItCannotSubscribeWithoutSegments() {
+    $router = new Subscribers();
+    $response = $router->subscribe(array(
+      'email' => 'toto@mailpoet.com',
+      'form_id' => $this->form->id
       // no segments specified
     ));
 
@@ -390,23 +409,46 @@ class SubscribersTest extends MailPoetTest {
     $router = new Subscribers();
     $response = $router->subscribe(array(
       'email' => 'toto@mailpoet.com',
+      'form_id' => $this->form->id,
       'segments' => array($this->segment_1->id, $this->segment_2->id)
     ));
     expect($response->status)->equals(APIResponse::STATUS_OK);
   }
 
+  function testItCanFilterOutNonFormFieldsWhenSubscribing() {
+    $router = new Subscribers();
+    $response = $router->subscribe(array(
+      'email' => 'toto@mailpoet.com',
+      'form_id' => $this->form->id,
+      'segments' => array($this->segment_1->id, $this->segment_2->id),
+      // exists in table and in the form
+      'first_name' => 'aaa',
+      // exists in table, but not in the form
+      'last_name' => 'bbb',
+      // doesn't exist
+      'bogus' => 'hahaha'
+    ));
+    expect($response->status)->equals(APIResponse::STATUS_OK);
+    $subscriber = Subscriber::findOne($response->data['id']);
+    expect($subscriber->first_name)->equals('aaa');
+    expect($subscriber->last_name)->isEmpty();
+    expect(isset($subscriber->bogus))->false();
+  }
+
   function testItCannotMassSubscribe() {
     $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
 
     $router = new Subscribers();
     $response = $router->subscribe(array(
       'email' => 'toto@mailpoet.com',
+      'form_id' => $this->form->id,
       'segments' => array($this->segment_1->id, $this->segment_2->id)
     ));
 
     try {
       $response = $router->subscribe(array(
         'email' => 'tata@mailpoet.com',
+        'form_id' => $this->form->id,
         'segments' => array($this->segment_1->id, $this->segment_2->id)
       ));
       $this->fail('It should not be possible to subscribe a second time so soon');

tests/unit/Models/FormTest.php

@@ -92,6 +92,26 @@ class FormTest extends MailPoetTest {
     expect($form->name)->equals('Updated Form');
   }
 
+  function testItCanProvideAFieldList() {
+    $form = Form::createOrUpdate(array(
+      'name' => 'My Form',
+      'body' => array(
+        array(
+          'type' => 'text',
+          'id' => 'email',
+        ),
+        array(
+          'type' => 'text',
+          'id' => 2,
+        ),
+        array(
+          'type' => 'submit',
+          'id' => 'submit',
+        )
+      )
+    ));
+    expect($form->getFieldList())->equals(array('email', 'cf_2'));
+  }
 
   function _after() {
     Form::deleteMany();

tests/unit/_bootstrap.php

@@ -44,3 +44,121 @@ Fixtures::add(
     'email' => 'john.doe@example.com'
   )
 );
+
+Fixtures::add(
+  'form_body_template',
+  array(
+    array(
+      'type' => 'text',
+      'name' => 'First name',
+      'id' => 'first_name',
+      'unique' => '1',
+      'static' => '0',
+      'params' =>
+      array(
+        'label' => 'First name',
+      ),
+      'position' => '1',
+    ),
+    array(
+      'type' => 'text',
+      'name' => 'Nickname',
+      'id' => '4',
+      'unique' => '1',
+      'static' => '0',
+      'params' =>
+      array(
+        'label' => 'Nickname',
+      ),
+      'position' => '2',
+    ),
+    array(
+      'type' => 'text',
+      'name' => 'Age',
+      'id' => '2',
+      'unique' => '1',
+      'static' => '0',
+      'params' =>
+      array(
+        'required' => '',
+        'validate' => 'number',
+        'label' => 'Age',
+      ),
+      'position' => '3',
+    ),
+    array (
+      'type' => 'divider',
+      'name' => 'Divider',
+      'id' => 'divider',
+      'unique' => '0',
+      'static' => '0',
+      'params' => '',
+      'position' => '4',
+    ),
+    array (
+      'type' => 'radio',
+      'name' => '3-way choice',
+      'id' => '3',
+      'unique' => '1',
+      'static' => '0',
+      'params' =>
+      array (
+        'values' =>
+        array (
+          0 =>
+          array (
+            'value' => '1',
+          ),
+          1 =>
+          array (
+            'value' => '2',
+          ),
+          2 =>
+          array (
+            'value' => '3',
+          ),
+        ),
+        'required' => '',
+        'label' => '3-way choice',
+      ),
+      'position' => '5',
+    ),
+    array (
+      'type' => 'html',
+      'name' => 'Random text or HTML',
+      'id' => 'html',
+      'unique' => '0',
+      'static' => '0',
+      'params' =>
+      array (
+        'text' => 'Subscribe to our newsletter and join [mailpoet_subscribers_count] other subscribers.',
+      ),
+      'position' => '6',
+    ),
+    array(
+      'type' => 'text',
+      'name' => 'Email',
+      'id' => 'email',
+      'unique' => '0',
+      'static' => '1',
+      'params' =>
+      array(
+        'label' => 'Email',
+        'required' => 'true',
+      ),
+      'position' => '7',
+    ),
+    array(
+      'type' => 'submit',
+      'name' => 'Submit',
+      'id' => 'submit',
+      'unique' => '0',
+      'static' => '1',
+      'params' =>
+      array(
+        'label' => 'Subscribe!',
+      ),
+      'position' => '8',
+    ),
+  )
+);