Adding tracking as an option #17

max-mathieu · 2016-03-16T19:33:02Z

No description provided.

thehungrycoder · 2016-03-16T19:35:11Z

trunk/admin.widget.class.php

@@ -32,7 +32,7 @@ public function add_plugin_page()

    protected function render_message($msg, $msg_type = 'error')
    {
-        echo "<div class='$msg_type notice is-dismissible'><p>$msg</p></div>";
+        echo '<div class="' . $msg_type . ' notice is-dismissible"><p>$msg</p></div>';


i think using sprintf or printf is even better than this.

Good point, and I did not realize that there was $msg in it too.

rajumsys · 2016-03-16T21:51:53Z

trunk/mailer.smtp.class.php

@@ -21,6 +21,13 @@ public function configure_phpmailer($phpmailer) {
        if(!$options['enable_sparkpost'] || empty($options['password'])) {
            return;
        }
+        $tracking_enabled = !!$options['enable_tracking'];
+        $x_msys_api => array(


$x_msys_api = array(

Seems you didn't run it ;)

I did not have time yet

rajumsys · 2016-03-16T22:01:51Z

Here are a few more observations:

Tracking option should not be a checkbox. It should be dropdown to allow 3 options

<option>Default</option>
<option>Yes</option>
<option>No</option>

Now, we always setting the tracking to either true or false. But we should have an option to use system default. So, we default is selected, we won't pass the options at all, leaving it to default behaviour.

So it'll make more sense to put this field in the override section.

max-mathieu · 2016-03-17T02:03:09Z

trunk/admin.widget.class.php

@@ -32,7 +32,7 @@ public function add_plugin_page()

    protected function render_message($msg, $msg_type = 'error')
    {
-        printf('<div class="%s notice is-dismissible"><p>$msg</p></div>', $msg_type);
+        printf('<div class="%s notice is-dismissible"><p>%s</p></div>', $msg_type, $msg);


weird, I had fixed that locally. Probably forgot to push before leaving

max-mathieu · 2016-03-17T02:15:03Z

Tracking option should not be a checkbox. It should be dropdown to allow 3 options

So it'll make more sense to put this field in the override section.

I disagree. The default for tracking can be set at various levels (system, per account, per template) and they can be different between SMTP and API at those levels. There's no simple way to expose it here, and at the end of the day, users want either to track or not, so showing a "Default" without saying what it is does not help much

This reverts commit 72801e0.

…kpost into tracking-option

This reverts commit 6acffe4.

rob · 2016-03-17T18:51:00Z

Hopefully this gets merged soon! We're switching from Mandrill and just ran into an issue related to this.

We're trying to use the HTTP API version (instead of SMTP), but the open_tracking and click_tracking options are being set to true automatically (by SparkPost) even though we've disabled both of them under 'Engagement Tracking' in SparkPost's web interface. It seems that only applies to SMTP sending, not for the HTTP API (which sets them to true by default.)

Thanks for your work on this @max-mathieu! I appreciate it. I was going to work on the same thing until I saw this PR.

max-mathieu · 2016-03-18T00:59:49Z

trunk/wordpress-sparkpost.php

@@ -28,9 +28,9 @@
 }
 $sp = new SparkPost();

-if ($sp->get_option('enable_sparkpost')) {
+if ($sp::get_option('enable_sparkpost')) {


Why don't we do SparkPost:: instead of $sp:: ?

I am even wonder how it works! Seems I can call static method through instance which is weird.

Ok, I see why it works

A property declared as static cannot be accessed with an instantiated class object (though a static method can).

So back to your question, why that's preferred while language supports this.

rajumsys · 2016-03-18T13:52:01Z

trunk/mailer.http.class.php

     * @return array
     */
-    protected function get_headers_in_content()
+    protected function get_reply_to($body_content)


seems $body_content not used. So we can remove it.

oh right, I planned to pass $body['content'] as reference there and changed my mind :)

max-mathieu · 2016-03-18T16:27:09Z

trunk/sparkpost.class.php

@@ -48,7 +48,7 @@ public function sp_deactivate()

    static function get_options()
    {
-        return array_merge(self::$options_default, get_option('sp_settings'));
+        return array_merge(self::$options_default, get_option('sp_settings', []));


should be array() for php < 5.4

Adding tracking as an option

Remove 2016 theme

Css init fix

Adding tracking as an option

b0179d6

thehungrycoder reviewed Mar 16, 2016
View reviewed changes

Fixing render_message

f5ae7fa

rajumsys reviewed Mar 16, 2016
View reviewed changes

Fix headers, fix syntax issues, update default values, copy etc.

c03072b

max-mathieu reviewed Mar 17, 2016
View reviewed changes

max-mathieu added 8 commits March 17, 2016 09:01

Revert "Release v2.0.1"

6acffe4

This reverts commit 72801e0.

Merge branch 'tracking-option' of github.com:SparkPost/wordpress-spar…

13e1ab2

…kpost into tracking-option

Some minor changes

4e9272e

Add support for Reply-To

af1f5b0

Always default options

971f139

Revert "Revert "Release v2.0.1""

9622d61

This reverts commit 6acffe4.

Fixed get_options

4a41ad4

Fixed stuff

419f414

rajumsys added 2 commits March 17, 2016 18:45

Fix method/var visibility and invocation

998d731

Fix static method invocation

6d506e9

max-mathieu reviewed Mar 18, 2016
View reviewed changes

Fix Reply to

2dfa7b9

rajumsys force-pushed the tracking-option branch from 72d04d7 to 2dfa7b9 Compare March 18, 2016 01:06

Adding support for ReplyTo for API

a94a1a6

rajumsys reviewed Mar 18, 2016
View reviewed changes

rajumsys mentioned this pull request Mar 18, 2016

Support CC, BCC #23

Closed

rajumsys added 3 commits March 18, 2016 11:23

Fix error text

57d09f0

Remove redundant arg, Disable Cc in header (temporary)

4376d0c

Fix plugin activation warning

e82de48

max-mathieu reviewed Mar 18, 2016
View reviewed changes

fix default value for options

6a927cc

rajumsys added a commit that referenced this pull request Mar 18, 2016

Merge pull request #17 from SparkPost/tracking-option

38c9bf0

Adding tracking as an option

rajumsys merged commit 38c9bf0 into master Mar 18, 2016

maxharrisnet pushed a commit that referenced this pull request Sep 17, 2020

Merge pull request #17 from Automattic/rm/2016

e395964

Remove 2016 theme

maxharrisnet pushed a commit that referenced this pull request Sep 17, 2020

Merge pull request #17 from wpcomvip/css-init-fix

336d27c

Css init fix

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding tracking as an option #17

Adding tracking as an option #17

max-mathieu commented Mar 16, 2016

thehungrycoder Mar 16, 2016

max-mathieu Mar 16, 2016

rajumsys Mar 16, 2016

max-mathieu Mar 17, 2016

rajumsys commented Mar 16, 2016

max-mathieu Mar 17, 2016

max-mathieu commented Mar 17, 2016

rob commented Mar 17, 2016

max-mathieu Mar 18, 2016

rajumsys Mar 18, 2016

rajumsys Mar 18, 2016

max-mathieu Mar 18, 2016

max-mathieu Mar 18, 2016

Adding tracking as an option #17

Adding tracking as an option #17

Conversation

max-mathieu commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajumsys commented Mar 16, 2016

Choose a reason for hiding this comment

max-mathieu commented Mar 17, 2016

rob commented Mar 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment