Merge pull request #761 from mailpoet/existing_subscriber_update

Update subscriber data on repeated subscriptions [MAILPOET-760]

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)'
     );

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);
@@ -812,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(

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);
     }
   }
 

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,122 @@ 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
+    $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);
+  }
+
   function testItCanBeUpdatedByEmail() {
     $subscriber_updated = Subscriber::createOrUpdate(array(
       'email' => $this->data['email'],