From 95f8f130eaeed33852089b0aedd1ae2548204514 Mon Sep 17 00:00:00 2001 From: Alexey Stoletniy Date: Thu, 29 Dec 2016 15:23:24 +0300 Subject: [PATCH 1/2] Update subscriber data on repeated subscriptions [MAILPOET-760] --- lib/Config/Migrator.php | 1 + lib/Models/Subscriber.php | 60 ++++++++++----- lib/Subscription/Pages.php | 20 +++-- tests/unit/Models/SubscriberTest.php | 107 +++++++++++++++++++++++++++ 4 files changed, 166 insertions(+), 22 deletions(-) diff --git a/lib/Config/Migrator.php b/lib/Config/Migrator.php index d80d8ea493..47309e12c2 100644 --- a/lib/Config/Migrator.php +++ b/lib/Config/Migrator.php @@ -140,6 +140,7 @@ class Migrator { 'created_at TIMESTAMP NULL,', 'updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,', 'deleted_at TIMESTAMP NULL,', + 'unconfirmed_data longtext,', 'PRIMARY KEY (id),', 'UNIQUE KEY email (email)' ); diff --git a/lib/Models/Subscriber.php b/lib/Models/Subscriber.php index b10289df99..4a5973f5f8 100644 --- a/lib/Models/Subscriber.php +++ b/lib/Models/Subscriber.php @@ -170,21 +170,7 @@ class Subscriber extends Model { static function subscribe($subscriber_data = array(), $segment_ids = array()) { // filter out keys from the subscriber_data array // that should not be editable when subscribing - $reserved_columns = array( - 'id', - 'wp_user_id', - 'status', - 'subscribed_ip', - 'confirmed_ip', - 'confirmed_at', - 'created_at', - 'updated_at', - 'deleted_at' - ); - $subscriber_data = array_diff_key( - $subscriber_data, - array_flip($reserved_columns) - ); + $subscriber_data = self::filterOutReservedColumns($subscriber_data); $signup_confirmation_enabled = (bool)Setting::getValue( 'signup_confirmation.enabled' @@ -209,14 +195,17 @@ class Subscriber extends Model { $subscriber = self::findOne($subscriber_data['email']); - if($subscriber === false) { - // create new subscriber + if($subscriber === false || !$signup_confirmation_enabled) { + // create new subscriber or update if no confirmation is required $subscriber = self::createOrUpdate($subscriber_data); if($subscriber->getErrors() !== false) { return $subscriber; } $subscriber = self::findOne($subscriber->id); + } else { + // store subscriber data to be updated after confirmation + $subscriber->setUnconfirmedData($subscriber_data); } // restore trashed subscriber @@ -250,6 +239,26 @@ class Subscriber extends Model { return $subscriber; } + static function filterOutReservedColumns(array $subscriber_data) { + $reserved_columns = array( + 'id', + 'wp_user_id', + 'status', + 'subscribed_ip', + 'confirmed_ip', + 'confirmed_at', + 'created_at', + 'updated_at', + 'deleted_at', + 'unconfirmed_data' + ); + $subscriber_data = array_diff_key( + $subscriber_data, + array_flip($reserved_columns) + ); + return $subscriber_data; + } + static function search($orm, $search = '') { if(strlen(trim($search) === 0)) { return $orm; @@ -485,6 +494,9 @@ class Subscriber extends Model { } } + // wipe any unconfirmed data at this point + $data['unconfirmed_data'] = null; + $old_status = false; $new_status = false; @@ -587,6 +599,20 @@ class Subscriber extends Model { )); } + function setUnconfirmedData(array $subscriber_data) { + $subscriber_data = self::filterOutReservedColumns($subscriber_data); + $this->unconfirmed_data = json_encode($subscriber_data); + } + + function getUnconfirmedData() { + if(!empty($this->unconfirmed_data)) { + $subscriber_data = json_decode($this->unconfirmed_data, true); + $subscriber_data = self::filterOutReservedColumns((array)$subscriber_data); + return $subscriber_data; + } + return null; + } + static function bulkAddToList($orm, $data = array()) { $segment_id = (isset($data['segment_id']) ? (int)$data['segment_id'] : 0); $segment = Segment::findOne($segment_id); diff --git a/lib/Subscription/Pages.php b/lib/Subscription/Pages.php index 72676d7cde..1026c7da47 100644 --- a/lib/Subscription/Pages.php +++ b/lib/Subscription/Pages.php @@ -57,11 +57,21 @@ class Pages { } function confirm() { - if($this->subscriber !== false) { - $this->subscriber->status = Subscriber::STATUS_SUBSCRIBED; - $this->subscriber->confirmed_ip = $_SERVER['REMOTE_ADDR']; - $this->subscriber->setExpr('confirmed_at', 'NOW()'); - $this->subscriber->save(); + if($this->subscriber === false) { + return false; + } + + $subscriber_data = $this->subscriber->getUnconfirmedData(); + + $this->subscriber->status = Subscriber::STATUS_SUBSCRIBED; + $this->subscriber->confirmed_ip = $_SERVER['REMOTE_ADDR']; + $this->subscriber->setExpr('confirmed_at', 'NOW()'); + $this->subscriber->unconfirmed_data = null; + $this->subscriber->save(); + + // update subscriber from stored data after confirmation + if(!empty($subscriber_data)) { + Subscriber::createOrUpdate($subscriber_data); } } diff --git a/tests/unit/Models/SubscriberTest.php b/tests/unit/Models/SubscriberTest.php index d3205e2ed9..1f71bd1e58 100644 --- a/tests/unit/Models/SubscriberTest.php +++ b/tests/unit/Models/SubscriberTest.php @@ -4,6 +4,7 @@ use Carbon\Carbon; use Codeception\Util\Fixtures; use MailPoet\Models\CustomField; use MailPoet\Models\Segment; +use MailPoet\Models\Setting; use MailPoet\Models\Subscriber; use MailPoet\Models\SubscriberCustomField; use MailPoet\Models\SubscriberSegment; @@ -364,6 +365,112 @@ class SubscriberTest extends MailPoetTest { expect($subscriber->deleted_at)->equals(null); } + function testItOverwritesSubscriberDataWhenConfirmationIsDisabled() { + $original_setting_value = Setting::getValue('signup_confirmation.enabled'); + Setting::setValue('signup_confirmation.enabled', false); + + $segment = Segment::create(); + $segment->hydrate(array('name' => 'List #1')); + $segment->save(); + + $segment2 = Segment::create(); + $segment2->hydrate(array('name' => 'List #2')); + $segment2->save(); + + $data = array( + 'email' => 'some@example.com', + 'first_name' => 'Some', + 'last_name' => 'Example', + ); + + $subscriber = Subscriber::subscribe( + $data, + array($segment->id()) + ); + + expect($subscriber->id() > 0)->equals(true); + expect($subscriber->segments()->count())->equals(1); + expect($subscriber->email)->equals($data['email']); + expect($subscriber->first_name)->equals($data['first_name']); + expect($subscriber->last_name)->equals($data['last_name']); + + $data2 = $data; + $data2['first_name'] = 'Aaa'; + $data2['last_name'] = 'Bbb'; + + $subscriber = Subscriber::subscribe( + $data2, + array($segment->id(), $segment2->id()) + ); + + expect($subscriber->id() > 0)->equals(true); + expect($subscriber->segments()->count())->equals(2); + expect($subscriber->email)->equals($data2['email']); + expect($subscriber->first_name)->equals($data2['first_name']); + expect($subscriber->last_name)->equals($data2['last_name']); + + Setting::setValue('signup_confirmation.enabled', $original_setting_value); + } + + function testItStoresUnconfirmedSubscriberDataWhenConfirmationIsEnabled() { + $original_setting_value = Setting::getValue('signup_confirmation.enabled'); + Setting::setValue('signup_confirmation.enabled', true); + + $segment = Segment::create(); + $segment->hydrate(array('name' => 'List #1')); + $segment->save(); + + $segment2 = Segment::create(); + $segment2->hydrate(array('name' => 'List #2')); + $segment2->save(); + + $data = array( + 'email' => 'some@example.com', + 'first_name' => 'Some', + 'last_name' => 'Example', + ); + + $subscriber = Subscriber::subscribe( + $data, + array($segment->id()) + ); + + expect($subscriber->id() > 0)->equals(true); + expect($subscriber->segments()->count())->equals(1); + expect($subscriber->email)->equals($data['email']); + expect($subscriber->first_name)->equals($data['first_name']); + expect($subscriber->last_name)->equals($data['last_name']); + + expect($subscriber->unconfirmed_data)->isEmpty(); + + $data2 = $data; + $data2['first_name'] = 'Aaa'; + $data2['last_name'] = 'Bbb'; + + $subscriber = Subscriber::subscribe( + $data2, + array($segment->id(), $segment2->id()) + ); + + expect($subscriber->id() > 0)->equals(true); + expect($subscriber->segments()->count())->equals(2); + // fields should be left intact + expect($subscriber->email)->equals($data['email']); + expect($subscriber->first_name)->equals($data['first_name']); + expect($subscriber->last_name)->equals($data['last_name']); + + expect($subscriber->unconfirmed_data)->notEmpty(); + expect($subscriber->unconfirmed_data)->equals(json_encode($data2)); + + // Unconfirmed data should be wiped after any direct update + // during confirmation, manual admin editing, import etc. + $subscriber = Subscriber::createOrUpdate($data2); + + expect($subscriber->unconfirmed_data)->isEmpty(); + + Setting::setValue('signup_confirmation.enabled', $original_setting_value); + } + function testItCanBeUpdatedByEmail() { $subscriber_updated = Subscriber::createOrUpdate(array( 'email' => $this->data['email'], From d5a1d94bca7f2bfbef6d546fa70832cbaad3706e Mon Sep 17 00:00:00 2001 From: Alexey Stoletniy Date: Fri, 30 Dec 2016 10:53:29 +0300 Subject: [PATCH 2/2] Wipe unconfirmed subscriber data during import [MAILPOET-760] --- lib/Models/Subscriber.php | 1 + tests/unit/Models/SubscriberTest.php | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/Models/Subscriber.php b/lib/Models/Subscriber.php index 4a5973f5f8..3031d2672e 100644 --- a/lib/Models/Subscriber.php +++ b/lib/Models/Subscriber.php @@ -838,6 +838,7 @@ class Subscriber extends Model { 'UPDATE `' . self::$_table . '` ' . 'SET ' . implode(', ', $sql('statement')) . ' '. (($updated_at) ? ', updated_at = "' . $updated_at . '" ' : '') . + ', unconfirmed_data = NULL ' . 'WHERE email IN ' . '(' . rtrim(str_repeat('?,', count($subscribers)), ',') . ')', array_merge( diff --git a/tests/unit/Models/SubscriberTest.php b/tests/unit/Models/SubscriberTest.php index 1f71bd1e58..c1dcff2422 100644 --- a/tests/unit/Models/SubscriberTest.php +++ b/tests/unit/Models/SubscriberTest.php @@ -463,9 +463,19 @@ class SubscriberTest extends MailPoetTest { expect($subscriber->unconfirmed_data)->equals(json_encode($data2)); // Unconfirmed data should be wiped after any direct update - // during confirmation, manual admin editing, import etc. + // during confirmation, manual admin editing $subscriber = Subscriber::createOrUpdate($data2); - + expect($subscriber->unconfirmed_data)->isEmpty(); + // during import + $subscriber->unconfirmed_data = json_encode($data2); + $subscriber->save(); + expect($subscriber->isDirty('unconfirmed_data'))->false(); + expect($subscriber->unconfirmed_data)->notEmpty(); + Subscriber::updateMultiple( + array_keys($data2), + array(array_values($data2)) + ); + $subscriber = Subscriber::where('email', $data2['email'])->findOne(); expect($subscriber->unconfirmed_data)->isEmpty(); Setting::setValue('signup_confirmation.enabled', $original_setting_value);