From ba9cd15651f7bb6f70d5ced8286b870310caf6d7 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 7 Jun 2016 09:08:01 -0400 Subject: [PATCH 1/7] - Extracts view in browser URl logic into a separate class --- lib/Newsletter/Shortcodes/Categories/Link.php | 41 ++++------------- lib/Newsletter/Viewer/Url.php | 45 +++++++++++++++++++ 2 files changed, 54 insertions(+), 32 deletions(-) create mode 100644 lib/Newsletter/Viewer/Url.php diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index c26f987ec4..c78864ccf2 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -2,7 +2,7 @@ namespace MailPoet\Newsletter\Shortcodes\Categories; use MailPoet\Models\Setting; -use MailPoet\Models\Subscriber as SubscriberModel; +use MailPoet\Newsletter\Viewer\Url as ViewInBrowserUrl; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscription\Url as SubscriptionUrl; @@ -26,7 +26,7 @@ class Link { $url, __('Unsubscribe') ); - break; + break; case 'subscription_unsubscribe_url': return self::processUrl( @@ -34,7 +34,7 @@ class Link { SubscriptionUrl::getUnsubscribeUrl($subscriber), $queue ); - break; + break; case 'subscription_manage': $url = self::processUrl( @@ -47,7 +47,7 @@ class Link { $url, __('Manage subscription') ); - break; + break; case 'subscription_manage_url': return self::processUrl( @@ -55,21 +55,21 @@ class Link { SubscriptionUrl::getManageUrl($subscriber), $queue ); - break; + break; case 'newsletter_view_in_browser': $action = 'view_in_browser_url'; - $url = esc_attr(self::getViewInBrowserUrl($newsletter, $subscriber, $queue)); + $url = esc_attr(ViewInBrowserUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue)); $url = self::processUrl($action, $url, $queue); return sprintf( '%s', $url, __('View in your browser') ); - break; + break; case 'newsletter_view_in_browser_url': - $url = self::getViewInBrowserUrl($newsletter, $subscriber, $queue); + $url = ViewInBrowserUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue); return self::processUrl($action, $url, $queue); break; @@ -85,33 +85,10 @@ class Link { return ($url !== $shortcode) ? self::processUrl($action, $url, $queue) : false; - break; + break; } } - static function getViewInBrowserUrl( - $newsletter, - $subscriber = false, - $queue = false - ) { - $data = array( - 'newsletter' => (isset($newsletter['id'])) ? - $newsletter['id'] : - $newsletter, - 'subscriber' => (isset($subscriber['id'])) ? - $subscriber['id'] : - $subscriber, - 'subscriber_token' => (isset($subscriber['id'])) ? - SubscriberModel::generateToken($subscriber['email']) : - false, - 'queue' => (isset($queue['id'])) ? - $queue['id'] : - $queue - ); - $data = rtrim(base64_encode(serialize($data)), '='); - return home_url() . '/?mailpoet&endpoint=view_in_browser&data=' . $data; - } - static function processUrl($action, $url, $queue) { return ($queue !== false && (boolean) Setting::getValue('tracking.enabled')) ? self::getShortcode($action) : diff --git a/lib/Newsletter/Viewer/Url.php b/lib/Newsletter/Viewer/Url.php new file mode 100644 index 0000000000..efe21ee876 --- /dev/null +++ b/lib/Newsletter/Viewer/Url.php @@ -0,0 +1,45 @@ +asArray(); + } + if(is_object($subscriber)) { + $subscriber = $subscriber->asArray(); + } + if(is_object($queue)) { + $queue = $queue->asArray(); + } + $data = array( + 'newsletter' => (!empty($newsletter['id'])) ? + $newsletter['id'] : + $newsletter, + 'subscriber' => (!empty($subscriber['id'])) ? + $subscriber['id'] : + $subscriber, + 'subscriber_token' => (!empty($subscriber['id'])) ? + Subscriber::generateToken($subscriber['email']) : + false, + 'queue' => (!empty($queue['id'])) ? + $queue['id'] : + $queue + ); + $params = array( + 'endpoint=view_in_browser', + 'data=' . rtrim(base64_encode(serialize($data)), '=') + ); + return sprintf( + '%s?%s', + home_url(), + join('&', $params) + ); + } +} \ No newline at end of file From 03eb4ad0fc5a15187ba981b0ad4c12c97c403ef4 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 7 Jun 2016 09:16:48 -0400 Subject: [PATCH 2/7] - Changes location for the view in browser URL class --- lib/Newsletter/Shortcodes/Categories/Link.php | 2 +- lib/Newsletter/{Viewer => }/Url.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename lib/Newsletter/{Viewer => }/Url.php (96%) diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index c78864ccf2..88d11276b3 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -2,7 +2,7 @@ namespace MailPoet\Newsletter\Shortcodes\Categories; use MailPoet\Models\Setting; -use MailPoet\Newsletter\Viewer\Url as ViewInBrowserUrl; +use MailPoet\Newsletter\Url as ViewInBrowserUrl; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscription\Url as SubscriptionUrl; diff --git a/lib/Newsletter/Viewer/Url.php b/lib/Newsletter/Url.php similarity index 96% rename from lib/Newsletter/Viewer/Url.php rename to lib/Newsletter/Url.php index efe21ee876..02ec50555a 100644 --- a/lib/Newsletter/Viewer/Url.php +++ b/lib/Newsletter/Url.php @@ -1,5 +1,5 @@ Date: Tue, 7 Jun 2016 09:28:29 -0400 Subject: [PATCH 3/7] - Changes location for the main view in browser class - Updates code formattign for case statements --- lib/Config/PublicAPI.php | 2 +- lib/Newsletter/Shortcodes/Categories/Link.php | 22 +++++++++---------- lib/Newsletter/{Viewer => }/ViewInBrowser.php | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) rename lib/Newsletter/{Viewer => }/ViewInBrowser.php (98%) diff --git a/lib/Config/PublicAPI.php b/lib/Config/PublicAPI.php index b3b5ccf614..35f6466474 100644 --- a/lib/Config/PublicAPI.php +++ b/lib/Config/PublicAPI.php @@ -2,7 +2,7 @@ namespace MailPoet\Config; use MailPoet\Cron\Daemon; -use MailPoet\Newsletter\Viewer\ViewInBrowser; +use MailPoet\Newsletter\ViewInBrowser; use MailPoet\Statistics\Track\Clicks; use MailPoet\Statistics\Track\Opens; use MailPoet\Subscription; diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index 88d11276b3..6ce4de1bdb 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -26,7 +26,7 @@ class Link { $url, __('Unsubscribe') ); - break; + break; case 'subscription_unsubscribe_url': return self::processUrl( @@ -34,7 +34,7 @@ class Link { SubscriptionUrl::getUnsubscribeUrl($subscriber), $queue ); - break; + break; case 'subscription_manage': $url = self::processUrl( @@ -47,7 +47,7 @@ class Link { $url, __('Manage subscription') ); - break; + break; case 'subscription_manage_url': return self::processUrl( @@ -55,7 +55,7 @@ class Link { SubscriptionUrl::getManageUrl($subscriber), $queue ); - break; + break; case 'newsletter_view_in_browser': $action = 'view_in_browser_url'; @@ -66,12 +66,12 @@ class Link { $url, __('View in your browser') ); - break; + break; case 'newsletter_view_in_browser_url': $url = ViewInBrowserUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue); return self::processUrl($action, $url, $queue); - break; + break; default: $shortcode = self::getShortcode($action); @@ -85,7 +85,7 @@ class Link { return ($url !== $shortcode) ? self::processUrl($action, $url, $queue) : false; - break; + break; } } @@ -106,13 +106,13 @@ class Link { $unsubscribe->track($subscriber['id'], $queue['id'], $newsletter['id']); } $url = SubscriptionUrl::getUnsubscribeUrl($subscriber); - break; + break; case 'subscription_manage_url': $url = SubscriptionUrl::getManageUrl($subscriber); - break; + break; case 'newsletter_view_in_browser_url': $url = Link::getViewInBrowserUrl($newsletter, $subscriber, $queue); - break; + break; default: $shortcode = self::getShortcode($shortcode_action); $url = apply_filters( @@ -123,7 +123,7 @@ class Link { $queue ); $url = ($url !== $shortcode_action) ? $url : false; - break; + break; } return $url; } diff --git a/lib/Newsletter/Viewer/ViewInBrowser.php b/lib/Newsletter/ViewInBrowser.php similarity index 98% rename from lib/Newsletter/Viewer/ViewInBrowser.php rename to lib/Newsletter/ViewInBrowser.php index fb72e8422e..4ff32123ad 100644 --- a/lib/Newsletter/Viewer/ViewInBrowser.php +++ b/lib/Newsletter/ViewInBrowser.php @@ -1,5 +1,5 @@ Date: Tue, 7 Jun 2016 10:14:37 -0400 Subject: [PATCH 4/7] - Updates based on code review comments --- lib/Newsletter/Shortcodes/Categories/Link.php | 6 +++--- lib/Newsletter/Url.php | 8 +++----- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index 6ce4de1bdb..c94432b0de 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -2,7 +2,7 @@ namespace MailPoet\Newsletter\Shortcodes\Categories; use MailPoet\Models\Setting; -use MailPoet\Newsletter\Url as ViewInBrowserUrl; +use MailPoet\Newsletter\Url as NewsletterUrl; use MailPoet\Statistics\Track\Unsubscribes; use MailPoet\Subscription\Url as SubscriptionUrl; @@ -59,7 +59,7 @@ class Link { case 'newsletter_view_in_browser': $action = 'view_in_browser_url'; - $url = esc_attr(ViewInBrowserUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue)); + $url = esc_attr(NewsletterUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue)); $url = self::processUrl($action, $url, $queue); return sprintf( '%s', @@ -69,7 +69,7 @@ class Link { break; case 'newsletter_view_in_browser_url': - $url = ViewInBrowserUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue); + $url = NewsletterUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue); return self::processUrl($action, $url, $queue); break; diff --git a/lib/Newsletter/Url.php b/lib/Newsletter/Url.php index 02ec50555a..10f2acce68 100644 --- a/lib/Newsletter/Url.php +++ b/lib/Newsletter/Url.php @@ -33,13 +33,11 @@ class Url { $queue ); $params = array( + 'mailpoet', 'endpoint=view_in_browser', 'data=' . rtrim(base64_encode(serialize($data)), '=') ); - return sprintf( - '%s?%s', - home_url(), - join('&', $params) - ); + $url = home_url(); + return $url .= (parse_url($url, PHP_URL_QUERY) ? '&' : '?') . join('&', $params); } } \ No newline at end of file From a5c620acf3c72a0b625ebff18069ab80e2a9bc38 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 7 Jun 2016 10:41:19 -0400 Subject: [PATCH 5/7] - Updates the way the view in browser URL is constructed --- lib/Newsletter/Url.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/Newsletter/Url.php b/lib/Newsletter/Url.php index 10f2acce68..9a49fc6700 100644 --- a/lib/Newsletter/Url.php +++ b/lib/Newsletter/Url.php @@ -33,11 +33,10 @@ class Url { $queue ); $params = array( - 'mailpoet', - 'endpoint=view_in_browser', - 'data=' . rtrim(base64_encode(serialize($data)), '=') + 'mailpoet' => '', + 'endpoint' => 'view_in_browser', + 'data' => rtrim(base64_encode(serialize($data)), '=') ); - $url = home_url(); - return $url .= (parse_url($url, PHP_URL_QUERY) ? '&' : '?') . join('&', $params); + return add_query_arg($params, home_url()); } } \ No newline at end of file From d1826389712cd09991c478ecf5e6d2aeb697bb02 Mon Sep 17 00:00:00 2001 From: Vlad Date: Tue, 7 Jun 2016 10:53:01 -0400 Subject: [PATCH 6/7] - Updates references to the new view in browser URL class - Removes unnecessary rtrim condition in URL generation --- lib/Newsletter/Shortcodes/Categories/Link.php | 2 +- lib/Newsletter/Url.php | 2 +- lib/Router/Newsletters.php | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index c94432b0de..8d83a12249 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -111,7 +111,7 @@ class Link { $url = SubscriptionUrl::getManageUrl($subscriber); break; case 'newsletter_view_in_browser_url': - $url = Link::getViewInBrowserUrl($newsletter, $subscriber, $queue); + $url = NewsletterUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue); break; default: $shortcode = self::getShortcode($shortcode_action); diff --git a/lib/Newsletter/Url.php b/lib/Newsletter/Url.php index 9a49fc6700..5c957dd15f 100644 --- a/lib/Newsletter/Url.php +++ b/lib/Newsletter/Url.php @@ -35,7 +35,7 @@ class Url { $params = array( 'mailpoet' => '', 'endpoint' => 'view_in_browser', - 'data' => rtrim(base64_encode(serialize($data)), '=') + 'data' => base64_encode(serialize($data)) ); return add_query_arg($params, home_url()); } diff --git a/lib/Router/Newsletters.php b/lib/Router/Newsletters.php index 0db3ceede9..63bcda8f94 100644 --- a/lib/Router/Newsletters.php +++ b/lib/Router/Newsletters.php @@ -11,7 +11,7 @@ use MailPoet\Models\NewsletterOption; use MailPoet\Models\Subscriber; use MailPoet\Newsletter\Renderer\Renderer; use MailPoet\Newsletter\Scheduler\Scheduler; -use MailPoet\Newsletter\Shortcodes\Categories\Link; +use MailPoet\Newsletter\Url as NewsletterUrl; use MailPoet\Util\Helpers; if(!defined('ABSPATH')) exit; @@ -154,7 +154,7 @@ class Newsletters { $subscriber = Subscriber::where('email', $wp_user->data->user_email) ->findOne(); $subscriber = ($subscriber) ? $subscriber->asArray() : $subscriber; - $preview_url = Link::getViewInBrowserUrl($data, $subscriber); + $preview_url = NewsletterUrl::getViewInBrowserUrl($data, $subscriber); return array( 'result' => true, 'data' => array('url' => $preview_url) From d6cbe5aac8d91d86c285f694d78b49b7d2a21946 Mon Sep 17 00:00:00 2001 From: Vlad Date: Wed, 8 Jun 2016 11:08:32 -0400 Subject: [PATCH 7/7] - Fixes incorrect shortcode name - Updates unit test --- lib/Newsletter/Shortcodes/Categories/Link.php | 2 +- tests/unit/Newsletter/ShortcodesTest.php | 45 ++++++++++++------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/lib/Newsletter/Shortcodes/Categories/Link.php b/lib/Newsletter/Shortcodes/Categories/Link.php index 8d83a12249..9e97d91812 100644 --- a/lib/Newsletter/Shortcodes/Categories/Link.php +++ b/lib/Newsletter/Shortcodes/Categories/Link.php @@ -58,7 +58,7 @@ class Link { break; case 'newsletter_view_in_browser': - $action = 'view_in_browser_url'; + $action = 'newsletter_view_in_browser_url'; $url = esc_attr(NewsletterUrl::getViewInBrowserUrl($newsletter, $subscriber, $queue)); $url = self::processUrl($action, $url, $queue); return sprintf( diff --git a/tests/unit/Newsletter/ShortcodesTest.php b/tests/unit/Newsletter/ShortcodesTest.php index 50c394a1d4..690e4961d9 100644 --- a/tests/unit/Newsletter/ShortcodesTest.php +++ b/tests/unit/Newsletter/ShortcodesTest.php @@ -165,25 +165,25 @@ class ShortcodesTest extends MailPoetTest { $shortcodes_object = $this->shortcodes_object; $result = $shortcodes_object->process(array('[link:subscription_unsubscribe]')); - expect(preg_match('/^$/', $result['0']))->equals(1); - expect(preg_match('/action=unsubscribe/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^$/'); + expect($result['0'])->regExp('/action=unsubscribe/'); $result = $shortcodes_object->process(array('[link:subscription_unsubscribe_url]')); - expect(preg_match('/^http.*?action=unsubscribe/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^http.*?action=unsubscribe/'); $result = $shortcodes_object->process(array('[link:subscription_manage]')); - expect(preg_match('/^$/', $result['0']))->equals(1); - expect(preg_match('/action=manage/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^$/'); + expect($result['0'])->regExp('/action=manage/'); $result = $shortcodes_object->process(array('[link:subscription_manage_url]')); - expect(preg_match('/^http.*?action=manage/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^http.*?action=manage/'); $result = $shortcodes_object->process(array('[link:newsletter_view_in_browser]')); - expect(preg_match('/^$/', $result['0']))->equals(1); - expect(preg_match('/endpoint=view_in_browser/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^$/'); + expect($result['0'])->regExp('/endpoint=view_in_browser/'); $result = $shortcodes_object->process(array('[link:newsletter_view_in_browser_url]')); - expect(preg_match('/^http.*?endpoint=view_in_browser/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^http.*?endpoint=view_in_browser/'); } function testItReturnsShortcodeWhenTrackingEnabled() { @@ -191,9 +191,9 @@ class ShortcodesTest extends MailPoetTest { $shortcode = '[link:subscription_unsubscribe_url]'; $result = $shortcodes_object->process(array($shortcode)); - expect(preg_match('/^http.*?action=unsubscribe/', $result['0']))->equals(1); + expect($result['0'])->regExp('/^http.*?action=unsubscribe/'); Setting::setValue('tracking.enabled', true); - $shortcodes = array( + $initial_shortcodes = array( '[link:subscription_unsubscribe]', '[link:subscription_unsubscribe_url]', '[link:subscription_manage]', @@ -201,13 +201,26 @@ class ShortcodesTest extends MailPoetTest { '[link:newsletter_view_in_browser]', '[link:newsletter_view_in_browser_url]' ); + $expected_transformed_shortcodes = array( + '[link:subscription_unsubscribe_url]', + '[link:subscription_unsubscribe_url]', + '[link:subscription_manage_url]', + '[link:subscription_manage_url]', + '[link:newsletter_view_in_browser_url]', + '[link:newsletter_view_in_browser_url]' + ); // tracking function only works during sending, so queue object must not be false $shortcodes_object->queue = true; - $result = - $shortcodes_object->process($shortcodes); - // all returned shortcodes must end with url - $result = join(',', $result); - expect(substr_count($result, '_url'))->equals(count($shortcodes)); + $result = $shortcodes_object->process($initial_shortcodes); + foreach($result as $index => $transformed_shortcode) { + // 1. result must not contain a link + expect($transformed_shortcode)->regExp('/^((?!href="http).)*$/'); + // 2. result must include a URL shortcode. for example: + // [link:subscription_unsubscribe] should become + // [link:subscription_unsubscribe_url] + expect($transformed_shortcode) + ->regExp('/' . preg_quote($expected_transformed_shortcodes[$index]) . '/'); + } } function testItCanProcessCustomLinkShortcodes() {