From bfdc13a8d1be838dbdda80d3e80acbfe46ed76a3 Mon Sep 17 00:00:00 2001 From: Alexey Stoletniy Date: Tue, 24 Jan 2017 21:12:56 +0300 Subject: [PATCH] Fix fubscription form failing when some fields are absent or don't exist [MAILPOET-764] --- lib/API/Endpoints/Subscribers.php | 10 ++ lib/Models/Form.php | 26 ++++ tests/unit/API/Endpoints/SubscribersTest.php | 44 ++++++- tests/unit/Models/FormTest.php | 20 ++++ tests/unit/_bootstrap.php | 120 ++++++++++++++++++- 5 files changed, 218 insertions(+), 2 deletions(-) diff --git a/lib/API/Endpoints/Subscribers.php b/lib/API/Endpoints/Subscribers.php index ed0f322e43..329104aae1 100644 --- a/lib/API/Endpoints/Subscribers.php +++ b/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(); diff --git a/lib/Models/Form.php b/lib/Models/Form.php index 156857c06e..4d95c20891 100644 --- a/lib/Models/Form.php +++ b/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.'%'); } diff --git a/tests/unit/API/Endpoints/SubscribersTest.php b/tests/unit/API/Endpoints/SubscribersTest.php index 632fd0ceff..41fe164b78 100644 --- a/tests/unit/API/Endpoints/SubscribersTest.php +++ b/tests/unit/API/Endpoints/SubscribersTest.php @@ -1,7 +1,9 @@ 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'); diff --git a/tests/unit/Models/FormTest.php b/tests/unit/Models/FormTest.php index 44369dc866..9e8a716019 100644 --- a/tests/unit/Models/FormTest.php +++ b/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(); diff --git a/tests/unit/_bootstrap.php b/tests/unit/_bootstrap.php index 4fa15072d4..ce4f15b7d1 100644 --- a/tests/unit/_bootstrap.php +++ b/tests/unit/_bootstrap.php @@ -43,4 +43,122 @@ Fixtures::add( 'last_name' => 'John', 'email' => 'john.doe@example.com' ) -); \ No newline at end of file +); + +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', + ), + ) +);