Links are not re-hashed when re-rendering the same newsletter

lib/Cron/Workers/SendingQueue/SendingQueue.php

@@ -5,6 +5,7 @@ use MailPoet\Cron\CronHelper;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Links;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Mailer as MailerTask;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Newsletter as NewsletterTask;
+use MailPoet\Logger;
 use MailPoet\Mailer\MailerLog;
 use MailPoet\Models\Newsletter as NewsletterModel;
 use MailPoet\Models\SendingQueue as SendingQueueModel;

lib/Cron/Workers/SendingQueue/Tasks/Links.php

@@ -15,15 +15,15 @@ if(!defined('ABSPATH')) exit;
 class Links {
   static function process($rendered_newsletter, $newsletter, $queue) {
     list($rendered_newsletter, $links) =
-      self::hashAndReplaceLinks($rendered_newsletter);
+      self::hashAndReplaceLinks($rendered_newsletter, $newsletter->id, $queue->id);
     self::saveLinks($links, $newsletter, $queue);
     return $rendered_newsletter;
   }
 
-  static function hashAndReplaceLinks($rendered_newsletter) {
+  static function hashAndReplaceLinks($rendered_newsletter, $newsletter_id, $queue_id) {
     // join HTML and TEXT rendered body into a text string
     $content = Helpers::joinObject($rendered_newsletter);
-    list($content, $links) = NewsletterLinks::process($content);
+    list($content, $links) = NewsletterLinks::process($content, $newsletter_id, $queue_id);
     // split the processed body with hashed links back to HTML and TEXT
     list($rendered_newsletter['html'], $rendered_newsletter['text'])
       = Helpers::splitObject($content);

lib/Cron/Workers/SendingQueue/Tasks/Newsletter.php

@@ -5,6 +5,7 @@ namespace MailPoet\Cron\Workers\SendingQueue\Tasks;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Links as LinksTask;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Posts as PostsTask;
 use MailPoet\Cron\Workers\SendingQueue\Tasks\Shortcodes as ShortcodesTask;
+use MailPoet\Logger;
 use MailPoet\Mailer\MailerLog;
 use MailPoet\Models\Newsletter as NewsletterModel;
 use MailPoet\Models\NewsletterSegment as NewsletterSegmentModel;

lib/Newsletter/Links/Links.php

@@ -1,6 +1,7 @@
 <?php
 namespace MailPoet\Newsletter\Links;
 
+use MailPoet\Logger;
 use MailPoet\Models\NewsletterLink;
 use MailPoet\Models\Subscriber;
 use MailPoet\Newsletter\Shortcodes\Categories\Link;
@@ -8,8 +9,8 @@ use MailPoet\Newsletter\Shortcodes\Shortcodes;
 use MailPoet\Router\Endpoints\Track as TrackEndpoint;
 use MailPoet\Router\Router;
 use MailPoet\Util\Helpers;
-use MailPoet\Util\pQuery\pQuery as DomParser;
 use MailPoet\Util\Security;
+use MailPoet\Util\pQuery\pQuery as DomParser;
 
 class Links {
   const DATA_TAG_CLICK = '[mailpoet_click_data]';
@@ -17,9 +18,13 @@ class Links {
   const LINK_TYPE_SHORTCODE = 'shortcode';
   const LINK_TYPE_URL = 'link';
 
-  static function process($content) {
+  static function process($content, $newsletter_id, $queue_id) {
     $extracted_links = self::extract($content);
-    $processed_links = self::hash($extracted_links);
+    Logger::log('extracted', $extracted_links);
+    $saved_links = self::load($newsletter_id, $queue_id);
+    Logger::log('saved', $saved_links);
+    $processed_links = self::hash($extracted_links, $saved_links);
+    Logger::log('processed', $processed_links);
     return self::replace($content, $processed_links);
   }
 
@@ -51,13 +56,31 @@ class Links {
     return array_unique($extracted_links, SORT_REGULAR);
   }
 
-  static function hash($extracted_links) {
-    $processed_links = array();
+  static function load($newsletter_id, $queue_id) {
+    $links = NewsletterLink::whereEqual('newsletter_id', $newsletter_id)
+      ->whereEqual('queue_id', $queue_id)
+      ->findMany();
+    $saved_links = array();
+    foreach ($links as $link) {
+      $saved_links[$link->url] = $link->asArray();
+    }
+    return $saved_links;
+  }
+
+  static function hash($extracted_links, $saved_links) {
+    $processed_links = array_map(function(&$link) {
+      $link['type'] = Links::LINK_TYPE_URL;
+      $link['link'] = $link['url'];
+      $link['processed_link'] = self::DATA_TAG_CLICK . '-' . $link['hash'];
+      return $link;
+    }, $saved_links);
     foreach($extracted_links as $extracted_link) {
+      $link = $extracted_link['link'];
+      if (array_key_exists($link, $processed_links))
+        continue;
       $hash = Security::generateHash();
       // Use URL as a key to map between extracted and processed links
       // regardless of their sequential position (useful for link skips etc.)
-      $link = $extracted_link['link'];
       $processed_links[$link] = array(
         'type' => $extracted_link['type'],
         'hash' => $hash,
@@ -137,6 +160,8 @@ class Links {
 
   static function save(array $links, $newsletter_id, $queue_id) {
     foreach($links as $link) {
+      if (isset($link['id']))
+        continue;
       if(empty($link['hash']) || empty($link['link'])) continue;
       $newsletter_link = NewsletterLink::create();
       $newsletter_link->newsletter_id = $newsletter_id;

tests/unit/Cron/Workers/SendingQueue/Tasks/LinksTest.php

@@ -30,7 +30,7 @@ class LinksTest extends \MailPoetTest {
       'html' => '<a href="http://example.com">Example Link</a>',
       'text' => '<a href="http://example.com">Example Link</a>'
     );
-    $result = Links::hashAndReplaceLinks($rendered_newsletter);
+    $result = Links::hashAndReplaceLinks($rendered_newsletter, 0, 0);
     $processed_rendered_newsletter_body = $result[0];
     $processed_and_hashed_links = $result[1];
     expect($processed_rendered_newsletter_body['html'])

tests/unit/Newsletter/Links/LinksTest.php

@@ -24,7 +24,7 @@ class LinksTest extends \MailPoetTest {
 
   function testItOnlyHashesAndReplacesLinksInAnchorTags() {
     $template = '<a href="http://example.com"><img src="http://example.com" /></a>';
-    $result = Links::process($template);
+    $result = Links::process($template, 0, 0);
     expect($result[0])->equals(
       sprintf(
         '<a href="%s-%s"><img src="http://example.com" /></a>',
@@ -34,6 +34,25 @@ class LinksTest extends \MailPoetTest {
     );
   }
 
+  function testItDoesnotRehashExistingLinks() {
+    $link = NewsletterLink::create();
+    $link->newsletter_id = 3;
+    $link->queue_id = 3;
+    $link->hash = 123;
+    $link->url = 'http://example.com';
+    $link->save();
+
+    $template = '<a href="http://example.com"><img src="http://example.com" /></a>';
+    $result = Links::process($template, 3, 3);
+    expect($result[0])->equals(
+      sprintf(
+        '<a href="%s-%s"><img src="http://example.com" /></a>',
+        Links::DATA_TAG_CLICK,
+        123
+      )
+    );
+  }
+
   function testItCanExtactLinkShortcodes() {
     $template = '[notlink:shortcode] [link:some_link_shortcode]';
     $result = Links::extract($template);
@@ -48,7 +67,7 @@ class LinksTest extends \MailPoetTest {
 
   function testItHashesAndReplacesLinks() {
     $template = '<a href="http://example.com">some site</a> [link:some_link_shortcode]';
-    list($updated_content, $hashed_links) = Links::process($template);
+    list($updated_content, $hashed_links) = Links::process($template, 0, 0);
 
     // 2 links were hashed
     expect(count($hashed_links))->equals(2);
@@ -63,7 +82,7 @@ class LinksTest extends \MailPoetTest {
 
   function testItHashesAndReplacesLinksWithSpecialCharacters() {
     $template = '<a href="http://сайт.cóm/彌撒時間">some site</a>';
-    $result = Links::process($template);
+    $result = Links::process($template, 0, 0);
     expect($result[0])->equals(
       sprintf(
         '<a href="%s-%s">some site</a>',
@@ -171,6 +190,28 @@ class LinksTest extends \MailPoetTest {
     expect($newsltter_link->url)->equals('http://example.com');
   }
 
+  function testItCanLoadLinks() {
+    $link = NewsletterLink::create();
+    $link->newsletter_id = 1;
+    $link->queue_id = 2;
+    $link->hash = 123;
+    $link->url = 'http://example.com';
+    $link->save();
+
+    $link = NewsletterLink::create();
+    $link->newsletter_id = 1;
+    $link->queue_id = 3;
+    $link->hash = 456;
+    $link->url = 'http://demo.com';
+    $link->save();
+
+    $links = Links::load(1, 2);
+    expect(is_array($links))->true();
+    expect(count($links))->equals(1);
+    expect($links['http://example.com']['hash'])->equals(123);
+    expect($links['http://example.com']['url'])->equals('http://example.com');
+  }
+
   function testItMatchesHashedLinks() {
     $regex = Links::getLinkRegex();
     expect((boolean)preg_match($regex, '[some_tag]-123'))->false();