Refactor & Improvements

- fixed position of checkbox in comment form
- refactored Subscriber::subscribe() method
- removed Form\Subscribe class in favor of Subscription\Comment (I'll create a similar class for Registration)
- added labels in Settings > Basics for Subscribe in comments & registration
- added method in Setting model to check whether signup confirmation is enabled

lib/Config/Hooks.php

@@ -16,18 +16,20 @@ class Hooks {
         && $subscribe_settings['on_comment']['enabled']
       ) {
         add_action(
-          'comment_form',
+          'comment_form_after_fields',
-          '\MailPoet\Form\Subscribe::inComments'
+          '\MailPoet\Subscription\Comment::extendForm'
         );
+
         add_action(
           'comment_post',
-          '\MailPoet\Form\Subscribe::onCommentSubmit',
+          '\MailPoet\Subscription\Comment::onSubmit',
           60,
           2
         );
+
         add_action(
           'wp_set_comment_status',
-          '\MailPoet\Form\Subscribe::onCommentStatusUpdate',
+          '\MailPoet\Subscription\Comment::onStatusUpdate',
           60,
           2
         );

lib/Form/Subscribe.php

@@ -1,92 +0,0 @@
-<?php
-namespace MailPoet\Form;
-use \MailPoet\Models\Setting;
-use \MailPoet\Models\Subscriber;
-
-class Subscribe {
-  static function inComments() {
-    $label = (isset($subscribe_settings['on_comment']['label'])
-      ? $subscribe_settings['on_comment']['label']
-      : __('Yes, add me to your mailing list.')
-    );
-
-    $html = <<<EOL
-      <p class="comment-form-mailpoet">
-        <label for="mailpoet_subscribe_on_comment">
-          <input
-            type="checkbox"
-            id="mailpoet_subscribe_on_comment"
-            value="1"
-            name="mailpoet[subscribe_on_comment]"
-          />&nbsp;$label
-        </label>
-      </p>
-EOL;
-    echo $html;
-  }
-
-  static function onCommentSubmit($comment_id, $comment_approved) {
-    if($comment_approved === 'spam') return;
-
-    if(
-      isset($_POST['mailpoet']['subscribe_on_comment'])
-      &&
-      filter_var(
-        $_POST['mailpoet']['subscribe_on_comment'],
-        FILTER_VALIDATE_BOOLEAN
-      ) === true
-    ) {
-      if($comment_approved === 0) {
-        add_comment_meta(
-          $comment_id,
-          'mailpoet',
-          'subscribe_on_comment',
-          true
-        );
-      } else {
-        $subscribe_settings = Setting::getValue('subscribe');
-        $segment_ids = (array)$subscribe_settings['on_comment']['segments'];
-
-        if($subscribe_settings !== null) {
-          $comment = get_comment($comment_id);
-
-          $result = Subscriber::subscribe(
-            array(
-              'email' => $comment->comment_author_email,
-              'first_name' => $comment->comment_author
-            ),
-            $segment_ids
-          );
-        }
-      }
-    }
-  }
-
-  static function onCommentStatusUpdate($comment_id, $comment_status) {
-    $comment_meta = get_comment_meta(
-      $comment_id,
-      'mailpoet',
-      true
-    );
-
-    if(
-      $comment_status ==='approve'
-      && $comment_meta === 'subscribe_on_comment'
-    ) {
-      $subscribe_settings = Setting::getValue('subscribe');
-      $segment_ids = (array)$subscribe_settings['on_comment']['segments'];
-
-      if($subscribe_settings !== null) {
-        $comment = get_comment($comment_id);
-
-        Subscriber::subscribe(
-          array(
-            'email' => $comment->comment_author_email,
-            'first_name' => $comment->comment_author
-          ),
-          $segment_ids
-        );
-      }
-    }
-  }
-}

lib/Models/Setting.php

@@ -67,4 +67,16 @@ class Setting extends Model {
     $exists->value = $model['value'];
     return $exists->save();
   }
+
+  public static function hasSignupConfirmation() {
+    $signup_confirmation = Setting::getValue('signup_confirmation', array());
+    $has_signup_confirmation = true;
+    if(array_key_exists('enabled', $signup_confirmation)) {
+      $has_signup_confirmation = filter_var(
+        $signup_confirmation['enabled'],
+        FILTER_VALIDATE_BOOLEAN
+      );
+    }
+    return $has_signup_confirmation;
+  }
 }

lib/Models/Subscriber.php

@@ -42,6 +42,12 @@ class Subscriber extends Model {
     }
   }
 
+  function sendConfirmationEmail() {
+    $subscriber->set('status', 'unconfirmed');
+
+    // TODO
+  }
+
   static function subscribe($subscriber_data = array(), $segment_ids = array()) {
     if(empty($subscriber_data) or empty($segment_ids)) {
       return false;
@@ -50,34 +56,19 @@ class Subscriber extends Model {
     $subscriber = static::createOrUpdate($subscriber_data);
 
     if($subscriber !== false && $subscriber->id() > 0) {
-      $signup_confirmation = Setting::getValue('signup_confirmation', array());
-      $has_signup_confirmation = true;
-      if(array_key_exists('enabled', $signup_confirmation)) {
-        $has_signup_confirmation = filter_var(
-          $signup_confirmation['enabled'],
-          FILTER_VALIDATE_BOOLEAN
-        );
-      }
-
       // restore deleted subscriber
       if($subscriber->deleted_at !== NULL) {
         $subscriber->setExpr('deleted_at', 'NULL');
       }
 
-      if($has_signup_confirmation === false) {
+      if(Setting::hasSignupConfirmation()) {
-        // auto subscribe when signup confirmation is turned off
-        $subscriber->set('status', 'subscribed');
-      } else {
         // reset status of existing subscribers if signup confirmation
         // is turned on
         if($subscriber->status !== 'subscribed') {
-          $subscriber->set('status', 'unconfirmed');
+          $subscriber->sendConfirmationEmail();
-        }
-
-        // send confirmation email to unconfirmed subscribers
-        if($subscriber->status === 'unconfirmed') {
-          // TODO: send signup confirmation email
         }
+      } else {
+        $subscriber->set('status', 'subscribed');
       }
 
       if($subscriber->save()) {
@@ -255,34 +246,6 @@ class Subscriber extends Model {
       $subscriber->set($data);
     }
 
-    // TODO: Cf
-    /*
-    // custom fields
-    $custom_fields = array();
-    foreach($data as $key => $value) {
-      if(strpos($key, 'cf_') === 0) {
-        $custom_fields[substr($key, 3)] = $value;
-        unset($data[$key]);
-      }
-    }
-
-    // add custom fields
-    if(!empty($custom_fields)) {
-      foreach($custom_fields as $custom_field_id => $value) {
-        if(is_array($value)) {
-          // date
-          $value = mktime(0, 0, 0, $value['month'], $value['day'], $value['year']);
-        }
-        $subscriber_custom_field = SubscriberCustomField::create();
-        $subscriber_custom_field->hydrate(array(
-          'subscriber_id' => $subscriber->id(),
-          'custom_field_id' => $custom_field_id,
-          'value' => $value
-        ));
-        $subscriber_custom_field->save();
-      }
-    }*/
-
     $subscriber->save();
     return $subscriber;
   }
@@ -364,8 +327,7 @@ class Subscriber extends Model {
 
     if(!empty($subscribers)) {
       foreach($subscribers as $subscriber) {
-        // TODO: send confirmation email
+        $subscriber->sendConfirmationEmail();
-        // $subscriber->sendConfirmationEmail()
       }
 
       return $subscribers->count();

lib/Subscription/Comment.php

@@ -0,0 +1,91 @@
+<?php
+namespace MailPoet\Subscription;
+use \MailPoet\Models\Setting;
+use \MailPoet\Models\Subscriber;
+
+class Comment {
+  const SPAM = 'spam';
+  const APPROVED = 1;
+  const PENDING_APPROVAL = 0;
+
+  static function extendForm() {
+    $subscribe_settings = Setting::getValue('subscribe');
+    $label = (isset($subscribe_settings['on_comment']['label'])
+      ? $subscribe_settings['on_comment']['label']
+      : __('Yes, add me to your mailing list.')
+    );
+
+    print '<p class="comment-form-mailpoet">
+      <label for="mailpoet_subscribe_on_comment">
+        <input
+          type="checkbox"
+          id="mailpoet_subscribe_on_comment"
+          value="1"
+          name="mailpoet[subscribe_on_comment]"
+        />&nbsp;'.esc_attr($label).'
+      </label>
+    </p>';
+  }
+
+  static function onSubmit($comment_id, $comment_status) {
+    if($comment_status === Comment::SPAM) return;
+
+    if(
+      isset($_POST['mailpoet']['subscribe_on_comment'])
+      &&
+      filter_var(
+        $_POST['mailpoet']['subscribe_on_comment'],
+        FILTER_VALIDATE_BOOLEAN
+      ) === true
+    ) {
+      if($comment_status === Comment::PENDING_APPROVAL) {
+        // add a comment meta to remember to subscribe the user
+        // once the comment gets approved
+        add_comment_meta(
+          $comment_id,
+          'mailpoet',
+          'subscribe_on_comment',
+          true
+        );
+      } else if($comment_status === Comment::APPROVED) {
+        static::subscribeAuthorOfComment($comment_id);
+      }
+    }
+  }
+
+  static function onStatusUpdate($comment_id, $action) {
+    if($action === 'approve') {
+      // check if the comment's author wants to subscribe
+      $do_subscribe = (
+        get_comment_meta(
+          $comment_id,
+          'mailpoet',
+          true
+        ) === 'subscribe_on_comment'
+      );
+
+      if($do_subscribe === true) {
+        static::subscribeAuthorOfComment($comment_id);
+
+        delete_comment_meta($comment_id, 'mailpoet');
+      }
+    }
+  }
+
+  private static function subscribeAuthorOfComment($comment_id) {
+    $subscribe_settings = Setting::getValue('subscribe');
+    $segment_ids = (array)$subscribe_settings['on_comment']['segments'];
+
+    if($subscribe_settings !== null) {
+      $comment = get_comment($comment_id);
+
+      $result = Subscriber::subscribe(
+        array(
+          'email' => $comment->comment_author_email,
+          'first_name' => $comment->comment_author
+        ),
+        $segment_ids
+      );
+    }
+  }
+}

views/settings/basics.html

@@ -102,6 +102,9 @@
               <% endif %>
             />
           </p>
+          <p>
+            <label><%= __('Users will be subscribed to these lists:') %></label>
+          </p>
           <p>
             <select
               id="mailpoet_subscribe_on_comment_segments"
@@ -163,6 +166,9 @@
                 <% endif %>
               />
             </p>
+            <p>
+              <label><%= __('Users will be subscribed to these lists:') %></label>
+            </p>
             <p>
               <select
                 id="mailpoet_subscribe_on_register_segments"
@@ -227,7 +233,7 @@
           ><%= __('Edit') %></a>
         </p>
         <p>
-          <label><%= __('Subscribers can choose from these lists :') %></label>
+          <label><%= __('Subscribers can choose from these lists:') %></label>
         </p>
         <p>
           <select