diff --git a/config/services/parameters.yml b/config/services/parameters.yml index 18aa6ccf..9a9b4aea 100644 --- a/config/services/parameters.yml +++ b/config/services/parameters.yml @@ -14,3 +14,5 @@ parameters: notify_end_default: 'end@example.com' always_add_google_tracking: true click_track: true + # sign click-tracking links with the application secret, see LinkTrackTokenService + click_track_sign_with_hmac: false diff --git a/config/services/repositories.yml b/config/services/repositories.yml index 95952280..739b5254 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -11,6 +11,14 @@ services: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: - PhpList\Core\Domain\Analytics\Model\LinkTrackUmlClick + PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository: + parent: PhpList\Core\Domain\Common\Repository\AbstractRepository + arguments: + - PhpList\Core\Domain\Analytics\Model\LinkTrackForward + PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository: + parent: PhpList\Core\Domain\Common\Repository\AbstractRepository + arguments: + - PhpList\Core\Domain\Analytics\Model\LinkTrackMl PhpList\Core\Domain\Configuration\Repository\ConfigRepository: diff --git a/config/services/services.yml b/config/services/services.yml index ac841afa..ae06bc01 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -61,6 +61,15 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService: + autowire: true + autoconfigure: true + + PhpList\Core\Domain\Analytics\Service\LinkClickService: + autowire: true + autoconfigure: true + public: true + PhpList\Core\Domain\Analytics\Service\AnalyticsService: autowire: true autoconfigure: true diff --git a/src/Domain/Analytics/Model/Dto/LinkTrackToken.php b/src/Domain/Analytics/Model/Dto/LinkTrackToken.php new file mode 100644 index 00000000..4b6746a6 --- /dev/null +++ b/src/Domain/Analytics/Model/Dto/LinkTrackToken.php @@ -0,0 +1,30 @@ +type === self::TYPE_HTML; + } + + public static function isValidType(string $type): bool + { + return in_array($type, [self::TYPE_HTML, self::TYPE_TEXT], true); + } +} diff --git a/src/Domain/Analytics/Model/UserMessageView.php b/src/Domain/Analytics/Model/UserMessageView.php index 66240fee..f5eeb46c 100644 --- a/src/Domain/Analytics/Model/UserMessageView.php +++ b/src/Domain/Analytics/Model/UserMessageView.php @@ -15,7 +15,7 @@ #[ORM\Index(name: 'phplist_user_message_view_msgidx', columns: ['messageid'])] #[ORM\Index(name: 'phplist_user_message_view_useridx', columns: ['userid'])] #[ORM\Index(name: 'phplist_user_message_view_usermsgidx', columns: ['userid', 'messageid'])] -// todo: #[ORM\Index(name: 'phplist_user_message_view_viewedidx', columns: ['viewed'])] +// @performance #[ORM\Index(name: 'phplist_user_message_view_viewedidx', columns: ['viewed'])] class UserMessageView implements DomainModel, Identity { #[ORM\Id] diff --git a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php index 38de3f74..1db61194 100644 --- a/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackForwardRepository.php @@ -4,6 +4,9 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; +use Doctrine\DBAL\Types\Types; +use PhpList\Core\Domain\Analytics\Model\LinkTrackForward; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; @@ -11,4 +14,78 @@ class LinkTrackForwardRepository extends AbstractRepository implements PaginatableRepositoryInterface { use CursorPaginationTrait; + + public function findOneByUuid(string $uuid): ?LinkTrackForward + { + return $this->findOneBy(['uuid' => $uuid]); + } + + public function findOneByUrlHash(string $urlHash): ?LinkTrackForward + { + return $this->findOneBy(['urlHash' => $urlHash]); + } + + /** + * Returns the forward for a (cleaned) URL, creating it when it does not exist yet. Creation uses a plain + * insert, so two workers racing on the same URL end up with the one row enforced by the urlhash unique key + * instead of a closed entity manager - this is an exception of the usual workflow of creating entities in manager + * classes. + */ + public function findOrCreateByUrl(string $url, bool $personalise): LinkTrackForward + { + $urlHash = md5($url); + + $forward = $this->findOneByUrlHash($urlHash); + if ($forward === null) { + // false when a concurrent worker created it first, the select then finds that row + $this->insert($url, $urlHash, $personalise); + $forward = $this->findOneByUrlHash($urlHash); + } + + if (empty($forward->getUuid())) { + $metadata = $this->getClassMetadata(); + $uuidColumn = $metadata->getColumnName('uuid'); + // only fills an empty uuid, so a worker that got there first is never overwritten + $this->getEntityManager()->getConnection()->executeStatement( + sprintf( + 'UPDATE %s SET %s = :uuid WHERE %s = :id AND (%s IS NULL OR %s = \'\')', + $metadata->getTableName(), + $uuidColumn, + $metadata->getColumnName('id'), + $uuidColumn, + $uuidColumn + ), + ['uuid' => self::generateUuid(), 'id' => $forward->getId()], + ); + $this->getEntityManager()->refresh($forward); + } + + return $forward; + } + + private function insert(string $url, string $urlHash, bool $personalise): bool + { + $metadata = $this->getClassMetadata(); + try { + $this->getEntityManager()->getConnection()->insert( + $metadata->getTableName(), + [ + $metadata->getColumnName('url') => $url, + $metadata->getColumnName('urlHash') => $urlHash, + $metadata->getColumnName('personalise') => $personalise, + $metadata->getColumnName('uuid') => self::generateUuid(), + ], + [$metadata->getColumnName('personalise') => Types::BOOLEAN], + ); + } catch (UniqueConstraintViolationException) { + return false; + } + + return true; + } + + private static function generateUuid(): string + { + return bin2hex(random_bytes(18)); + } } diff --git a/src/Domain/Analytics/Repository/LinkTrackMlRepository.php b/src/Domain/Analytics/Repository/LinkTrackMlRepository.php index 6914105b..3e1292b4 100644 --- a/src/Domain/Analytics/Repository/LinkTrackMlRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackMlRepository.php @@ -4,8 +4,106 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use DateTimeInterface; +use Doctrine\DBAL\Exception; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use PhpList\Core\Domain\Common\Repository\AbstractRepository; +/** + * Per campaign and link aggregates. Counters are changed with atomic statements so that concurrent sending + * workers and clicking subscribers cannot lose updates. Column names come from the entity mapping. + */ class LinkTrackMlRepository extends AbstractRepository { + /** Counts one more email of the campaign that contains the link. */ + public function incrementTotal(int $messageId, int $forwardId): void + { + $total = $this->column('total'); + + $this->upsert( + updateSql: sprintf('%1$s = COALESCE(%1$s, 0) + 1', $total), + updateParams: [], + insertColumns: [$total => 1], + messageId: $messageId, + forwardId: $forwardId, + ); + } + + public function recordClick(int $messageId, int $forwardId, bool $isHtml, DateTimeInterface $now): void + { + $timestamp = $now->format('Y-m-d H:i:s'); + [$clicked, $firstClick, $latestClick, $htmlClicked, $textClicked] = array_map( + $this->column(...), + ['clicked', 'firstClick', 'latestClick', 'htmlClicked', 'textClicked'] + ); + + $this->upsert( + updateSql: sprintf( + '%1$s = COALESCE(%1$s, 0) + 1, %2$s = COALESCE(%2$s, :now), %3$s = :now,' + . ' %4$s = COALESCE(%4$s, 0) + :html, %5$s = COALESCE(%5$s, 0) + :text', + $clicked, + $firstClick, + $latestClick, + $htmlClicked, + $textClicked + ), + updateParams: ['now' => $timestamp, 'html' => (int) $isHtml, 'text' => (int) !$isHtml], + insertColumns: [ + $clicked => 1, + $firstClick => $timestamp, + $latestClick => $timestamp, + $htmlClicked => (int) $isHtml, + $textClicked => (int) !$isHtml, + ], + messageId: $messageId, + forwardId: $forwardId, + ); + } + + private function column(string $field): string + { + return $this->getClassMetadata()->getColumnName($field); + } + + /** + * @param string $updateSql the assignments of the UPDATE, without SET + * @param array $updateParams + * @param array $insertColumns column => value + * @throws Exception + */ + private function upsert( + string $updateSql, + array $updateParams, + array $insertColumns, + int $messageId, + int $forwardId, + ): void { + $connection = $this->getEntityManager()->getConnection(); + $tableName = $this->getClassMetadata()->getTableName(); + $messageColumn = $this->column('messageId'); + $forwardColumn = $this->column('forwardId'); + + $sql = sprintf( + 'UPDATE %s SET %s WHERE %s = :messageId AND %s = :forwardId', + $connection->quoteIdentifier($tableName), + $updateSql, + $messageColumn, + $forwardColumn + ); + $params = $updateParams + ['messageId' => $messageId, 'forwardId' => $forwardId]; + + if ($connection->executeStatement($sql, $params) > 0) { + return; + } + + try { + $connection->insert( + $tableName, + [$messageColumn => $messageId, $forwardColumn => $forwardId] + $insertColumns + ); + } catch (UniqueConstraintViolationException) { + // inserted by a concurrent request in the meantime + $connection->executeStatement($sql, $params); + } + } } diff --git a/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php b/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php index 5662d672..2f3254c9 100644 --- a/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php +++ b/src/Domain/Analytics/Repository/LinkTrackUmlClickRepository.php @@ -4,6 +4,8 @@ namespace PhpList\Core\Domain\Analytics\Repository; +use DateTimeInterface; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use PhpList\Core\Domain\Common\Repository\AbstractRepository; use PhpList\Core\Domain\Common\Repository\CursorPaginationTrait; use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; @@ -11,4 +13,67 @@ class LinkTrackUmlClickRepository extends AbstractRepository implements PaginatableRepositoryInterface { use CursorPaginationTrait; + + /** + * Counts a click of a subscriber on a link of a campaign, creating the row on the first click. Column names + * come from the entity mapping. + */ + public function recordClick( + int $messageId, + int $userId, + int $forwardId, + bool $isHtml, + DateTimeInterface $now, + ): void { + $connection = $this->getEntityManager()->getConnection(); + $metadata = $this->getClassMetadata(); + $timestamp = $now->format('Y-m-d H:i:s'); + [$message, $user, $forward, $clicked, $firstClick, $latestClick, $htmlClicked, $textClicked] = array_map( + $metadata->getColumnName(...), + ['messageId', 'userId', 'forwardId', 'clicked', 'firstClick', 'latestClick', 'htmlClicked', 'textClicked'] + ); + + $sql = sprintf( + 'UPDATE %4$s SET %5$s = COALESCE(%5$s, 0) + 1, %6$s = COALESCE(%6$s, :now), %7$s = :now,' + . ' %8$s = COALESCE(%8$s, 0) + :html, %9$s = COALESCE(%9$s, 0) + :text' + . ' WHERE %1$s = :messageId AND %2$s = :userId AND %3$s = :forwardId', + $message, + $user, + $forward, + $connection->quoteIdentifier($metadata->getTableName()), + $clicked, + $firstClick, + $latestClick, + $htmlClicked, + $textClicked + ); + $params = [ + 'now' => $timestamp, + 'html' => (int) $isHtml, + 'text' => (int) !$isHtml, + 'messageId' => $messageId, + 'userId' => $userId, + 'forwardId' => $forwardId, + ]; + + if ($connection->executeStatement($sql, $params) > 0) { + return; + } + + try { + $connection->insert($metadata->getTableName(), [ + $message => $messageId, + $user => $userId, + $forward => $forwardId, + $firstClick => $timestamp, + $latestClick => $timestamp, + $clicked => 1, + $htmlClicked => (int) $isHtml, + $textClicked => (int) !$isHtml, + ]); + } catch (UniqueConstraintViolationException) { + // inserted by a concurrent request in the meantime + $connection->executeStatement($sql, $params); + } + } } diff --git a/src/Domain/Analytics/Service/LinkClickService.php b/src/Domain/Analytics/Service/LinkClickService.php new file mode 100644 index 00000000..538f2dd3 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -0,0 +1,108 @@ + $metadata HTTP_USER_AGENT, HTTP_REFERER and client_ip of the request + * @return string|null the url to redirect to, or null when the token is invalid or the link is unknown + */ + public function trackClick(string $tid, ?string $signature, array $metadata): ?string + { + $token = $this->tokenService->parse($tid, $signature); + if ($token === null) { + return null; + } + + $forward = $this->forwardRepository->findOneByUuid($token->forwardUuid); + $message = $this->messageRepository->findOneBy(['uuid' => $token->messageUuid]); + $subscriber = $this->subscriberRepository->findOneBy(['uuid' => $token->subscriberUuid]); + if (!$forward instanceof LinkTrackForward + || !$message instanceof Message + || !$subscriber instanceof Subscriber + ) { + return null; + } + + $userMessage = $this->userMessageRepository->findByUserAndMessage($subscriber, $message); + $url = $this->resolveUrl($forward, $subscriber, $userMessage); + if ($url === null) { + return null; + } + + $now = new DateTimeImmutable(); + $messageId = (int) $message->getId(); + $forwardId = (int) $forward->getId(); + $this->mlRepository->recordClick($messageId, $forwardId, $token->isHtml(), $now); + + // a click on a link also proves the campaign was opened, but only the first one counts as a view + if ($userMessage !== null && !$userMessage->isViewed()) { + $this->userMessageService->trackUserMessageView($subscriber->getUniqueId(), $messageId, $metadata); + } + + $this->umlClickRepository->recordClick( + $messageId, + (int) $subscriber->getId(), + $forwardId, + $token->isHtml(), + $now + ); + + return $url; + } + + private function resolveUrl(LinkTrackForward $forward, Subscriber $subscriber, ?UserMessage $userMessage): ?string + { + $url = $forward->getUrl(); + if ($url === null) { + return null; + } + + if ($forward->isPersonalise()) { + // personalised links are only valid for subscribers the campaign was actually sent to + if ($userMessage === null) { + return null; + } + // the uid has to go before any fragment, which is never sent to the server + $fragmentPos = strpos($url, '#'); + $fragment = $fragmentPos === false ? '' : substr($url, $fragmentPos); + $base = $fragmentPos === false ? $url : substr($url, 0, $fragmentPos); + $url = $base . (str_contains($base, '?') ? '&' : '?') . 'uid=' . $subscriber->getUniqueId() . $fragment; + } + + return preg_match('~^(https?|ftp)://~i', $url) ? $url : null; + } +} diff --git a/src/Domain/Analytics/Service/LinkTrackService.php b/src/Domain/Analytics/Service/LinkTrackService.php index dc478e84..05a9baa6 100644 --- a/src/Domain/Analytics/Service/LinkTrackService.php +++ b/src/Domain/Analytics/Service/LinkTrackService.php @@ -4,19 +4,37 @@ namespace PhpList\Core\Domain\Analytics\Service; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Core\ParameterProvider; -use PhpList\Core\Domain\Analytics\Exception\MissingMessageIdException; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; +use PhpList\Core\Domain\Analytics\Model\Dto\LinkTrackToken; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; - +use PhpList\Core\Domain\Messaging\Model\Message; +use PhpList\Core\Domain\Subscription\Model\Subscriber; + +/** + * Replaces the links of a campaign email with click-tracking links, like phplist3's sendemaillib.php. + * + * Each distinct URL is stored once as a LinkTrackForward. The replacement link carries a token that identifies + * the forward, the campaign and the subscriber (see LinkTrackTokenService); LinkClickService resolves it again. + */ class LinkTrackService { + private const MAX_URL_LENGTH = 255; + private const HTML_ANCHOR_PATTERN = '/(]*?[\s"\'])?)href=(["\'])(.*?)\2([^>]*>)(.*?)<\/a>/is'; + private const TEXT_URL_PATTERN = '#https?://[^\s<>},]+#i'; + private const PLACEHOLDER_PATTERN = '/\[[A-Z][A-Z0-9_]*(?::[^\]]*)?\]/'; + + /** @var array forwards by url hash, kept for the worker's lifetime */ + private array $forwards = []; + public function __construct( private readonly LinkTrackRepository $linkTrackRepository, + private readonly LinkTrackForwardRepository $linkTrackForwardRepository, + private readonly LinkTrackMlRepository $linkTrackMlRepository, + private readonly LinkTrackTokenService $tokenService, private readonly ParameterProvider $paramProvider, - private readonly EntityManagerInterface $entityManager, ) { } @@ -26,71 +44,165 @@ public function getUrlById(int $id): ?string return $linkTrack?->getUrl(); } - public function isExtractAndSaveLinksApplicable(): bool + public function isClickTrackingEnabled(): bool { return (bool)$this->paramProvider->get('click_track', false); } /** - * Extract links from message content and save them to the LinkTrackRepository + * Returns a copy of the content in which the links of the HTML parts (content, HTML footer) and of the text + * parts (text content, text footer) point to the click-tracking endpoint for this subscriber. * - * @return LinkTrack[] The saved LinkTrack entities - * @throws MissingMessageIdException + * Links are left alone when they are not http(s)/ftp, are longer than the forward url column, still contain + * placeholders (the redirect could not expand them), or are shown as text that itself contains a URL (the + * rewrite would look like phishing). */ - public function extractAndSaveLinks(MessagePrecacheDto $content, int $userId, ?int $messageId = null): array - { - // todo: in case of forwarded message, we need to use 'forwarded' instead of user id - if (!$this->isExtractAndSaveLinksApplicable()) { - return []; + public function trackLinks( + MessagePrecacheDto $content, + Message $campaign, + Subscriber $subscriber + ): MessagePrecacheDto { + $messageUuid = (string) $campaign->getUuid(); + $subscriberUuid = $subscriber->getUuid(); + if (!$this->isClickTrackingEnabled() || $messageUuid === '' || $subscriberUuid === '') { + return $content; + } + + $tracked = clone $content; + $forwardIds = []; + $trackUrl = function (string $url, string $type) use ($messageUuid, $subscriberUuid, &$forwardIds): ?string { + return $this->createTrackingUrl($url, $type, $messageUuid, $subscriberUuid, $forwardIds); + }; + + $tracked->content = $this->rewriteHtml($tracked->content, $trackUrl); + $tracked->htmlFooter = $this->rewriteHtml($tracked->htmlFooter, $trackUrl); + $tracked->textContent = $this->rewriteText($tracked->textContent, $trackUrl); + if ($tracked->textFooter !== null) { + $tracked->textFooter = $this->rewriteText($tracked->textFooter, $trackUrl); } - if ($messageId === null) { - throw new MissingMessageIdException(); + foreach (array_keys($forwardIds) as $forwardId) { + $this->linkTrackMlRepository->incrementTotal((int) $campaign->getId(), $forwardId); } - $links = $this->extractLinksFromHtml($content->content); + return $tracked; + } - if ($content->htmlFooter) { - $links = array_merge($links, $this->extractLinksFromHtml($content->htmlFooter)); + /** @param callable(string, string): ?string $trackUrl */ + private function rewriteHtml(string $html, callable $trackUrl): string + { + if ($html === '') { + return $html; } - $links = array_unique($links); + return (string) preg_replace_callback( + self::HTML_ANCHOR_PATTERN, + function (array $match) use ($trackUrl): string { + $linkText = strip_tags($match[5]); + if (stripos($linkText, 'http://') !== false || stripos($linkText, 'https://') !== false) { + return $match[0]; + } + + $href = html_entity_decode(trim($match[3]), ENT_QUOTES | ENT_HTML5); + $url = $trackUrl($href, LinkTrackToken::TYPE_HTML); + if ($url === null) { + return $match[0]; + } + + return $match[1] . 'href=' . $match[2] . htmlspecialchars($url, ENT_QUOTES) . $match[2] + . $match[4] . $match[5] . ''; + }, + $html + ); + } - $savedLinks = []; - foreach ($links as $url) { - $existingLinkTrack = $this->linkTrackRepository->findByUrlUserIdAndMessageId($url, $userId, $messageId); - if ($existingLinkTrack !== null) { - $savedLinks[] = $existingLinkTrack; - continue; - } - $linkTrack = new LinkTrack(); - $linkTrack->setMessageId($messageId); - $linkTrack->setUserId($userId); - $linkTrack->setUrl($url); + /** @param callable(string, string): ?string $trackUrl */ + private function rewriteText(string $text, callable $trackUrl): string + { + if ($text === '') { + return $text; + } - $this->linkTrackRepository->persist($linkTrack); - $savedLinks[] = $linkTrack; + return (string) preg_replace_callback( + self::TEXT_URL_PATTERN, + function (array $match) use ($trackUrl): string { + $trailing = str_ends_with($match[0], '.') ? '.' : ''; + $link = substr($match[0], 0, strlen($match[0]) - strlen($trailing)); + $url = $trackUrl($link, LinkTrackToken::TYPE_TEXT); + + return $url === null ? $match[0] : $url . $trailing; + }, + $text + ); + } + + /** @param array $forwardIds collects the ids of the forwards used in the email */ + private function createTrackingUrl( + string $url, + string $type, + string $messageUuid, + string $subscriberUuid, + array &$forwardIds, + ): ?string { + $personalise = false; + $cleanUrl = $this->cleanUrl($url, $personalise); + + if (!preg_match('~^(https?|ftp)://~i', $cleanUrl) + || strlen($cleanUrl) > self::MAX_URL_LENGTH + || preg_match(self::PLACEHOLDER_PATTERN, $cleanUrl) + || str_contains($cleanUrl, LinkTrackTokenService::ENDPOINT_PATH . '?tid=') + ) { + return null; } - $this->entityManager->flush(); + $forward = $this->getForward($cleanUrl, $personalise); + $forwardIds[$forward['id']] = true; + + return $this->tokenService->createUrl( + new LinkTrackToken($type, $forward['uuid'], $messageUuid, $subscriberUuid) + ); + } + + /** @return array{id: int, uuid: string} */ + private function getForward(string $cleanUrl, bool $personalise): array + { + $urlHash = md5($cleanUrl); + if (!isset($this->forwards[$urlHash])) { + $forward = $this->linkTrackForwardRepository->findOrCreateByUrl($cleanUrl, $personalise); + $this->forwards[$urlHash] = ['id' => (int) $forward->getId(), 'uuid' => (string) $forward->getUuid()]; + } - return $savedLinks; + return $this->forwards[$urlHash]; } /** - * Extract links from HTML content - * - * @return string[] The extracted links + * Removes the personal `uid` and `PHPSESSID` query parameters, which must not end up in the stored url. + * $personalise is set when a `uid` was present: the redirect then adds the subscriber's uid again. */ - private function extractLinksFromHtml(string $html): array + private function cleanUrl(string $url, bool &$personalise): string { - $links = []; + $fragment = ''; + $fragmentPosition = strpos($url, '#'); + if ($fragmentPosition !== false) { + $fragment = substr($url, $fragmentPosition); + $url = substr($url, 0, $fragmentPosition); + } - $pattern = '/]*href=(["\'])([^"\']+)\1[^>]*>/i'; - if (preg_match_all($pattern, $html, $matches)) { - $links = $matches[2]; + $queryPosition = strpos($url, '?'); + if ($queryPosition === false) { + return $url . $fragment; + } + + $kept = []; + foreach (explode('&', substr($url, $queryPosition + 1)) as $pair) { + $name = strtolower(urldecode(explode('=', $pair, 2)[0])); + if ($name === 'uid') { + $personalise = true; + } elseif ($name !== 'phpsessid' && $pair !== '') { + $kept[] = $pair; + } } - return $links; + return substr($url, 0, $queryPosition) . ($kept === [] ? '' : '?' . implode('&', $kept)) . $fragment; } } diff --git a/src/Domain/Analytics/Service/LinkTrackTokenService.php b/src/Domain/Analytics/Service/LinkTrackTokenService.php new file mode 100644 index 00000000..796432ea --- /dev/null +++ b/src/Domain/Analytics/Service/LinkTrackTokenService.php @@ -0,0 +1,101 @@ +/lt?tid=`), the equivalent of + * phplist3's lt.php links. + * + * The token is the url-safe base64 of `|||`. The uuids are + * random, so a token cannot be guessed. With the `click_track_sign_with_hmac` parameter enabled the token is + * additionally signed (`hm`) with the application secret, so a request can be rejected before touching the + * database. Keep it off if the secret may be rotated, as that would invalidate the links of sent campaigns. + */ +class LinkTrackTokenService +{ + public const ENDPOINT_PATH = '/lt'; + + public function __construct( + private readonly ParameterProvider $parameterProvider, + #[Autowire('%app.rest_api_base_url%')] private readonly string $restApiBaseUrl, + #[Autowire('%app.secret%')] private readonly string $secret, + ) { + } + + public function createUrl(LinkTrackToken $token): string + { + $encoded = rtrim(strtr(base64_encode(implode('|', [ + $token->type, + $token->forwardUuid, + $token->messageUuid, + $token->subscriberUuid, + ])), '+/', '-_'), '='); + + $url = rtrim($this->restApiBaseUrl, '/') . self::ENDPOINT_PATH . '?tid=' . $encoded; + if ($this->isSigningEnabled()) { + $url .= '&hm=' . $this->sign($encoded); + } + + return $url; + } + + /** Returns null for malformed tokens and, when signing is enabled, for missing or wrong signatures. */ + public function parse(string $tid, ?string $signature = null): ?LinkTrackToken + { + if (!$this->hasValidSignature($tid, $signature)) { + return null; + } + + $decoded = base64_decode(strtr($tid, '-_', '+/'), true); + if ($decoded === false) { + return null; + } + + $parts = explode('|', $decoded); + if (count($parts) !== 4 + || !LinkTrackToken::isValidType($parts[0]) + || !$this->areValidUuids(array_slice($parts, 1)) + ) { + return null; + } + + return new LinkTrackToken($parts[0], $parts[1], $parts[2], $parts[3]); + } + + private function hasValidSignature(string $tid, ?string $signature): bool + { + if (!$this->isSigningEnabled()) { + return true; + } + + return $signature !== null && $signature !== '' && hash_equals($this->sign($tid), $signature); + } + + /** @param string[] $uuids */ + private function areValidUuids(array $uuids): bool + { + foreach ($uuids as $uuid) { + if (!preg_match('/^[A-Za-z0-9-]{1,64}$/', $uuid)) { + return false; + } + } + + return true; + } + + private function isSigningEnabled(): bool + { + return (bool) $this->parameterProvider->get('click_track_sign_with_hmac', false); + } + + private function sign(string $encodedToken): string + { + return hash_hmac('sha256', $encodedToken, $this->secret); + } +} diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index b8995b35..7073abf8 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -33,32 +33,53 @@ public function readFileContents(string $path): ?string } } - // todo: check - Handle temp-file rename and write failures. - // Appending an extension leaves the original temp file orphaned, and fopen/fwrite failures aren’t - // checked (so the method can return a path that wasn’t actually written) public function writeFileToDirectory(string $directory, string $originalFilename, string $contents): ?string { + $realDirectory = $this->resolveTrustedDirectory($directory); + if ($realDirectory === null) { + return null; + } + $pathInfo = pathinfo($originalFilename); $name = $pathInfo['filename'] === '' ? 'file' : $pathInfo['filename']; $ext = $pathInfo['extension'] ?? ''; - $newFile = tempnam($directory, $name); - if ($newFile === false) { + // Exclusive create (O_EXCL) never follows a symlink or reuses an existing entry, + // so the file is created and written in a single step without rename. + $fullPath = $realDirectory . '/' . $name . bin2hex(random_bytes(6)) . ($ext !== '' ? '.' . $ext : ''); + $handle = fopen($fullPath, 'xb'); + if ($handle === false) { return null; } - if ($ext !== '') { - $newFile .= '.' . $ext; + $written = fwrite($handle, $contents); + fclose($handle); + if ($written !== strlen($contents)) { + unlink($fullPath); + + return null; } - $relativeName = basename($newFile); - $fullPath = $directory . '/' . $relativeName; + return $fullPath; + } + + private function resolveTrustedDirectory(string $directory): ?string + { + if (!is_dir($directory) || !is_writable($directory)) { + return null; + } - $fileHandle = fopen($fullPath, 'w'); + $realDirectory = realpath($directory); + if ($realDirectory === false) { + return null; + } - fwrite($fileHandle, $contents); - fclose($fileHandle); + // Untrusted writers could swap entries in the directory between checks and writes + $perms = fileperms($realDirectory); + if ($perms === false || (($perms & 0022) !== 0 && ($perms & 01000) === 0)) { + return null; + } - return $fullPath; + return $realDirectory; } } diff --git a/src/Domain/Common/Storage/LocalUploadStorage.php b/src/Domain/Common/Storage/LocalUploadStorage.php index bc731088..b08bedb4 100644 --- a/src/Domain/Common/Storage/LocalUploadStorage.php +++ b/src/Domain/Common/Storage/LocalUploadStorage.php @@ -22,7 +22,6 @@ public function __construct( public function store(UploadedFile $file, string $filename): StoredFile { - // todo: get upload_dir from configs $targetDirectory = $this->projectDir . DIRECTORY_SEPARATOR . 'public' diff --git a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php index af7d08d2..c3b8ad07 100644 --- a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php +++ b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php @@ -48,7 +48,10 @@ public function __construct( public function __invoke(CampaignProcessorMessage|SyncCampaignProcessorMessage $data): void { - // todo: recheck this stuckCampaignThresholdSeconds logic + // Note: a running campaign only proves it is alive by updating `modified` when it sends. Keep + // the threshold larger than the longest expected gap without a send (throttle sleeps, slow + // precache, large subscriber queries), otherwise a second dispatch can reclaim a live campaign + // and two workers will process it at once. 0 disables reclaiming. $campaign = $this->messageRepository->tryClaimForProcessing( $data->getMessageId(), $this->stuckCampaignThresholdSeconds diff --git a/src/Domain/Messaging/Model/UserMessageBounce.php b/src/Domain/Messaging/Model/UserMessageBounce.php index 9a350f88..efb6ea61 100644 --- a/src/Domain/Messaging/Model/UserMessageBounce.php +++ b/src/Domain/Messaging/Model/UserMessageBounce.php @@ -19,7 +19,6 @@ #[ORM\Index(name: 'phplist_user_message_bounce_msgidx', columns: ['message'])] #[ORM\Index(name: 'phplist_user_message_bounce_umbindex', columns: ['user', 'message', 'bounce'])] #[ORM\Index(name: 'phplist_user_message_bounce_useridx', columns: ['user'])] -// todo: #[ORM\Index(name: 'phplist_user_message_bounce_timeidx', columns: ['time'])] class UserMessageBounce implements DomainModel, Identity, diff --git a/src/Domain/Messaging/Repository/BounceRepository.php b/src/Domain/Messaging/Repository/BounceRepository.php index e3e0ab4f..1ff480f8 100644 --- a/src/Domain/Messaging/Repository/BounceRepository.php +++ b/src/Domain/Messaging/Repository/BounceRepository.php @@ -97,4 +97,23 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes lastId: $lastId, ); } + + /** + * @param int[] $ids + * @return array indexed by bounce id + */ + public function findByIdsIndexedById(array $ids): array + { + if ($ids === []) { + return []; + } + + $bouncesById = []; + /** @var Bounce $bounce */ + foreach ($this->findBy(['id' => $ids]) as $bounce) { + $bouncesById[$bounce->getId()] = $bounce; + } + + return $bouncesById; + } } diff --git a/src/Domain/Messaging/Repository/MessageRepository.php b/src/Domain/Messaging/Repository/MessageRepository.php index c5698775..84f4da7d 100644 --- a/src/Domain/Messaging/Repository/MessageRepository.php +++ b/src/Domain/Messaging/Repository/MessageRepository.php @@ -7,7 +7,7 @@ use DateTime; use DateTimeImmutable; use DateTimeInterface; -use Doctrine\ORM\AbstractQuery; +use Doctrine\DBAL\Exception; use PhpList\Core\Domain\Common\Model\Filter\FilterRequestInterface; use PhpList\Core\Domain\Common\Model\PaginatedResult; use PhpList\Core\Domain\Common\Repository\AbstractRepository; @@ -175,7 +175,11 @@ public function findByIdAndStatus(int $id, Message\MessageStatus $status): ?Mess public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Message { $connection = $this->getEntityManager()->getConnection(); - $table = $connection->quoteIdentifier($this->getClassMetadata()->getTableName()); + $metadata = $this->getClassMetadata(); + $table = $connection->quoteIdentifier($metadata->getTableName()); + $idColumn = $metadata->getColumnName('id'); + $statusColumn = $metadata->getColumnName('metadata.status'); + $modifiedColumn = $metadata->getColumnName('updatedAt'); $now = new DateTime(); $params = [ @@ -185,9 +189,13 @@ public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Mes 'from' => Message\MessageStatus::Submitted->value, ]; - $claimCondition = 'status = :from'; + $claimCondition = sprintf('%s = :from', $statusColumn); if ($staleAfterSeconds > 0) { - $claimCondition = '(status = :from OR (status IN (:prepared, :inProcess) AND modified < :staleBefore))'; + $claimCondition = sprintf( + '(%1$s = :from OR (%1$s IN (:prepared, :inProcess) AND %2$s < :staleBefore))', + $statusColumn, + $modifiedColumn + ); $params['prepared'] = Message\MessageStatus::Prepared->value; $params['inProcess'] = Message\MessageStatus::InProcess->value; $params['staleBefore'] = (clone $now) @@ -195,7 +203,14 @@ public function tryClaimForProcessing(int $id, int $staleAfterSeconds = 0): ?Mes ->format('Y-m-d H:i:s'); } - $sql = sprintf('UPDATE %s SET status = :to, modified = :now WHERE id = :id AND %s', $table, $claimCondition); + $sql = sprintf( + 'UPDATE %s SET %s = :to, %s = :now WHERE %s = :id AND %s', + $table, + $statusColumn, + $modifiedColumn, + $idColumn, + $claimCondition + ); $affected = $connection->executeStatement($sql, $params); @@ -257,21 +272,50 @@ public function incrementSentCounts(int $messageId, OutputFormat $sentAs): void ->execute(); } + /** + * Non-empty columns of the message row keyed by their database column names (subject, fromfield, + * message, status, ...), like the legacy `select * from message`. Entity hydration is not used on + * purpose: it would key embedded values as `content.subject` etc. + * + * @return array + * @throws Exception + */ public function getNonEmptyFields(int $id): array { - $message = $this->createQueryBuilder('m') - ->where('m.id = :id') - ->setParameter('id', $id) - ->getQuery() - ->getOneOrNullResult(AbstractQuery::HYDRATE_ARRAY) ?? []; + $connection = $this->getEntityManager()->getConnection(); + $metadata = $this->getClassMetadata(); + + $message = $connection->fetchAssociative( + sprintf( + 'SELECT * FROM %s WHERE %s = :id', + $connection->quoteIdentifier($metadata->getTableName()), + $metadata->getColumnName('id') + ), + ['id' => $id] + ); - foreach ($message as $key => $value) { - if ($value === null || $value === '') { - unset($message[$key]); - } + return array_filter($message ?: [], static fn (mixed $value): bool => $value !== null && $value !== ''); + } + + /** + * Campaign ids and subjects for bounce reports, without any bounce counts. + * + * @param int|null $ownerId Limit results to campaigns owned by this admin when provided. + * @return array + */ + public function getSubjectsForBounceReport(?int $ownerId = null): array + { + $queryBuilder = $this->createQueryBuilder('m') + ->select('m.id AS messageId', 'm.content.subject AS subject') + ->orderBy('m.id', 'ASC'); + + if ($ownerId !== null) { + $queryBuilder + ->andWhere('IDENTITY(m.owner) = :ownerId') + ->setParameter('ownerId', $ownerId); } - return $message; + return $queryBuilder->getQuery()->getArrayResult(); } /** diff --git a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php index 9bc94e1f..6bca9465 100644 --- a/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php +++ b/src/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReader.php @@ -5,17 +5,15 @@ namespace PhpList\Core\Domain\Messaging\Repository; use DateTime; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Domain\Messaging\Model\Bounce; use PhpList\Core\Domain\Messaging\Model\Interfaces\UserMessageBounceRecordInterface; -use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Model\ReadModel\UserMessageBounceReadModel; use PhpList\Core\Domain\Messaging\Model\UserMessage; use PhpList\Core\Domain\Messaging\Model\UserMessageBounce; use PhpList\Core\Domain\Messaging\Repository\Interfaces\UserMessageBounceReportReaderInterface; use PhpList\Core\Domain\Search\Client\ElasticsearchClientInterface; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PhpList\Core\Domain\Subscription\Model\Subscription; +use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; /** * Reporting/joining queries that correlate Elasticsearch-backed bounce data with related entities @@ -33,11 +31,13 @@ */ class UserMessageBounceElasticsearchHybridReader implements UserMessageBounceReportReaderInterface { - // todo: move db queries into repositories and inject them here, rather than using the entity manager directly public function __construct( private readonly ElasticsearchClientInterface $client, private readonly string $indexPrefix, - private readonly EntityManagerInterface $entityManager, + private readonly SubscriberRepository $subscriberRepository, + private readonly MessageRepository $messageRepository, + private readonly UserMessageRepository $userMessageRepository, + private readonly BounceRepository $bounceRepository, ) { } @@ -52,21 +52,7 @@ public function __construct( */ public function getListBounceTotals(int $listId): array { - $rows = $this->entityManager->createQueryBuilder() - ->select( - 'subscriber.id AS subscriberId', - 'subscriber.email AS email', - 'subscriber.confirmed AS confirmed', - 'subscriber.blacklisted AS blacklisted' - ) - ->from(Subscriber::class, 'subscriber') - ->innerJoin(Subscription::class, 'subscription', 'ON', 'subscription.subscriber = subscriber') - ->where('IDENTITY(subscription.subscriberList) = :listId') - ->setParameter('listId', $listId) - ->groupBy('subscriber.id, subscriber.email, subscriber.confirmed, subscriber.blacklisted') - ->orderBy('subscriber.id', 'ASC') - ->getQuery() - ->getArrayResult(); + $rows = $this->subscriberRepository->getBounceReportRowsByListId($listId); if ($rows === []) { return []; @@ -99,19 +85,7 @@ public function getListBounceTotals(int $listId): array /** @return array */ public function getCampaignBounceTotals(?int $ownerId = null): array { - $queryBuilder = $this->entityManager->createQueryBuilder() - ->select('m.id AS messageId', 'm.content.subject AS subject') - ->from(Message::class, 'm') - ->orderBy('m.id', 'ASC'); - - if ($ownerId !== null) { - $queryBuilder - ->andWhere('IDENTITY(m.owner) = :ownerId') - ->setParameter('ownerId', $ownerId); - } - - /** @var array $rows */ - $rows = $queryBuilder->getQuery()->getArrayResult(); + $rows = $this->messageRepository->getSubjectsForBounceReport($ownerId); if ($rows === []) { return []; @@ -162,7 +136,7 @@ public function getPaginatedWithJoinNoRelation(int $fromId, int $limit): array $records ))); - $bouncesById = $this->findBouncesByIdIndexedById($bounceIds); + $bouncesById = $this->bounceRepository->findByIdsIndexedById($bounceIds); $result = []; foreach ($records as $record) { @@ -187,24 +161,14 @@ public function getPaginatedWithJoinNoRelation(int $fromId, int $limit): array */ public function getUserMessageHistoryWithBounces(Subscriber $subscriber): array { - /** @var UserMessage[] $userMessages */ - $userMessages = $this->entityManager->createQueryBuilder() - ->select('um') - ->from(UserMessage::class, 'um') - ->where('um.user = :userId') - ->andWhere('um.status = :status') - ->setParameter('userId', $subscriber->getId()) - ->setParameter('status', 'sent') - ->orderBy('um.createdAt', 'DESC') - ->getQuery() - ->getResult(); + $userMessages = $this->userMessageRepository->findSentByUserId((int) $subscriber->getId()); if ($userMessages === []) { return []; } $docsByMessageId = $this->groupByMessageId($this->fetchDocsByUserId((int) $subscriber->getId())); - $bouncesById = $this->findBouncesByIdIndexedById($this->bounceIdsUsedIn($docsByMessageId)); + $bouncesById = $this->bounceRepository->findByIdsIndexedById($this->bounceIdsUsedIn($docsByMessageId)); $result = []; foreach ($userMessages as $userMessage) { @@ -274,25 +238,6 @@ private function bounceIdsUsedIn(array $docsByMessageId): array return array_values(array_unique($bounceIds)); } - /** - * @param int[] $bounceIds - * @return array - */ - private function findBouncesByIdIndexedById(array $bounceIds): array - { - if ($bounceIds === []) { - return []; - } - - $bouncesById = []; - /** @var Bounce $bounce */ - foreach ($this->entityManager->getRepository(Bounce::class)->findBy(['id' => $bounceIds]) as $bounce) { - $bouncesById[$bounce->getId()] = $bounce; - } - - return $bouncesById; - } - /** @param array{_source: array} $hit */ private function hydrate(array $hit): UserMessageBounceReadModel { diff --git a/src/Domain/Messaging/Repository/UserMessageRepository.php b/src/Domain/Messaging/Repository/UserMessageRepository.php index bf307737..b92fa470 100644 --- a/src/Domain/Messaging/Repository/UserMessageRepository.php +++ b/src/Domain/Messaging/Repository/UserMessageRepository.php @@ -39,6 +39,21 @@ public function countSentBetween(DateTimeInterface $start, DateTimeInterface $en ->getSingleScalarResult(); } + /** + * @return UserMessage[] + */ + public function findSentByUserId(int $userId): array + { + return $this->createQueryBuilder('um') + ->where('um.user = :userId') + ->andWhere('um.status = :status') + ->setParameter('userId', $userId) + ->setParameter('status', UserMessageStatus::Sent->value) + ->orderBy('um.createdAt', 'DESC') + ->getQuery() + ->getResult(); + } + /** * Counts how many user messages have status "sent" since the given time. */ diff --git a/src/Domain/Messaging/Service/ForwardContentService.php b/src/Domain/Messaging/Service/ForwardContentService.php index 2fa0e17d..48320a21 100644 --- a/src/Domain/Messaging/Service/ForwardContentService.php +++ b/src/Domain/Messaging/Service/ForwardContentService.php @@ -19,7 +19,6 @@ class ForwardContentService { public function __construct( private readonly CacheInterface $cache, - private readonly MessageProcessingPreparator $messagePreparator, private readonly ForwardEmailBuilder $forwardEmailBuilder, ) { } @@ -39,18 +38,13 @@ public function getContents( throw new MessageCacheMissingException(); } - // todo: check how should links be handled in case of forwarding - $processed = $this->messagePreparator->processMessageLinks( - campaignId: $campaign->getId(), - cachedMessageDto: $messagePrecacheDto, - subscriber: $forwardingSubscriber - ); - + // links of forwarded messages are not click tracked: the forwarding subscriber would + // be credited with the clicks of the friend return $this->forwardEmailBuilder->buildForwardEmail( messageId: $campaign->getId(), friendEmail: $friendEmail, forwardedBy: $forwardingSubscriber, - data: $processed, + data: $messagePrecacheDto, htmlPref: $forwardingSubscriber->hasHtmlEmail(), fromName: $forwardDto->getFromName(), fromEmail: $forwardDto->getFromEmail(), diff --git a/src/Domain/Messaging/Service/Manager/TemplateImageManager.php b/src/Domain/Messaging/Service/Manager/TemplateImageManager.php index bb6c0771..15cfbc6c 100644 --- a/src/Domain/Messaging/Service/Manager/TemplateImageManager.php +++ b/src/Domain/Messaging/Service/Manager/TemplateImageManager.php @@ -211,10 +211,14 @@ public function createCachedLogoImage(int $size): void $orgLogoImage ); + // resized output is always PNG, so take the mime type from the final content + $finalSize = getimagesizefromstring($imageContent); + $mimeType = $finalSize !== false ? $finalSize['mime'] : $imgSize['mime']; + // else copy original $templateImage = (new TemplateImage()) ->setFilename('ORGANISATIONLOGO' . $size . '.png') - ->setMimetype($imgSize['mime']) + ->setMimetype($mimeType) ->setData(base64_encode($imageContent)) ->setWidth($newWidth) ->setHeight($newHeight); @@ -266,7 +270,7 @@ private function resizeImageIfNecessary( //# creates a black image (why would you want that....) $resized = imagecreatetruecolor($newWidth, $newHeight); imagesavealpha($resized, true); - //# white. All the methods to make it transparent didn't work for me @@TODO really make transparent + //# white. All the methods to make it transparent didn't work for me $transparent = imagecolorallocatealpha($resized, 255, 255, 255, 127); imagefill($resized, 0, 0, $transparent); [$sizeW, $sizeH] = [$imgSize[0], $imgSize[1]]; diff --git a/src/Domain/Messaging/Service/MessageDataLoader.php b/src/Domain/Messaging/Service/MessageDataLoader.php index 161d302d..c1db86b5 100644 --- a/src/Domain/Messaging/Service/MessageDataLoader.php +++ b/src/Domain/Messaging/Service/MessageDataLoader.php @@ -11,10 +11,10 @@ use PhpList\Core\Domain\Messaging\Repository\MessageRepository; use Psr\Log\LoggerInterface; -// todo: check types class MessageDataLoader { private const AS_FORMAT_FIELDS = ['astext', 'ashtml', 'astextandhtml', 'aspdf', 'astextandpdf']; + private const LIST_FIELDS = ['targetlist', 'excludelist']; private const SCHEDULE_FIELDS = ['embargo', 'repeatuntil', 'requeueuntil']; private ?string $messageFromAddress; @@ -39,7 +39,6 @@ public function __invoke(Message $message): array $this->normaliseScheduleFields($messageData); $this->populateTargetLists($messageData, $message); $this->ensureSendUrlAndMethod($messageData); - // todo: check how should $message->getOptions()->getFromField() be used here $this->normaliseFromField($messageData, $defaultFrom); $this->cleanupSelections($messageData); $this->deriveCampaignTitle($messageData); @@ -72,12 +71,13 @@ private function buildDefaultMessageData(): array 'textmessage' => '', 'rsstemplate' => '', 'embargo' => $this->getDateArray(), - 'repeatinterval' => '0', + 'repeatinterval' => 0, 'repeatuntil' => $this->getDateArray(), - 'requeueinterval' => '0', + 'requeueinterval' => 0, 'requeueuntil' => $this->getDateArray(), 'finishsending' => $this->getDateArray($finishSending), 'fromfield' => '', + 'tofield' => '', 'subject' => '', 'forwardsubject' => '', 'footer' => $this->configProvider->getValue(ConfigOption::MessageFooter), @@ -95,9 +95,8 @@ private function buildDefaultMessageData(): array value: $this->configProvider->getValue(ConfigOption::AlwaysAddGoogleTracking), filter: FILTER_VALIDATE_BOOL ), - // todo: check where this is set 'excludelist' => [], - 'sentastest' => '0', + 'sentastest' => 0, ]; } @@ -121,19 +120,28 @@ private function mergeStoredMessageData(array &$messageData, Message $message): { $storedMessageData = $this->messageDataRepository->getForMessage($message->getId()); foreach ($storedMessageData as $storedMessageDatum) { - if (str_starts_with($storedMessageDatum->getData(), 'SER:')) { - $unserialized = unserialize(substr($storedMessageDatum->getData(), 4), ['allowed_classes' => false]); + $rawData = $storedMessageDatum->getData(); + if ($rawData === null && in_array($storedMessageDatum->getName(), self::LIST_FIELDS, true)) { + // keep the array defaults, a string here would break populateTargetLists + continue; + } + $rawData ??= ''; + if (str_starts_with($rawData, 'SER:')) { + $unserialized = unserialize(substr($rawData, 4), ['allowed_classes' => false]); if (!is_array($unserialized)) { $this->logger->warning('Invalid serialized data for message ID: ' . $message->getId()); continue; } array_walk_recursive($unserialized, function (&$val) { - $val = stripslashes($val); + // stored values can be ints/bools (e.g. ids stored through the API), only strings have slashes + if (is_string($val)) { + $val = stripslashes($val); + } }); $data = $unserialized; } else { - $data = stripslashes($storedMessageDatum->getData()); + $data = stripslashes($rawData); } if (!in_array($storedMessageDatum->getName(), self::AS_FORMAT_FIELDS)) { //# don't overwrite counters in the message table from the data table diff --git a/src/Domain/Messaging/Service/MessageForwardService.php b/src/Domain/Messaging/Service/MessageForwardService.php index b93f5d76..c27af684 100644 --- a/src/Domain/Messaging/Service/MessageForwardService.php +++ b/src/Domain/Messaging/Service/MessageForwardService.php @@ -78,7 +78,6 @@ public function forward(MessageForwardDto $messageForwardDto, Message $campaign) | EmailBlacklistedException | InvalidRecipientOrSubjectException $e ) { - // todo: check if need to catch MessageCacheMissingException $forwardingRecipientResult = $this->handleFailure( campaign: $campaign, forwardingSubscriber: $forwardingSubscriber, diff --git a/src/Domain/Messaging/Service/MessageProcessingPreparator.php b/src/Domain/Messaging/Service/MessageProcessingPreparator.php index 05f044ea..cfbcc148 100644 --- a/src/Domain/Messaging/Service/MessageProcessingPreparator.php +++ b/src/Domain/Messaging/Service/MessageProcessingPreparator.php @@ -4,7 +4,6 @@ namespace PhpList\Core\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; @@ -15,9 +14,6 @@ class MessageProcessingPreparator { - // @todo: create functionality to track - public const LINK_TRACK_ENDPOINT = '/api/v2/link-track'; - public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly MessageRepository $messageRepository, @@ -57,53 +53,23 @@ public function ensureCampaignsHaveUuid(OutputInterface $output): void } /** - * Process message content to extract URLs and replace them with link track URLs + * Replaces the links of the cached message content with click-tracking links for this subscriber. + * Returns the content unchanged when click tracking is off or the campaign is unknown. */ public function processMessageLinks( int $campaignId, MessagePrecacheDto $cachedMessageDto, Subscriber $subscriber ): MessagePrecacheDto { - if (!$this->linkTrackService->isExtractAndSaveLinksApplicable()) { + if (!$this->linkTrackService->isClickTrackingEnabled()) { return $cachedMessageDto; } - $savedLinks = $this->linkTrackService->extractAndSaveLinks( - content: $cachedMessageDto, - userId: $subscriber->getId(), - messageId: $campaignId, - ); - if (empty($savedLinks)) { + $campaign = $this->messageRepository->findById($campaignId); + if ($campaign === null) { return $cachedMessageDto; } - // todo: check if getTextMessage should replace links as well - if ($cachedMessageDto->content) { - $cachedMessageDto->content = $this->replaceLinks( - savedLinks: $savedLinks, - htmlText: $cachedMessageDto->content - ); - } - - if ($cachedMessageDto->htmlFooter) { - $cachedMessageDto->htmlFooter = $this->replaceLinks( - savedLinks: $savedLinks, - htmlText: $cachedMessageDto->htmlFooter - ); - } - - return $cachedMessageDto; - } - - private function replaceLinks(array $savedLinks, string $htmlText): string - { - /** @var LinkTrack $linkTrack */ - foreach ($savedLinks as $linkTrack) { - $originalUrl = $linkTrack->getUrl(); - $trackUrl = self::LINK_TRACK_ENDPOINT . '?id=' . $linkTrack->getId(); - $htmlText = str_replace('href="' . $originalUrl . '"', 'href="' . $trackUrl . '"', $htmlText); - } - - return $htmlText; + return $this->linkTrackService->trackLinks($cachedMessageDto, $campaign, $subscriber); } } diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index b55164c3..4ee046c2 100644 --- a/src/Domain/Subscription/Repository/SubscriberRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberRepository.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Common\Repository\Interfaces\PaginatableRepositoryInterface; use PhpList\Core\Domain\Subscription\Model\Filter\SubscriberFilter; use PhpList\Core\Domain\Subscription\Model\Subscriber; +use PhpList\Core\Domain\Subscription\Model\Subscription; /** * Repository for Subscriber models. @@ -272,6 +273,29 @@ public function findSubscriberWithSubscriptions(int $id): ?Subscriber ->getOneOrNullResult(); } + /** + * Subscribers of a list with the data needed for bounce reports, without any bounce counts. + * + * @return array + */ + public function getBounceReportRowsByListId(int $listId): array + { + return $this->createQueryBuilder('subscriber') + ->select( + 'subscriber.id AS subscriberId', + 'subscriber.email AS email', + 'subscriber.confirmed AS confirmed', + 'subscriber.blacklisted AS blacklisted' + ) + ->innerJoin(Subscription::class, 'subscription', 'ON', 'subscription.subscriber = subscriber') + ->where('IDENTITY(subscription.subscriberList) = :listId') + ->setParameter('listId', $listId) + ->groupBy('subscriber.id, subscriber.email, subscriber.confirmed, subscriber.blacklisted') + ->orderBy('subscriber.id', 'ASC') + ->getQuery() + ->getArrayResult(); + } + public function isEmailBlacklisted(string $email): bool { $queryBuilder = $this->getEntityManager()->createQueryBuilder(); diff --git a/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php b/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php new file mode 100644 index 00000000..c20d1a1f --- /dev/null +++ b/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php @@ -0,0 +1,130 @@ +loadSchema(); + + $this->forwardRepository = self::getContainer()->get(LinkTrackForwardRepository::class); + $this->mlRepository = self::getContainer()->get(LinkTrackMlRepository::class); + $this->umlClickRepository = self::getContainer()->get(LinkTrackUmlClickRepository::class); + } + + protected function tearDown(): void + { + $schemaTool = new SchemaTool($this->entityManager); + $schemaTool->dropDatabase(); + parent::tearDown(); + } + + public function testFindOrCreateByUrlCreatesOneForwardPerUrl(): void + { + $first = $this->forwardRepository->findOrCreateByUrl('https://example.com/a', false); + $again = $this->forwardRepository->findOrCreateByUrl('https://example.com/a', false); + $other = $this->forwardRepository->findOrCreateByUrl('https://example.com/b', true); + + self::assertSame($first->getId(), $again->getId()); + self::assertNotSame($first->getId(), $other->getId()); + self::assertSame('https://example.com/a', $first->getUrl()); + self::assertSame(md5('https://example.com/a'), $first->getUrlHash()); + self::assertFalse($first->isPersonalise()); + self::assertTrue($other->isPersonalise()); + self::assertSame(36, strlen((string) $first->getUuid())); + self::assertNotSame($first->getUuid(), $other->getUuid()); + self::assertSame($first->getId(), $this->forwardRepository->findOneByUuid($first->getUuid())->getId()); + self::assertNull($this->forwardRepository->findOneByUuid('unknown')); + } + + public function testIncrementTotalCreatesAndIncrementsTheRow(): void + { + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->incrementTotal(7, 12); + + self::assertSame(2, $this->findMl(7, 11)->getTotal()); + self::assertSame(1, $this->findMl(7, 12)->getTotal()); + self::assertSame(0, $this->findMl(7, 11)->getClicked()); + } + + public function testRecordClickOnMlKeepsTheFirstClickAndSplitsHtmlAndText(): void + { + $this->mlRepository->incrementTotal(7, 11); + $this->mlRepository->recordClick(7, 11, true, new DateTimeImmutable('2026-01-01 10:00:00')); + $this->mlRepository->recordClick(7, 11, false, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->mlRepository->recordClick(7, 11, true, new DateTimeImmutable('2026-01-03 12:00:00')); + + $ml = $this->findMl(7, 11); + self::assertSame(1, $ml->getTotal(), 'sending total is not touched by clicks'); + self::assertSame(3, $ml->getClicked()); + self::assertSame(2, $ml->getHtmlClicked()); + self::assertSame(1, $ml->getTextClicked()); + self::assertSame('2026-01-01 10:00:00', $ml->getFirstClick()->format('Y-m-d H:i:s')); + self::assertSame('2026-01-03 12:00:00', $ml->getLatestClick()->format('Y-m-d H:i:s')); + } + + public function testRecordClickOnMlCreatesTheRowWhenNothingWasSentYet(): void + { + $this->mlRepository->recordClick(7, 11, false, new DateTimeImmutable('2026-01-01 10:00:00')); + + $ml = $this->findMl(7, 11); + self::assertSame(1, $ml->getClicked()); + self::assertSame(1, $ml->getTextClicked()); + self::assertSame(0, $ml->getHtmlClicked()); + } + + public function testRecordClickPerSubscriberIsUniquePerCampaignSubscriberAndLink(): void + { + $this->umlClickRepository->recordClick(7, 3, 11, true, new DateTimeImmutable('2026-01-01 10:00:00')); + $this->umlClickRepository->recordClick(7, 3, 11, false, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->umlClickRepository->recordClick(7, 4, 11, true, new DateTimeImmutable('2026-01-02 11:00:00')); + $this->umlClickRepository->recordClick(7, 3, 12, true, new DateTimeImmutable('2026-01-02 11:00:00')); + + $click = $this->findUml(7, 3, 11); + self::assertSame(2, $click->getClicked()); + self::assertSame(1, $click->getHtmlClicked()); + self::assertSame(1, $click->getTextClicked()); + self::assertSame('2026-01-01 10:00:00', $click->getFirstClick()->format('Y-m-d H:i:s')); + self::assertSame('2026-01-02 11:00:00', $click->getLatestClick()->format('Y-m-d H:i:s')); + self::assertSame(1, $this->findUml(7, 4, 11)->getClicked()); + self::assertSame(1, $this->findUml(7, 3, 12)->getClicked()); + self::assertCount(3, $this->umlClickRepository->findAll()); + } + + private function findMl(int $messageId, int $forwardId): LinkTrackMl + { + $this->entityManager->clear(); + + return $this->mlRepository->findOneBy(['messageId' => $messageId, 'forwardId' => $forwardId]); + } + + private function findUml(int $messageId, int $userId, int $forwardId): LinkTrackUmlClick + { + $this->entityManager->clear(); + + return $this->umlClickRepository->findOneBy( + ['messageId' => $messageId, 'userId' => $userId, 'forwardId' => $forwardId] + ); + } +} diff --git a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php index a384457e..a496f42c 100644 --- a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php +++ b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php @@ -330,4 +330,25 @@ public function testGetFilteredAfterIdSortsDescendingAndCursorsBackward(): void self::assertCount(1, $secondPage->getItems()); self::assertSame($first->getId(), $secondPage->getItems()[0]->getId()); } + + public function testGetNonEmptyFieldsReturnsLegacyColumnNames(): void + { + $message = $this->persistMessage(Message\MessageStatus::Submitted, 'Ready to send'); + $this->entityManager->flush(); + $id = $message->getId(); + $this->entityManager->clear(); + + $fields = $this->messageRepository->getNonEmptyFields($id); + + self::assertSame('Ready to send', $fields['subject']); + self::assertSame('submitted', $fields['status']); + self::assertSame('text', $fields['sendformat']); + self::assertArrayNotHasKey('content.subject', $fields); + self::assertArrayNotHasKey('fromfield', $fields, 'empty columns are dropped'); + } + + public function testGetNonEmptyFieldsReturnsEmptyArrayForUnknownMessage(): void + { + self::assertSame([], $this->messageRepository->getNonEmptyFields(999999)); + } } diff --git a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php index 4459a4da..0a28f547 100644 --- a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php +++ b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php @@ -43,7 +43,14 @@ protected function setUp(): void $this->loadSchema(); $this->client = $this->createMock(ElasticsearchClientInterface::class); - $this->reader = new UserMessageBounceElasticsearchHybridReader($this->client, 'phplist_', $this->entityManager); + $this->reader = new UserMessageBounceElasticsearchHybridReader( + $this->client, + 'phplist_', + $this->entityManager->getRepository(Subscriber::class), + $this->entityManager->getRepository(Message::class), + $this->entityManager->getRepository(UserMessage::class), + $this->entityManager->getRepository(Bounce::class), + ); } protected function tearDown(): void diff --git a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php new file mode 100644 index 00000000..5b59986d --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -0,0 +1,223 @@ +forwardRepository = $this->createMock(LinkTrackForwardRepository::class); + $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); + $this->umlClickRepository = $this->createMock(LinkTrackUmlClickRepository::class); + $this->messageRepository = $this->createMock(MessageRepository::class); + $this->subscriberRepository = $this->createMock(SubscriberRepository::class); + $this->userMessageRepository = $this->createMock(UserMessageRepository::class); + $this->userMessageService = $this->createMock(UserMessageService::class); + $this->tokenService = new LinkTrackTokenService(new ParameterProvider([]), 'https://example.com/api/v2', 's'); + + $this->service = new LinkClickService( + $this->tokenService, + $this->forwardRepository, + $this->mlRepository, + $this->umlClickRepository, + $this->messageRepository, + $this->subscriberRepository, + $this->userMessageRepository, + $this->userMessageService, + ); + + $this->message = $this->createMock(Message::class); + $this->message->method('getId')->willReturn(7); + $this->subscriber = $this->createMock(Subscriber::class); + $this->subscriber->method('getId')->willReturn(3); + $this->subscriber->method('getUniqueId')->willReturn('uniq-3'); + } + + private function tid(string $type = 'H'): string + { + $url = $this->tokenService->createUrl(new LinkTrackToken($type, 'fwd', 'msg', 'usr')); + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + + return $query['tid']; + } + + private function givenKnownLink(string $url = 'https://example.com/a', bool $personalise = false): void + { + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getId')->willReturn(11); + $forward->method('getUrl')->willReturn($url); + $forward->method('isPersonalise')->willReturn($personalise); + + $this->forwardRepository->method('findOneByUuid')->with('fwd')->willReturn($forward); + $this->messageRepository->method('findOneBy')->with(['uuid' => 'msg'])->willReturn($this->message); + $this->subscriberRepository->method('findOneBy')->with(['uuid' => 'usr'])->willReturn($this->subscriber); + } + + private function givenUserMessage(bool $viewed): void + { + $userMessage = $this->createMock(UserMessage::class); + $userMessage->method('isViewed')->willReturn($viewed); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn($userMessage); + } + + public function testRedirectsToTheStoredUrlAndCountsTheClick(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: false); + + $this->mlRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 11, true, self::anything()); + $this->umlClickRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 3, 11, true, self::anything()); + $this->userMessageService->expects(self::once()) + ->method('trackUserMessageView') + ->with('uniq-3', 7, ['HTTP_USER_AGENT' => 'ua']); + + self::assertSame( + 'https://example.com/a', + $this->service->trackClick($this->tid(), null, ['HTTP_USER_AGENT' => 'ua']) + ); + } + + public function testTextClicksAreCountedAsText(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $this->mlRepository->expects(self::once())->method('recordClick')->with(7, 11, false, self::anything()); + $this->umlClickRepository->expects(self::once()) + ->method('recordClick') + ->with(7, 3, 11, false, self::anything()); + + self::assertNotNull($this->service->trackClick($this->tid('T'), null, [])); + } + + public function testOnlyTheFirstClickCountsAsView(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $this->userMessageService->expects(self::never())->method('trackUserMessageView'); + + self::assertNotNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testPersonalisedLinkGetsTheSubscribersUid(): void + { + $this->givenKnownLink('https://example.com/p?a=1', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame('https://example.com/p?a=1&uid=uniq-3', $this->service->trackClick($this->tid(), null, [])); + } + + public function testPersonalisedLinkPutsTheUidBeforeTheFragment(): void + { + $this->givenKnownLink('https://example.com/p?a=1#top', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame( + 'https://example.com/p?a=1&uid=uniq-3#top', + $this->service->trackClick($this->tid(), null, []) + ); + } + + public function testPersonalisedLinkWithOnlyAFragmentStartsTheQueryBeforeIt(): void + { + $this->givenKnownLink('https://example.com/p#a?b', personalise: true); + $this->givenUserMessage(viewed: true); + + self::assertSame( + 'https://example.com/p?uid=uniq-3#a?b', + $this->service->trackClick($this->tid(), null, []) + ); + } + + public function testPersonalisedLinkIsRefusedForSubscribersThatNeverGotTheCampaign(): void + { + $this->givenKnownLink('https://example.com/p', personalise: true); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testNonPersonalisedLinkWorksWithoutUserMessage(): void + { + $this->givenKnownLink(); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->userMessageService->expects(self::never())->method('trackUserMessageView'); + + self::assertSame('https://example.com/a', $this->service->trackClick($this->tid(), null, [])); + } + + public function testInvalidTokenIsRejected(): void + { + $this->forwardRepository->expects(self::never())->method('findOneByUuid'); + + self::assertNull($this->service->trackClick('garbage', null, [])); + } + + public function testUnknownForwardIsRejected(): void + { + $this->forwardRepository->method('findOneByUuid')->willReturn(null); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testUnknownSubscriberIsRejected(): void + { + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getUrl')->willReturn('https://example.com/a'); + $this->forwardRepository->method('findOneByUuid')->willReturn($forward); + $this->messageRepository->method('findOneBy')->willReturn($this->message); + $this->subscriberRepository->method('findOneBy')->willReturn(null); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } + + public function testNeverRedirectsToNonWebUrls(): void + { + $this->givenKnownLink('javascript:alert(1)'); + $this->mlRepository->expects(self::never())->method('recordClick'); + + self::assertNull($this->service->trackClick($this->tid(), null, [])); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php index c7495f99..7457c0bb 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -4,257 +4,243 @@ namespace PhpList\Core\Tests\Unit\Domain\Analytics\Service; -use Doctrine\ORM\EntityManagerInterface; use PhpList\Core\Core\ParameterProvider; -use PhpList\Core\Domain\Analytics\Exception\MissingMessageIdException; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; +use PhpList\Core\Domain\Analytics\Model\LinkTrackForward; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackForwardRepository; +use PhpList\Core\Domain\Analytics\Repository\LinkTrackMlRepository; use PhpList\Core\Domain\Analytics\Repository\LinkTrackRepository; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; +use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Model\Message; -use PhpList\Core\Domain\Messaging\Model\Message\MessageContent; +use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; class LinkTrackServiceTest extends TestCase { - private LinkTrackService $subject; - private LinkTrackRepository|MockObject $linkTrackRepository; + private LinkTrackForwardRepository&MockObject $forwardRepository; + private LinkTrackMlRepository&MockObject $mlRepository; + private LinkTrackTokenService $tokenService; + private Message&MockObject $campaign; + private Subscriber $subscriber; + /** @var array */ + private array $createdForwards = []; protected function setUp(): void { - $this->linkTrackRepository = $this->createMock(LinkTrackRepository::class); - $paramProvider = $this->createMock(ParameterProvider::class); + $this->forwardRepository = $this->createMock(LinkTrackForwardRepository::class); + $this->mlRepository = $this->createMock(LinkTrackMlRepository::class); + $this->tokenService = new LinkTrackTokenService( + new ParameterProvider([]), + 'https://example.com/api/v2', + 'secret' + ); + + $this->campaign = $this->createMock(Message::class); + $this->campaign->method('getId')->willReturn(7); + $this->campaign->method('getUuid')->willReturn('campaign-uuid'); + + $this->subscriber = new Subscriber('alice@example.com'); + $this->subscriber->setUuid('subscriber-uuid'); - $paramProvider->method('get') - ->with('click_track', false) - ->willReturn(true); + $this->forwardRepository->method('findOrCreateByUrl')->willReturnCallback( + function (string $url, bool $personalise): LinkTrackForward { + $this->createdForwards[$url] = ['url' => $url, 'personalise' => $personalise]; + $id = array_search($url, array_keys($this->createdForwards), true) + 1; - $this->subject = new LinkTrackService( - linkTrackRepository: $this->linkTrackRepository, - paramProvider: $paramProvider, - entityManager: $this->createMock(EntityManagerInterface::class) + return $this->createForward($id, $url); + } ); } - public function testExtractAndSaveLinksWithNoLinks(): void + private function createForward(int $id, string $url): LinkTrackForward&MockObject { - $messageId = 123; - $userId = 456; + $forward = $this->createMock(LinkTrackForward::class); + $forward->method('getId')->willReturn($id); + $forward->method('getUuid')->willReturn('forward-uuid-' . $id); + $forward->method('getUrl')->willReturn($url); - $messageContent = new MessagePrecacheDto(); - $messageContent->subject = 'Test Subject'; - $messageContent->content = 'No links here'; + return $forward; + } - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn(new MessageContent('Test Subject', 'No links here')); + private function createService(bool $clickTrack = true): LinkTrackService + { + return new LinkTrackService( + $this->createMock(LinkTrackRepository::class), + $this->forwardRepository, + $this->mlRepository, + $this->tokenService, + new ParameterProvider(['click_track' => $clickTrack]), + ); + } - $this->linkTrackRepository->expects(self::never())->method('persist'); + /** @return string[] the uuids found in all tracking links of the text, in order */ + private function trackedForwardUuids(string $text): array + { + preg_match_all('~/lt\?tid=([A-Za-z0-9_-]+)~', $text, $matches); - $result = $this->subject->extractAndSaveLinks($messageContent, $userId, $messageId); + return array_map( + fn (string $tid): string => $this->tokenService->parse($tid)->forwardUuid, + $matches[1] + ); + } - self::assertEmpty($result); + public function testIsClickTrackingEnabledFollowsTheParameter(): void + { + self::assertTrue($this->createService(true)->isClickTrackingEnabled()); + self::assertFalse($this->createService(false)->isClickTrackingEnabled()); } - public function testExtractAndSaveLinksWithLinks(): void + public function testContentIsReturnedUntouchedWhenClickTrackingIsDisabled(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Check out this link and ' - . 'this one.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::exactly(2)) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertContains($linkTrack->getUrl(), ['https://example.com', 'https://test.com']); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = 'x'; + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService(false)->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://test.com', $result[1]->getUrl()); + self::assertSame($content, $result); } - public function testExtractAndSaveLinksWithFooter(): void + public function testContentIsReturnedUntouchedWhenCampaignHasNoUuid(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Main content with a link.

'; - $footerContent = '

Footer with another link.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent, null, $footerContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - $messagePrecacheDto->htmlFooter = $footerContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::exactly(2)) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertContains($linkTrack->getUrl(), ['https://example.com', 'https://footer.com']); - return null; - }); - - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $campaign = $this->createMock(Message::class); + $campaign->method('getUuid')->willReturn(null); + $content = new MessagePrecacheDto(); + $content->content = 'x'; - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://footer.com', $result[1]->getUrl()); + self::assertSame($content, $this->createService()->trackLinks($content, $campaign, $this->subscriber)); } - public function testExtractAndSaveLinksWithDuplicateLinks(): void + public function testHtmlLinksAreReplacedWithTrackingLinks(): void { - $messageId = 123; - $userId = 456; - $htmlContent = '

Link 1 and Link 2.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::once()) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertSame('https://example.com', $linkTrack->getUrl()); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = '

A ' + . "

"; + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + self::assertStringContainsString('A', $result->content); + self::assertStringContainsString("content); + self::assertArrayHasKey('https://example.com/a?x=1&y=2', $this->createdForwards, 'stored url is decoded'); + self::assertArrayHasKey('http://example.org/b', $this->createdForwards); + } - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + public function testTrackingLinkIdentifiesForwardCampaignAndSubscriber(): void + { + $content = new MessagePrecacheDto(); + $content->content = 'A'; + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + preg_match('~tid=([A-Za-z0-9_-]+)~', $result->content, $match); + $token = $this->tokenService->parse($match[1]); + self::assertNotNull($token); + self::assertTrue($token->isHtml()); + self::assertSame('forward-uuid-1', $token->forwardUuid); + self::assertSame('campaign-uuid', $token->messageUuid); + self::assertSame('subscriber-uuid', $token->subscriberUuid); + } - self::assertCount(1, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); + public function testLinksThatMustNotBeTrackedAreLeftAlone(): void + { + $html = implode(' ', [ + 'mail', + 'unsubscribe', + 'placeholder', + 'script', + 'https://example.com/other', + 'too long', + 'already tracked', + 'no href', + ]); + $content = new MessagePrecacheDto(); + $content->content = $html; + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + $this->mlRepository->expects(self::never())->method('incrementTotal'); + + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); + + self::assertSame($html, $result->content); } - public function testExtractAndSaveLinksWithNullText(): void + public function testPersonalUidAndSessionParametersAreStrippedAndMarkTheForwardPersonal(): void { - $messageId = 123; - $userId = 456; - $footerContent = '

Footer with a link.

'; - - $messageContent = new MessageContent('Test Subject', null, null, $footerContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->htmlFooter = $footerContent; - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - - $this->linkTrackRepository->expects(self::once()) - ->method('persist') - ->willReturnCallback(function (LinkTrack $linkTrack) use ($messageId, $userId) { - self::assertSame($messageId, $linkTrack->getMessageId()); - self::assertSame($userId, $linkTrack->getUserId()); - self::assertSame('https://footer.com', $linkTrack->getUrl()); - return null; - }); + $content = new MessagePrecacheDto(); + $content->content = 'P'; - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(1, $result); - self::assertSame('https://footer.com', $result[0]->getUrl()); + self::assertSame( + ['https://example.com/p?a=1#top' => ['url' => 'https://example.com/p?a=1#top', 'personalise' => true]], + $this->createdForwards + ); } - public function testExtractAndSaveLinksWithMessageWithoutId(): void + public function testTextUrlsAreReplacedWithTextTokens(): void { - $userId = 456; - $htmlContent = '

Link

'; + $content = new MessagePrecacheDto(); + $content->textContent = "Read https://example.com/a. Or see http://example.org/b?x=1\nbye"; + $content->textFooter = 'More: https://example.com/a'; - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn(null); - $message->method('getContent')->willReturn($messageContent); - - $this->expectException(MissingMessageIdException::class); - $this->expectExceptionMessage('Message must have an ID'); - - $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $message->getId()); + self::assertMatchesRegularExpression( + '~^Read https://example.com/api/v2/lt\?tid=[A-Za-z0-9_-]+\. Or see https://example.com/api/v2/lt\?tid=~', + $result->textContent + ); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result->textContent, $match); + self::assertSame('T', $this->tokenService->parse($match[1])->type); + self::assertSame( + [$this->trackedForwardUuids($result->textContent)[0]], + $this->trackedForwardUuids((string) $result->textFooter), + 'the same url gets the same forward' + ); + self::assertSame("\nbye", substr($result->textContent, -4)); } - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsTrue(): void + public function testFootersAreTrackedToo(): void { - self::assertTrue($this->subject->isExtractAndSaveLinksApplicable()); - } + $content = new MessagePrecacheDto(); + $content->htmlFooter = 'Footer'; - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsFalse(): void - { - $paramProvider = $this->createMock(ParameterProvider::class); - $paramProvider->method('get') - ->with('click_track', false) - ->willReturn(false); - - $subject = new LinkTrackService( - linkTrackRepository: $this->linkTrackRepository, - paramProvider: $paramProvider, - entityManager: $this->createMock(EntityManagerInterface::class) - ); + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertFalse($subject->isExtractAndSaveLinksApplicable()); + self::assertSame(['forward-uuid-1'], $this->trackedForwardUuids($result->htmlFooter)); } - public function testExtractAndSaveLinksWithExistingLink(): void + public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): void { - $messageId = 123; - $userId = 456; - $url = 'https://example.com'; - $htmlContent = '

Check out this link.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); - - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn($messageId); - $message->method('getContent')->willReturn($messageContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; + $content = new MessagePrecacheDto(); + $content->content = '12' + . '3'; + $content->textContent = 'https://example.com/a'; + + $incremented = []; + $this->mlRepository->expects(self::exactly(2)) + ->method('incrementTotal') + ->willReturnCallback(function (int $messageId, int $forwardId) use (&$incremented): void { + $incremented[] = [$messageId, $forwardId]; + }); - $existingLinkTrack = new LinkTrack(); - $existingLinkTrack->setMessageId($messageId); - $existingLinkTrack->setUserId($userId); - $existingLinkTrack->setUrl($url); + $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - $this->linkTrackRepository->expects(self::once()) - ->method('findByUrlUserIdAndMessageId') - ->with($url, $userId, $messageId) - ->willReturn($existingLinkTrack); + self::assertSame([[7, 1], [7, 2]], $incremented); + } - $this->linkTrackRepository->expects(self::never()) - ->method('persist'); + public function testTheGivenContentIsNotModified(): void + { + $content = new MessagePrecacheDto(); + $content->content = 'A'; - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService()->trackLinks($content, $this->campaign, $this->subscriber); - self::assertCount(1, $result); - self::assertSame($existingLinkTrack, $result[0]); + self::assertNotSame($content, $result); + self::assertSame('A', $content->content); } } diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php new file mode 100644 index 00000000..e97f1fc5 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php @@ -0,0 +1,95 @@ + $sign]), + 'https://example.com/api/v2/', + $secret + ); + } + + /** @return array */ + private function queryOf(string $url): array + { + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + + return $query; + } + + public function testCreatesAbsoluteUrlToTheLinkTrackEndpoint(): void + { + $url = $this->createService()->createUrl(new LinkTrackToken('H', 'fwd-1', 'msg-2', 'usr-3')); + + self::assertStringStartsWith('https://example.com/api/v2/lt?tid=', $url); + self::assertSame(['tid'], array_keys($this->queryOf($url))); + } + + public function testParsesWhatItCreated(): void + { + $service = $this->createService(); + $url = $service->createUrl(new LinkTrackToken('T', 'fwd-1', 'msg-2', 'usr-3')); + + $token = $service->parse($this->queryOf($url)['tid']); + + self::assertNotNull($token); + self::assertSame('T', $token->type); + self::assertFalse($token->isHtml()); + self::assertSame('fwd-1', $token->forwardUuid); + self::assertSame('msg-2', $token->messageUuid); + self::assertSame('usr-3', $token->subscriberUuid); + } + + /** @dataProvider malformedTokenProvider */ + public function testRejectsMalformedTokens(string $tid): void + { + self::assertNull($this->createService()->parse($tid)); + } + + public static function malformedTokenProvider(): array + { + $encode = static fn (string $value): string => rtrim(strtr(base64_encode($value), '+/', '-_'), '='); + + return [ + 'not base64' => ['!!!'], + 'too few parts' => [$encode('H|a|b')], + 'too many parts' => [$encode('H|a|b|c|d')], + 'unknown type' => [$encode('X|a|b|c')], + 'illegal characters' => [$encode("H|a|b|c'; drop table")], + 'empty uuid' => [$encode('H||b|c')], + ]; + } + + public function testSignedUrlsAreVerified(): void + { + $service = $this->createService(sign: true); + $query = $this->queryOf($service->createUrl(new LinkTrackToken('H', 'f', 'm', 'u'))); + + self::assertArrayHasKey('hm', $query); + self::assertNotNull($service->parse($query['tid'], $query['hm'])); + self::assertNull($service->parse($query['tid']), 'missing signature'); + self::assertNull($service->parse($query['tid'], 'wrong'), 'wrong signature'); + self::assertNull( + $this->createService(sign: true, secret: 'other')->parse($query['tid'], $query['hm']), + 'signature of another secret' + ); + } + + public function testUnsignedUrlsHaveNoSignature(): void + { + $query = $this->queryOf($this->createService()->createUrl(new LinkTrackToken('H', 'f', 'm', 'u'))); + + self::assertArrayNotHasKey('hm', $query); + } +} diff --git a/tests/Unit/Domain/Common/FileHelperTest.php b/tests/Unit/Domain/Common/FileHelperTest.php index 9f652623..f9f231d9 100644 --- a/tests/Unit/Domain/Common/FileHelperTest.php +++ b/tests/Unit/Domain/Common/FileHelperTest.php @@ -14,7 +14,7 @@ class FileHelperTest extends TestCase protected function setUp(): void { $this->tmpDir = sys_get_temp_dir() . '/phplist-filehelper-' . bin2hex(random_bytes(6)); - mkdir($this->tmpDir, 0777, true); + mkdir($this->tmpDir, 0700, true); } protected function tearDown(): void @@ -74,6 +74,23 @@ public function testWriteFileToDirectoryCreatesFileWithExtensionAndContents(): v $this->assertSame('payload', file_get_contents($writtenPath)); } + public function testWriteFileToDirectoryLeavesNoOrphanedTempFile(): void + { + $helper = new FileHelper(); + + $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + + $this->assertSame([$writtenPath], glob($this->tmpDir . '/*')); + } + + public function testWriteFileToDirectoryReturnsNullWhenDirectoryIsMissing(): void + { + $helper = new FileHelper(); + + $this->assertNull($helper->writeFileToDirectory($this->tmpDir . '/missing', 'report.txt', 'payload')); + $this->assertSame([], glob($this->tmpDir . '/*')); + } + public function testWriteFileToDirectoryUsesDefaultNameWhenMissing(): void { $helper = new FileHelper(); diff --git a/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php b/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php index 28891f78..65bcc57e 100644 --- a/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php +++ b/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php @@ -11,7 +11,6 @@ use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Service\ForwardContentService; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; @@ -22,13 +21,11 @@ class ForwardContentServiceTest extends TestCase { private CacheInterface&MockObject $cache; - private MessageProcessingPreparator&MockObject $preparator; private ForwardEmailBuilder&MockObject $builder; protected function setUp(): void { $this->cache = $this->createMock(CacheInterface::class); - $this->preparator = $this->createMock(MessageProcessingPreparator::class); $this->builder = $this->createMock(ForwardEmailBuilder::class); } @@ -36,7 +33,6 @@ public function testThrowsWhenCacheMissing(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, ); @@ -66,11 +62,10 @@ public function testThrowsWhenCacheMissing(): void ); } - public function testProcessesLinksAndDelegatesToBuilder(): void + public function testDelegatesCachedContentToBuilderWithoutClickTracking(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, ); @@ -80,7 +75,6 @@ public function testProcessesLinksAndDelegatesToBuilder(): void $subscriber->setHtmlEmail(true); $cached = new MessagePrecacheDto(); - $processed = new MessagePrecacheDto(); $this->cache ->expects(self::once()) @@ -88,16 +82,6 @@ public function testProcessesLinksAndDelegatesToBuilder(): void ->with('messaging.message.base.42.1.0') ->willReturn($cached); - $this->preparator - ->expects(self::once()) - ->method('processMessageLinks') - ->with( - 42, - $cached, - $subscriber - ) - ->willReturn($processed); - $expectedEmail = new Email(); $this->builder ->expects(self::once()) @@ -106,7 +90,7 @@ public function testProcessesLinksAndDelegatesToBuilder(): void 42, 'f@example.com', $subscriber, - $processed, + $cached, true, 'From Name', 'from@example.com', diff --git a/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php index 296e0c1d..4d7fb047 100644 --- a/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php +++ b/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php @@ -95,6 +95,58 @@ public function testCreateImagesFromImagePathsStoresImageDataAndReplacesExisting $this->assertSame(self::ONE_PIXEL_PNG, $capturedImage->getData()); } + public function testCreateCachedLogoImageStoresResizedLogoAsPng(): void + { + $capturedImage = $this->createCachedLogoImageFromJpeg(width: 100, height: 50, size: 20); + + $this->assertSame('ORGANISATIONLOGO20.png', $capturedImage->getFilename()); + $this->assertSame('image/png', $capturedImage->getMimeType()); + $this->assertSame(20, $capturedImage->getWidth()); + $this->assertSame(10, $capturedImage->getHeight()); + } + + public function testCreateCachedLogoImageKeepsOriginalMimeTypeWhenNotResized(): void + { + $capturedImage = $this->createCachedLogoImageFromJpeg(width: 10, height: 10, size: 500); + + $this->assertSame('image/jpeg', $capturedImage->getMimeType()); + } + + private function createCachedLogoImageFromJpeg(int $width, int $height, int $size): TemplateImage + { + $canvas = imagecreatetruecolor($width, $height); + ob_start(); + imagejpeg($canvas); + $jpeg = (string) ob_get_clean(); + + $logoImage = $this->createMock(TemplateImage::class); + $logoImage->method('getData')->willReturn(base64_encode($jpeg)); + + $this->configProvider->method('getValue')->willReturn('1'); + $this->templateImageRepository->method('findByFilename')->willReturn(null); + $this->templateImageRepository->method('findById')->with(1)->willReturn($logoImage); + + $capturedImage = null; + $this->templateImageRepository->expects($this->once()) + ->method('persist') + ->with($this->callback(static function (TemplateImage $image) use (&$capturedImage): bool { + $capturedImage = $image; + return true; + })); + + // resizing closes the buffer that was active when it started, so provide one to keep PHPUnit's intact + $obLevel = ob_get_level(); + ob_start(); + $this->manager->createCachedLogoImage($size); + while (ob_get_level() > $obLevel) { + ob_end_clean(); + } + + $this->assertInstanceOf(TemplateImage::class, $capturedImage); + + return $capturedImage; + } + public function testGuessMimeType(): void { $reflection = new \ReflectionClass($this->manager); diff --git a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php index b3d945ae..b1446640 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php @@ -168,6 +168,72 @@ public function testParsesFromFieldIntoNameAndEmail(string $fromField, string $n $this->assertSame($email, $result['fromemail']); } + public function testStoredSerializedDataWithNonStringValuesAndNullDataDoNotBreakLoading(): void + { + $this->config->method('getValue')->willReturnMap([ + [ConfigOption::MessageFromAddress, 'from@example.com'], + ]); + $this->messageRepository->method('getNonEmptyFields')->willReturn([]); + + $excludeList = (new MessageData()) + ->setId(10) + ->setName('excludelist') + ->setData('SER:' . serialize([3, '4', true])); + $nullData = (new MessageData())->setId(10)->setName('footer')->setData(null); + $this->messageDataRepository->method('getForMessage')->willReturn([$excludeList, $nullData]); + + $message = $this->createMock(Message::class); + $message->method('getId')->willReturn(10); + $message->method('getListMessages')->willReturn(new ArrayCollection()); + + $loader = new MessageDataLoader( + configProvider: $this->config, + messageDataRepository: $this->messageDataRepository, + messageRepository: $this->messageRepository, + logger: $this->createMock(LoggerInterface::class), + defaultMessageAge: 3600 + ); + + $result = ($loader)($message); + + $this->assertSame([3, '4', true], $result['excludelist']); + $this->assertSame('', $result['footer']); + } + + public function testNullTargetAndExcludeListDataKeepArrayDefaults(): void + { + $this->config->method('getValue')->willReturnMap([ + [ConfigOption::MessageFromAddress, 'from@example.com'], + ]); + $this->messageRepository->method('getNonEmptyFields')->willReturn([]); + + $targetList = (new MessageData())->setId(11)->setName('targetlist')->setData(null); + $excludeList = (new MessageData())->setId(11)->setName('excludelist')->setData(null); + $this->messageDataRepository->method('getForMessage')->willReturn([$targetList, $excludeList]); + + $listMock = $this->createMock(SubscriberList::class); + $listMock->method('getId')->willReturn(42); + $listMessageMock = $this->createMock(ListMessage::class); + $listMessageMock->method('getList')->willReturn($listMock); + + $message = $this->createMock(Message::class); + $message->method('getId')->willReturn(11); + $message->method('getListMessages')->willReturn(new ArrayCollection([$listMessageMock])); + + $loader = new MessageDataLoader( + configProvider: $this->config, + messageDataRepository: $this->messageDataRepository, + messageRepository: $this->messageRepository, + logger: $this->createMock(LoggerInterface::class), + defaultMessageAge: 3600 + ); + + $result = ($loader)($message); + + $this->assertSame([42 => 1], $result['targetlist']); + $this->assertSame([], $result['excludelist']); + } + public static function fromFieldProvider(): array { return [ diff --git a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php index 4cd2e800..08ffa892 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php @@ -4,7 +4,6 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service; -use PhpList\Core\Domain\Analytics\Model\LinkTrack; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Repository\MessageRepository; @@ -116,87 +115,52 @@ public function testEnsureCampaignsHaveUuidWithCampaigns(): void $this->preparator->ensureCampaignsHaveUuid($this->output); } - public function testProcessMessageLinksWhenLinkTrackingNotApplicable(): void + public function testProcessMessageLinksWhenClickTrackingIsDisabled(): void { $messageContent = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); $this->linkTrackService->expects($this->once()) - ->method('isExtractAndSaveLinksApplicable') + ->method('isClickTrackingEnabled') ->willReturn(false); - - $this->linkTrackService->expects($this->never()) - ->method('extractAndSaveLinks'); + $this->messageRepository->expects($this->never())->method('findById'); + $this->linkTrackService->expects($this->never())->method('trackLinks'); $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); $this->assertSame($messageContent, $result); } - public function testProcessMessageLinksWhenNoLinksExtracted(): void + public function testProcessMessageLinksWhenCampaignIsUnknown(): void { - $messageId = 1; $messageContent = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); - - $this->linkTrackService->expects($this->once()) - ->method('isExtractAndSaveLinksApplicable') - ->willReturn(true); - $this->linkTrackService->expects($this->once()) - ->method('extractAndSaveLinks') - ->with($messageContent, 123, $messageId) - ->willReturn([]); + $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); + $this->messageRepository->expects($this->once())->method('findById')->with(1)->willReturn(null); + $this->linkTrackService->expects($this->never())->method('trackLinks'); - $result = $this->preparator->processMessageLinks($messageId, $messageContent, $subscriber); + $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); $this->assertSame($messageContent, $result); } - public function testProcessMessageLinksWithLinksExtracted(): void + public function testProcessMessageLinksReturnsTheContentWithTrackedLinks(): void { - $content = new MessagePrecacheDto(); + $messageContent = new MessagePrecacheDto(); + $tracked = new MessagePrecacheDto(); $subscriber = $this->createMock(Subscriber::class); - $subscriber->method('getId')->willReturn(123); - $subscriber->method('getEmail')->willReturn('test@example.com'); - - $linkTrack1 = $this->createMock(LinkTrack::class); - $linkTrack1->method('getId')->willReturn(1); - $linkTrack1->method('getUrl')->willReturn('https://example.com'); + $campaign = $this->createMock(Message::class); - $linkTrack2 = $this->createMock(LinkTrack::class); - $linkTrack2->method('getId')->willReturn(2); - $linkTrack2->method('getUrl')->willReturn('https://example.org'); - - $savedLinks = [$linkTrack1, $linkTrack2]; - - $this->linkTrackService->method('isExtractAndSaveLinksApplicable')->willReturn(true); - $this->linkTrackService - ->method('extractAndSaveLinks') - ->with($content, 123, 1) - ->willReturn($savedLinks); - - $content->content = 'Link 1 Link 2'; - $content->htmlFooter = 'Footer Link'; + $this->linkTrackService->method('isClickTrackingEnabled')->willReturn(true); + $this->messageRepository->method('findById')->with(1)->willReturn($campaign); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with($messageContent, $campaign, $subscriber) + ->willReturn($tracked); - $result = $this->preparator->processMessageLinks(1, $content, $subscriber); + $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - $this->assertSame($content, $result); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=1', - $content->content - ); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=2', - $content->content - ); - $this->assertStringContainsString( - MessageProcessingPreparator::LINK_TRACK_ENDPOINT . '?id=1', - $content->htmlFooter - ); + $this->assertSame($tracked, $result); } }