diff --git a/.env.dist b/.env.dist index 07a41b6a..80161df9 100644 --- a/.env.dist +++ b/.env.dist @@ -24,6 +24,9 @@ APP_POWERED_BY_PHPLIST=0 PREFERENCEPAGE_SHOW_PRIVATE_LISTS=0 API_BASE_URL=http://api.phplist.local/ +# Website address without scheme (e.g. www.example.com). Used as the default of the "website" setting; +# the "domain" setting defaults to the same value without a leading "www.". Both are editable in the admin UI. +APP_WEBSITE=phplist.local FRONT_END_BASE_URL=http://frontend.phplist.local PARALLEL_USE_WITH_PHPLIST3=0 diff --git a/config/parameters.yml b/config/parameters.yml index 28d4ed9c..aa00f865 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -23,6 +23,9 @@ parameters: app.rest_api_base_url: '%env(API_BASE_URL)%/api/v2' app.api_base_url: '%env(API_BASE_URL)%' app.frontend_base_url: '%env(FRONT_END_BASE_URL)%' + # falls back to empty so existing .env files without APP_WEBSITE keep working (the value may live in the database) + env(APP_WEBSITE): '' + app.website: '%env(APP_WEBSITE)%' app.parallel_use_with_phplist3: '%env(PARALLEL_USE_WITH_PHPLIST3)%' 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/providers.yml b/config/services/providers.yml index 481b23a3..830abb14 100644 --- a/config/services/providers.yml +++ b/config/services/providers.yml @@ -16,6 +16,8 @@ services: PhpList\Core\Domain\Configuration\Service\Provider\DefaultConfigProvider: autowire: true + arguments: + $website: '%app.website%' PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider: autowire: true 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..bf40c389 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -61,6 +61,21 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Analytics\Service\LinkRewriter: + autowire: true + + PhpList\Core\Domain\Analytics\Service\AnalyticsTrackingService: + autowire: 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/AnalyticsTrackingService.php b/src/Domain/Analytics/Service/AnalyticsTrackingService.php new file mode 100644 index 00000000..a9e19ecf --- /dev/null +++ b/src/Domain/Analytics/Service/AnalyticsTrackingService.php @@ -0,0 +1,150 @@ +messageDataLoader)($campaign); + if (empty($messageData['google_track'])) { + return $url; + } + + return $this->addToUrl($url, $isHtml ? self::FORMAT_HTML : self::FORMAT_TEXT, $messageData); + } + + /** + * For a message that is sent without click tracking: adds the parameters to all links. + * + * @param array $messageData the campaign's analytics settings, see MessagePrecacheDto + * @return array{0: string, 1: string} the HTML and the text + */ + public function addToBodies(string $html, string $text, array $messageData): array + { + $html = $this->linkRewriter->rewriteHtml( + $html, + fn (string $url): string => $this->addToUrl($url, self::FORMAT_HTML, $messageData) + ); + $text = $this->linkRewriter->rewriteText( + $text, + fn (string $url): string => $this->addToUrl($url, self::FORMAT_TEXT, $messageData) + ); + + return [$html, $text]; + } + + /** + * Only http and https urls get parameters. + * + * @param array $messageData + */ + public function addToUrl(string $url, string $format, array $messageData): string + { + if (preg_match('/^https?:/i', $url) !== 1) { + return $url; + } + + $query = $this->getQuery(); + // a value the campaign never set gets its default, as phplist3's form would have saved it; a value that + // was set to empty stays empty, which leaves that parameter out + foreach ($query->editableParameters($messageData) as $name => $default) { + if (!array_key_exists($name, $messageData)) { + $messageData[$name] = $default; + } + } + + return $this->withParameters( + $url, + $query->trackingParameters($format, $messageData), + $query->prefix() + ); + } + + /** + * Adds the parameters to the url, taking off tracking parameters that are already on it. Empty parameters are + * not added. A fragment stays at the end. + * + * @param array $trackingParameters + */ + public function withParameters(string $url, array $trackingParameters, string $prefix): string + { + $fragment = ''; + $position = strpos($url, '#'); + if ($position !== false) { + $fragment = substr($url, $position); + $url = substr($url, 0, $position); + } + + $url = $this->withoutPrefixedParameters($url, $prefix); + + $trackingCode = http_build_query( + array_filter($trackingParameters, static fn ($value): bool => $value !== null && $value !== '') + ); + if ($trackingCode === '') { + return $url . $fragment; + } + + return $url . (str_contains($url, '?') ? '&' : '?') . $trackingCode . $fragment; + } + + /** + * Takes off the query parameters whose name starts with the prefix, leaving the path and the other parameters + * as they are. The url is left untouched when nothing is removed. + */ + private function withoutPrefixedParameters(string $url, string $prefix): string + { + $queryPosition = strpos($url, '?'); + if ($prefix === '' || $queryPosition === false) { + return $url; + } + + $base = substr($url, 0, $queryPosition); + $pairs = explode('&', substr($url, $queryPosition + 1)); + $kept = array_filter( + $pairs, + static fn (string $pair): bool => !str_starts_with(urldecode(explode('=', $pair, 2)[0]), $prefix) + ); + if (count($kept) === count($pairs)) { + return $url; + } + + $kept = array_filter($kept, static fn (string $pair): bool => $pair !== ''); + + return $kept === [] ? $base : $base . '?' . implode('&', $kept); + } + + private function getQuery(): AnalyticsQueryInterface + { + return $this->configProvider->getValue(ConfigOption::AnalyticTracker) === 'matomo' + ? new MatomoAnalyticsQuery() + : new GoogleAnalyticsQuery(); + } +} diff --git a/src/Domain/Analytics/Service/LinkClickService.php b/src/Domain/Analytics/Service/LinkClickService.php new file mode 100644 index 00000000..2eb48dc0 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -0,0 +1,109 @@ + $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 $this->analyticsTrackingService->addToClickedUrl($url, $message, $token->isHtml()); + } + + 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/LinkRewriter.php b/src/Domain/Analytics/Service/LinkRewriter.php new file mode 100644 index 00000000..8da1e305 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkRewriter.php @@ -0,0 +1,73 @@ +]*?[\s"\'])?)href=(["\'])(.*?)\2([^>]*>)(.*?)<\/a>/is'; + private const TEXT_URL_PATTERN = '#https?://[^\s<>},]+#i'; + + /** + * Rewrites the href of every anchor. Anchors whose visible text itself contains a URL are left alone, as + * pointing them elsewhere would look like phishing. + * + * @param callable(string): ?string $rewrite gets the decoded href, returns the new url or null to keep the link + */ + public function rewriteHtml(string $html, callable $rewrite): string + { + if ($html === '') { + return $html; + } + + // null means a PCRE error (e.g. the backtrack limit on a large body): keep the original instead of emptying it + return preg_replace_callback( + self::HTML_ANCHOR_PATTERN, + function (array $match) use ($rewrite): 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 = $rewrite($href); + if ($url === null) { + return $match[0]; + } + + return $match[1] . 'href=' . $match[2] . htmlspecialchars($url, ENT_QUOTES) . $match[2] + . $match[4] . $match[5] . ''; + }, + $html + ) ?? $html; + } + + /** + * Rewrites every http(s) URL in the text, leaving a trailing dot (the end of a sentence) where it is. + * + * @param callable(string): ?string $rewrite gets the url, returns the new url or null to keep it + */ + public function rewriteText(string $text, callable $rewrite): string + { + if ($text === '') { + return $text; + } + + // null means a PCRE error: keep the original instead of emptying the text + return preg_replace_callback( + self::TEXT_URL_PATTERN, + function (array $match) use ($rewrite): string { + $trailing = str_ends_with($match[0], '.') ? '.' : ''; + $link = substr($match[0], 0, strlen($match[0]) - strlen($trailing)); + $url = $rewrite($link); + + return $url === null ? $match[0] : $url . $trailing; + }, + $text + ) ?? $text; + } +} diff --git a/src/Domain/Analytics/Service/LinkTrackService.php b/src/Domain/Analytics/Service/LinkTrackService.php index dc478e84..7153c40a 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\Repository\MessageRepository; +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 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 MessageRepository $messageRepository, + private readonly LinkTrackTokenService $tokenService, private readonly ParameterProvider $paramProvider, - private readonly EntityManagerInterface $entityManager, + private readonly LinkRewriter $linkRewriter, + private readonly AnalyticsTrackingService $analyticsTrackingService, ) { } @@ -26,71 +44,142 @@ 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 + * Replaces the links of the final HTML and text bodies with click-tracking links for this subscriber. It has to + * run once all placeholders are expanded, so that template links and links coming from placeholders (unsubscribe, + * preferences, ...) are tracked too, like phplist3 does. + * + * 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). + * + * Without click tracking, the analytics parameters (when given) are added straight onto the links instead, like + * phplist3 does. With click tracking they are added when the link is followed, see LinkClickService. + * + * Forwarded messages are not click tracked, as their receivers are not necessarily subscribers. * - * @return LinkTrack[] The saved LinkTrack entities - * @throws MissingMessageIdException + * @param array|null $analyticsParameters the campaign's analytics settings, null when it has + * no analytics tracking + * @return array{0: string, 1: string} the tracked HTML and text */ - 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( + string $html, + string $text, + int $campaignId, + Subscriber $subscriber, + ?array $analyticsParameters = null, + bool $forwarded = false, + ): array { + if (!$this->isClickTrackingEnabled()) { + return $analyticsParameters === null + ? [$html, $text] + : $this->analyticsTrackingService->addToBodies($html, $text, $analyticsParameters); } - if ($messageId === null) { - throw new MissingMessageIdException(); + $campaign = $forwarded ? null : $this->messageRepository->findById($campaignId); + if ($campaign === null) { + return [$html, $text]; } - $links = $this->extractLinksFromHtml($content->content); - - if ($content->htmlFooter) { - $links = array_merge($links, $this->extractLinksFromHtml($content->htmlFooter)); + $messageUuid = (string) $campaign->getUuid(); + $subscriberUuid = $subscriber->getUuid(); + if ($messageUuid === '' || $subscriberUuid === '') { + return [$html, $text]; } - $links = array_unique($links); + $forwardIds = []; + $trackUrl = function (string $url, string $type) use ($messageUuid, $subscriberUuid, &$forwardIds): ?string { + return $this->createTrackingUrl($url, $type, $messageUuid, $subscriberUuid, $forwardIds); + }; + + $html = $this->linkRewriter->rewriteHtml( + $html, + fn (string $href): ?string => $trackUrl($href, LinkTrackToken::TYPE_HTML) + ); + $text = $this->linkRewriter->rewriteText( + $text, + fn (string $url): ?string => $trackUrl($url, LinkTrackToken::TYPE_TEXT) + ); + + foreach (array_keys($forwardIds) as $forwardId) { + $this->linkTrackMlRepository->incrementTotal($campaignId, $forwardId); + } - $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); + return [$html, $text]; + } - $this->linkTrackRepository->persist($linkTrack); - $savedLinks[] = $linkTrack; + /** @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 $savedLinks; + 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 $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/Analytics/Service/Query/AnalyticsQueryInterface.php b/src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php new file mode 100644 index 00000000..befaab1a --- /dev/null +++ b/src/Domain/Analytics/Service/Query/AnalyticsQueryInterface.php @@ -0,0 +1,30 @@ + $messageData the campaign's message data + * @return array query parameters as name => value, empty values are not added to links + */ + public function trackingParameters(string $emailFormat, array $messageData): array; + + /** + * The parameters a campaign can set, with the values to use when it has not set them. + * + * @param array $messageData + * @return array + */ + public function editableParameters(array $messageData): array; + + /** Prefix shared by the parameter names, used to take off tracking parameters that are already on a link. */ + public function prefix(): string; +} diff --git a/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php b/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php new file mode 100644 index 00000000..1ad2a4c2 --- /dev/null +++ b/src/Domain/Analytics/Service/Query/GoogleAnalyticsQuery.php @@ -0,0 +1,32 @@ + $messageData['utm_source'] ?? null, + 'utm_medium' => $messageData['utm_medium'] ?? null, + 'utm_campaign' => $messageData['utm_campaign'] ?? null, + 'utm_content' => $emailFormat, + ]; + } + + public function editableParameters(array $messageData): array + { + return [ + 'utm_source' => 'phpList', + 'utm_medium' => 'email', + 'utm_campaign' => (string) ($messageData['subject'] ?? ''), + ]; + } + + public function prefix(): string + { + return 'utm_'; + } +} diff --git a/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php b/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php new file mode 100644 index 00000000..ce975b67 --- /dev/null +++ b/src/Domain/Analytics/Service/Query/MatomoAnalyticsQuery.php @@ -0,0 +1,32 @@ + $messageData['pk_source'] ?? null, + 'pk_medium' => $messageData['pk_medium'] ?? null, + 'pk_campaign' => $messageData['pk_campaign'] ?? null, + 'pk_content' => $emailFormat, + ]; + } + + public function editableParameters(array $messageData): array + { + return [ + 'pk_source' => 'phpList', + 'pk_medium' => 'email', + 'pk_campaign' => (string) ($messageData['subject'] ?? ''), + ]; + } + + public function prefix(): string + { + return 'pk_'; + } +} diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index b8995b35..fed3649f 100644 --- a/src/Domain/Common/FileHelper.php +++ b/src/Domain/Common/FileHelper.php @@ -33,32 +33,59 @@ 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 : ''); + // Restrictive umask so the file is private from creation (like tempnam()), whatever the process umask is + $previousUmask = umask(0077); + try { + $handle = fopen($fullPath, 'xb'); + } finally { + umask($previousUmask); + } + 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; + } - $fileHandle = fopen($fullPath, 'w'); + private function resolveTrustedDirectory(string $directory): ?string + { + if (!is_dir($directory) || !is_writable($directory)) { + return null; + } - fwrite($fileHandle, $contents); - fclose($fileHandle); + $realDirectory = realpath($directory); + if ($realDirectory === false) { + return null; + } - return $fullPath; + // 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 $realDirectory; } } diff --git a/src/Domain/Common/RemotePageFetcher.php b/src/Domain/Common/RemotePageFetcher.php index fdeb01db..5605c96d 100644 --- a/src/Domain/Common/RemotePageFetcher.php +++ b/src/Domain/Common/RemotePageFetcher.php @@ -52,7 +52,7 @@ public function __invoke(string $url, array $userData): string $cacheUrl = $this->urlCacheRepository->findByUrlAndLastModified($url, $lastModified); $content = $cacheUrl?->getContent(); if ($cacheUrl) { - // todo: check what the page should be for this log + // no page context here; phplist3 logs 'unknown page' in that case $this->eventLogManager->log(page: 'unknown page', entry: $url . ' was cached in database'); } else { $content = $this->fetchUrlDirect($url); diff --git a/src/Domain/Common/Service/ExternalImageService.php b/src/Domain/Common/Service/ExternalImageService.php index 63d9f35c..4f2314e2 100644 --- a/src/Domain/Common/Service/ExternalImageService.php +++ b/src/Domain/Common/Service/ExternalImageService.php @@ -91,7 +91,7 @@ private function removeOldFilesInCache(): void if ($cacheFile === false) { break; } - // todo: make sure that this is what we need + // skip hidden files and the . and .. entries, as phplist3 does if (!str_starts_with($cacheFile, '.')) { // phpcs:ignore Generic.PHP.NoSilencedErrors $cfmt = @filemtime($this->externalCacheDir . '/' . $cacheFile); 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/Configuration/Model/ConfigOption.php b/src/Domain/Configuration/Model/ConfigOption.php index 1c4a3376..41ec18dc 100644 --- a/src/Domain/Configuration/Model/ConfigOption.php +++ b/src/Domain/Configuration/Model/ConfigOption.php @@ -15,7 +15,6 @@ enum ConfigOption: string case ConfirmationUrl = 'confirmationurl'; case PreferencesUrl = 'preferencesurl'; case SubscribeUrl = 'subscribeurl'; - // todo: check where is this defined case Domain = 'domain'; case Website = 'website'; case MessageFromAddress = 'message_from_address'; @@ -23,6 +22,7 @@ enum ConfigOption: string case MessageReplyToAddress = 'message_replyto_address'; case SystemMessageTemplate = 'systemmessagetemplate'; case AlwaysAddGoogleTracking = 'always_add_googletracking'; + case AnalyticTracker = 'analytic_tracker'; case AdminAddress = 'admin_address'; case AdminAddresses = 'admin_addresses'; case DefaultMessageTemplate = 'defaultmessagetemplate'; diff --git a/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php b/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php index 8a5b626d..f99efa2c 100644 --- a/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php +++ b/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php @@ -4,15 +4,12 @@ namespace PhpList\Core\Domain\Configuration\Service\Placeholder; -use PhpList\Core\Domain\Configuration\Model\ConfigOption; -use PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider; use PhpList\Core\Domain\Configuration\Model\Dto\PlaceholderContext; use Symfony\Component\DependencyInjection\Attribute\Autowire; final class UserTrackValueResolver implements PlaceholderValueResolverInterface { public function __construct( - private readonly ConfigProvider $config, #[Autowire('%app.rest_api_base_url%')] private readonly string $restApiBaseUrl, ) { } @@ -24,9 +21,10 @@ public function name(): string public function __invoke(PlaceholderContext $ctx): string { - $base = $this->config->getValue(ConfigOption::Domain) ?? $this->restApiBaseUrl; + // The stored domain is scheme-free, which would make the pixel src relative, so always use the absolute base + $base = rtrim($this->restApiBaseUrl, '/'); - if ($ctx->isText() || empty($base)) { + if ($ctx->isText() || $base === '') { return ''; } $uid = $ctx->forwardedBy() ? 'forwarded' : $ctx->getUser()->getUniqueId(); diff --git a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php index 3222acc9..c1aea578 100644 --- a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php @@ -16,8 +16,10 @@ class DefaultConfigProvider */ private array $defaults = []; - public function __construct(private TranslatorInterface $translator) - { + public function __construct( + private readonly TranslatorInterface $translator, + private readonly string $website = '', + ) { } /** @SuppressWarnings("PHPMD.ExcessiveMethodLength") */ @@ -27,10 +29,29 @@ private function init(): void return; } + // the website may contain a port or path, the email domain is the bare host without a leading www. + $host = parse_url('//' . $this->website, PHP_URL_HOST); + $domain = preg_replace('/^www\./i', '', is_string($host) ? $host : $this->website); + $publicSchema = 'http'; $pageRoot = '/api/v2'; $this->defaults = [ + 'website' => [ + 'value' => $this->website, + 'description' => $this->translator->trans('Website address (without http://)'), + 'infoicon' => true, + 'type' => 'text', + 'allowempty' => false, + 'category' => 'general', + ], + 'domain' => [ + 'value' => $domain, + 'description' => $this->translator->trans('Domain Name of your server (for email)'), + 'type' => 'text', + 'allowempty' => false, + 'category' => 'general', + ], 'admin_address' => [ 'value' => 'webmaster@[DOMAIN]', 'description' => $this->translator->trans('Person in charge of this system (one email address)'), 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/MessageHandler/CampaignProcessorTestMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php index 61f47384..21895457 100644 --- a/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php +++ b/src/Domain/Messaging/MessageHandler/CampaignProcessorTestMessageHandler.php @@ -18,7 +18,6 @@ use PhpList\Core\Domain\Messaging\Service\MailSizeChecker; use PhpList\Core\Domain\Messaging\Service\MessageDataLoader; use PhpList\Core\Domain\Messaging\Service\MessagePrecacheService; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Service\Provider\SubscriberProvider; use Psr\Log\LoggerInterface; @@ -41,7 +40,6 @@ class CampaignProcessorTestMessageHandler public function __construct( private readonly MailerInterface $mailer, private readonly SubscriberProvider $subscriberProvider, - private readonly MessageProcessingPreparator $messagePreparator, private readonly LoggerInterface $logger, private readonly CacheInterface $cache, private readonly TranslatorInterface $translator, @@ -89,17 +87,10 @@ private function handleEmailSending( Subscriber $subscriber, MessagePrecacheDto $precachedContent, ): void { - // todo: check at which point link tracking should be applied (maybe after constructing full text?) - $processed = $this->messagePreparator->processMessageLinks( - campaignId: $campaign->getId(), - cachedMessageDto: $precachedContent, - subscriber: $subscriber - ); - try { $result = $this->campaignEmailBuilder->buildCampaignEmail( messageId: $campaign->getId(), - data: $processed, + data: $precachedContent, toEmail: $subscriber->getEmail(), skipBlacklistCheck: false, inBlast: true, @@ -132,7 +123,7 @@ private function handleEmailSending( ); $envelope = new Envelope( - sender: new Address($this->bounceEmail, 'PHPList'), + sender: new Address($this->bounceEmail), recipients: [new Address($email->getTo()[0]->getAddress())], ); $this->mailer->send(message: $email, envelope: $envelope); diff --git a/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php b/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php index 57cbc32b..928a5262 100644 --- a/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php +++ b/src/Domain/Messaging/Model/Dto/MessagePrecacheDto.php @@ -24,5 +24,7 @@ class MessagePrecacheDto public ?int $templateId = null; public bool $userSpecificUrl = false; public bool $googleTrack = false; + /** @var array the campaign's analytics settings (utm_*, pk_*) */ + public array $analyticsParameters = []; public array $adminAttributes = []; } 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/CampaignEmailSender.php b/src/Domain/Messaging/Service/CampaignEmailSender.php index 7cb590b0..e4babbfb 100644 --- a/src/Domain/Messaging/Service/CampaignEmailSender.php +++ b/src/Domain/Messaging/Service/CampaignEmailSender.php @@ -36,7 +36,6 @@ public function __construct( private readonly EmailBuilder $campaignEmailBuilder, private readonly RateLimitedCampaignMailer $rateLimitedCampaignMailer, private readonly MailSizeChecker $mailSizeChecker, - private readonly MessageProcessingPreparator $messagePreparator, private readonly MessageRepository $messageRepository, private readonly MessageStatusUpdater $messageStatusUpdater, private readonly SystemNotificationMailer $notificationMailer, @@ -54,17 +53,10 @@ public function send( UserMessage $userMessage, MessagePrecacheDto $precachedContent, ): void { - // todo: check at which point link tracking should be applied (maybe after constructing full text?) - $processed = $this->messagePreparator->processMessageLinks( - campaignId: $campaign->getId(), - cachedMessageDto: $precachedContent, - subscriber: $subscriber - ); - try { $result = $this->campaignEmailBuilder->buildCampaignEmail( messageId: $campaign->getId(), - data: $processed, + data: $precachedContent, toEmail: $subscriber->getEmail(), skipBlacklistCheck: false, inBlast: true, diff --git a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php index c7398a20..3d752bb2 100644 --- a/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php +++ b/src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Messaging\Service\Constructor; +use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Common\Html2Text; use PhpList\Core\Domain\Common\RemotePageFetcher; use PhpList\Core\Domain\Common\TextParser; @@ -28,6 +29,7 @@ public function __construct( private readonly Html2Text $html2Text, private readonly TextParser $textParser, private readonly MessagePlaceholderProcessor $placeholderProcessor, + private readonly LinkTrackService $linkTrackService, #[Autowire('%phplist.forward_personal_note_size%')] private readonly int $forwardPersonalNoteSize = 0, ) { } @@ -90,8 +92,16 @@ public function __invoke( forwardedBy: $forwardedBy, ); + [$htmlMessage, $textMessage] = $this->trackLinks( + htmlMessage: $htmlMessage, + textMessage: $textMessage, + receiver: $receiver, + campaignId: $campaignId, + messagePrecacheDto: $messagePrecacheDto, + forwarded: $forwardedBy !== null, + ); + $htmlMessage = $this->ensureHtmlFormating(content: $htmlMessage, addDefaultStyle: $addDefaultStyle); - // todo: add link CLICKTRACK to $htmlMessage [$htmlMessage, $textMessage] = $this->ensureNoteInCaseOfForwarded( htmlMessage: $htmlMessage, @@ -102,6 +112,33 @@ public function __invoke( return [$htmlMessage, $textMessage]; } + /** + * As in phplist3, links are tracked once all placeholders are expanded. + * + * @return array{0: string, 1: string} + */ + private function trackLinks( + string $htmlMessage, + string $textMessage, + Subscriber $receiver, + ?int $campaignId, + MessagePrecacheDto $messagePrecacheDto, + bool $forwarded, + ): array { + if ($campaignId === null) { + return [$htmlMessage, $textMessage]; + } + + return $this->linkTrackService->trackLinks( + html: $htmlMessage, + text: $textMessage, + campaignId: $campaignId, + subscriber: $receiver, + analyticsParameters: $messagePrecacheDto->googleTrack ? $messagePrecacheDto->analyticsParameters : null, + forwarded: $forwarded, + ); + } + private function replaceUserSpecificRemoteContent( MessagePrecacheDto $messagePrecacheDto, Subscriber $subscriber, 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/MessageDataManager.php b/src/Domain/Messaging/Service/Manager/MessageDataManager.php index 760fda60..b8dab1c2 100644 --- a/src/Domain/Messaging/Service/Manager/MessageDataManager.php +++ b/src/Domain/Messaging/Service/Manager/MessageDataManager.php @@ -34,33 +34,7 @@ public function setMessageData(Message $campaign, string $name, mixed $value): v $value = $this->normalizeValueByName($name, $value); - // todo: remove this once we have a proper way to handle targetlists -// if ($name === 'targetlist' && is_array($value)) { -// $this->listMessageRepository->removeAllListAssociationsForMessage($campaign); -// -// if (!empty($value['all']) || !empty($value['allactive'])) { -// // todo: should be with $GLOBALS['subselect'] filter for access control -// foreach ($this->subscriberListRepository->getAllActive() as $list) { -// $listMessage = (new ListMessage()) -// ->setMessage($campaign) -// ->setList($list); -// $this->listMessageRepository->persist($listMessage); -// } -// // once we used "all" to set all, unset it, to avoid confusion trying to unselect lists -// unset($value['all']); -// } else { -// foreach ($value as $listId => $val) { -// // see #16940 - ignore a list called "unselect" which is there to allow unselecting all -// if ($listId !== 'unselect') { -// $list = $this->subscriberListRepository->find($listId); -// $listMessage = (new ListMessage()) -// ->setMessage($campaign) -// ->setList($list); -// $this->listMessageRepository->persist($listMessage); -// } -// } -// } -// } + // todo: targetLists (not only here) should be with $GLOBALS['subselect'] filter for access control if (is_array($value) || is_object($value)) { $value = 'SER:' . serialize($value); 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/MessagePrecacheService.php b/src/Domain/Messaging/Service/MessagePrecacheService.php index 59641c5f..86ed5b31 100644 --- a/src/Domain/Messaging/Service/MessagePrecacheService.php +++ b/src/Domain/Messaging/Service/MessagePrecacheService.php @@ -77,6 +77,7 @@ public function precacheMessage( } $messagePrecacheDto->googleTrack = (bool) $loadedMessageData['google_track']; + $messagePrecacheDto->analyticsParameters = $this->extractAnalyticsParameters($loadedMessageData); $this->applyBasicReplacements($messagePrecacheDto, $loadedMessageData); $this->populateAdminAttributes($messagePrecacheDto, $campaign); @@ -257,4 +258,20 @@ private function populateAdminAttributes(MessagePrecacheDto $messagePrecacheDto, $messagePrecacheDto->adminAttributes['LISTOWNER.' . $attr['name']] = $attr['value']; } } + + /** + * The analytics settings of the campaign: the Google (utm_*) and Matomo (pk_*) parameters it has set, and its + * subject, which is their default campaign name. + */ + private function extractAnalyticsParameters(array $loadedMessageData): array + { + $parameters = ['subject' => (string) ($loadedMessageData['subject'] ?? '')]; + foreach ($loadedMessageData as $name => $value) { + if (is_string($name) && preg_match('/^(utm|pk)_/', $name) === 1) { + $parameters[$name] = $value; + } + } + + return $parameters; + } } diff --git a/src/Domain/Messaging/Service/MessageProcessingPreparator.php b/src/Domain/Messaging/Service/MessageProcessingPreparator.php index 05f044ea..acdd1914 100644 --- a/src/Domain/Messaging/Service/MessageProcessingPreparator.php +++ b/src/Domain/Messaging/Service/MessageProcessingPreparator.php @@ -4,24 +4,16 @@ 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; -use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Contracts\Translation\TranslatorInterface; 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, - private readonly LinkTrackService $linkTrackService, private readonly TranslatorInterface $translator, ) { } @@ -55,55 +47,4 @@ public function ensureCampaignsHaveUuid(OutputInterface $output): void } } } - - /** - * Process message content to extract URLs and replace them with link track URLs - */ - public function processMessageLinks( - int $campaignId, - MessagePrecacheDto $cachedMessageDto, - Subscriber $subscriber - ): MessagePrecacheDto { - if (!$this->linkTrackService->isExtractAndSaveLinksApplicable()) { - return $cachedMessageDto; - } - - $savedLinks = $this->linkTrackService->extractAndSaveLinks( - content: $cachedMessageDto, - userId: $subscriber->getId(), - messageId: $campaignId, - ); - if (empty($savedLinks)) { - 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; - } } diff --git a/src/Domain/Messaging/Service/SystemNotificationMailer.php b/src/Domain/Messaging/Service/SystemNotificationMailer.php index 62c60628..160ad669 100644 --- a/src/Domain/Messaging/Service/SystemNotificationMailer.php +++ b/src/Domain/Messaging/Service/SystemNotificationMailer.php @@ -40,9 +40,8 @@ public function send(int $messageId, string $toEmail, string $subject, string $c return false; } - // todo: check if from name should be from config $envelope = new Envelope( - sender: new Address($this->bounceEmail, 'PHPList'), + sender: new Address($this->bounceEmail), recipients: [new Address($email->getTo()[0]->getAddress())], ); $this->mailer->send(message: $email, envelope: $envelope); diff --git a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php index ee468fa1..c76c3430 100644 --- a/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php +++ b/src/Domain/Subscription/Model/Dto/ImportSubscriberDto.php @@ -20,9 +20,6 @@ class ImportSubscriberDto #[Assert\Type('bool')] public bool $confirmed; - #[Assert\Type('bool')] - public bool $blacklisted; - #[Assert\Type('bool')] public bool $htmlEmail; @@ -39,7 +36,6 @@ class ImportSubscriberDto public function __construct( string $email, bool $confirmed, - bool $blacklisted, bool $htmlEmail, bool $disabled, ?string $extraData = null, @@ -49,7 +45,6 @@ public function __construct( $this->email = $email; $this->confirmed = $confirmed; $this->sendConfirmation = !$confirmed; - $this->blacklisted = $blacklisted; $this->htmlEmail = $htmlEmail; $this->disabled = $disabled; $this->extraData = $extraData; diff --git a/src/Domain/Subscription/Model/Subscriber.php b/src/Domain/Subscription/Model/Subscriber.php index e29a9b7d..7e8ce0e4 100644 --- a/src/Domain/Subscription/Model/Subscriber.php +++ b/src/Domain/Subscription/Model/Subscriber.php @@ -174,7 +174,6 @@ public function setConfirmed(bool $confirmed): self return $this; } - // todo: check where is this used and if should be replaced by isBlacklisted with grace time public function isBlacklisted(): bool { return $this->blacklisted; diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index b55164c3..706e7692 100644 --- a/src/Domain/Subscription/Repository/SubscriberRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberRepository.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Subscription\Repository; +use DateTimeImmutable; use DateTimeInterface; use Doctrine\ORM\QueryBuilder; use InvalidArgumentException; @@ -13,6 +14,8 @@ 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; +use PhpList\Core\Domain\Subscription\Model\UserBlacklist; /** * Repository for Subscriber models. @@ -145,9 +148,7 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes } if ($filter->getIsBlacklisted() !== null) { - $queryBuilder - ->andWhere('subscriber.blacklisted = :isBlacklisted') - ->setParameter('isBlacklisted', $filter->getIsBlacklisted()); + $this->applyBlacklistedFilter($queryBuilder, $filter->getIsBlacklisted()); } if ($filter->getFindColumn() && $filter->getFindValue()) { @@ -272,6 +273,49 @@ 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(); + } + + /** + * Matches the live blacklist status (stored flag or an entry in the blacklist table), the same status the CSV + * export writes, so a stale stored flag does not make the filter and the exported value disagree. + */ + private function applyBlacklistedFilter(QueryBuilder $queryBuilder, bool $isBlacklisted): void + { + $inBlacklistTable = sprintf( + 'EXISTS (SELECT 1 FROM %s ub WHERE ub.email = subscriber.email AND ub.added < :blacklistNow)', + UserBlacklist::class + ); + + $queryBuilder + ->andWhere( + $isBlacklisted + ? '(subscriber.blacklisted = true OR ' . $inBlacklistTable . ')' + : '(subscriber.blacklisted = false AND NOT ' . $inBlacklistTable . ')' + ) + ->setParameter('blacklistNow', new DateTimeImmutable()); + } + public function isEmailBlacklisted(string $email): bool { $queryBuilder = $this->getEntityManager()->createQueryBuilder(); diff --git a/src/Domain/Subscription/Repository/UserBlacklistRepository.php b/src/Domain/Subscription/Repository/UserBlacklistRepository.php index 9dabc9ff..5651c20c 100644 --- a/src/Domain/Subscription/Repository/UserBlacklistRepository.php +++ b/src/Domain/Subscription/Repository/UserBlacklistRepository.php @@ -58,4 +58,28 @@ public function isEmailBlacklisted(string $email, ?bool $immediate = true): bool ->getQuery() ->getOneOrNullResult() !== null; } + + /** + * Batch variant of isEmailBlacklisted(): returns the given emails (lowercased) that are blacklisted. + * + * @param string[] $emails + * @return string[] + */ + public function findBlacklistedEmails(array $emails): array + { + if ($emails === []) { + return []; + } + + $rows = $this->createQueryBuilder('ub') + ->select('ub.email') + ->where('ub.email IN (:emails)') + ->andWhere('ub.added < :cutoff') + ->setParameter('emails', array_values($emails)) + ->setParameter('cutoff', new DateTimeImmutable()) + ->getQuery() + ->getScalarResult(); + + return array_map(static fn(array $row): string => strtolower($row['email']), $rows); + } } diff --git a/src/Domain/Subscription/Service/ArrayToDtoMapper.php b/src/Domain/Subscription/Service/ArrayToDtoMapper.php index dcd2f6e8..52e8be47 100644 --- a/src/Domain/Subscription/Service/ArrayToDtoMapper.php +++ b/src/Domain/Subscription/Service/ArrayToDtoMapper.php @@ -9,6 +9,7 @@ class ArrayToDtoMapper { private const FK_HEADER = 'foreignkey'; + // 'blacklisted' stays a known header so the column is ignored instead of becoming an attribute private const KNOWN_HEADERS = [ 'email', 'confirmed', 'blacklisted', 'html_email', 'disabled', 'extra_data', 'foreignkey', ]; @@ -31,7 +32,6 @@ public function map(array $row): ImportSubscriberDto return new ImportSubscriberDto( email: $email, confirmed: filter_var($normalizedRow['confirmed'] ?? false, FILTER_VALIDATE_BOOLEAN), - blacklisted: filter_var($normalizedRow['blacklisted'] ?? false, FILTER_VALIDATE_BOOLEAN), htmlEmail: filter_var($normalizedRow['html_email'] ?? false, FILTER_VALIDATE_BOOLEAN), disabled: filter_var($normalizedRow['disabled'] ?? false, FILTER_VALIDATE_BOOLEAN), extraData: $normalizedRow['extra_data'] ?? null, diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 189a2881..6c9e6f6d 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -110,7 +110,6 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber { $subscriber = new Subscriber($subscriberDto->email); $subscriber->setConfirmed($subscriberDto->confirmed); - $subscriber->setBlacklisted($subscriberDto->blacklisted); $subscriber->setHtmlEmail($subscriberDto->htmlEmail); $subscriber->setDisabled($subscriberDto->disabled); $subscriber->setExtraData($subscriberDto->extraData); @@ -127,7 +126,6 @@ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscribe { $existingSubscriber->setEmail($subscriberDto->email); $existingSubscriber->setConfirmed($subscriberDto->confirmed); - $existingSubscriber->setBlacklisted($subscriberDto->blacklisted); $existingSubscriber->setHtmlEmail($subscriberDto->htmlEmail); $existingSubscriber->setDisabled($subscriberDto->disabled); $existingSubscriber->setExtraData($subscriberDto->extraData); diff --git a/src/Domain/Subscription/Service/SubscriberCsvExporter.php b/src/Domain/Subscription/Service/SubscriberCsvExporter.php index 48802fe0..1420fe9e 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvExporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvExporter.php @@ -9,6 +9,7 @@ use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Resolver\AttributeValueResolver; use Psr\Log\LoggerInterface; @@ -27,6 +28,7 @@ public function __construct( private readonly SubscriberRepository $subscriberRepository, private readonly SubscriberAttributeDefinitionRepository $definitionRepository, private readonly LoggerInterface $logger, + private readonly UserBlacklistRepository $blacklistRepository, ) { } @@ -178,11 +180,16 @@ private function exportSubscribers( 'count' => $subscriberCount ]); + $blacklistedEmails = in_array('blacklisted', $headers, true) + ? $this->findBlacklistedEmails($subscribers) + : []; + foreach ($subscribers as $subscriber) { $row = $this->getSubscriberRow( subscriber: $subscriber, attributeDefinitions: $attributeDefinitions, headers: $headers, + blacklistedEmails: $blacklistedEmails, ); fputcsv($handle, $row); $lastId = $subscriber->getId(); @@ -209,11 +216,16 @@ private function exportSubscribers( * * @param Subscriber $subscriber The subscriber * @param SubscriberAttributeDefinition[] $attributeDefinitions Attribute definitions + * @param string[] $blacklistedEmails Lowercased emails of the batch found in the blacklist table * @return array Row data */ - private function getSubscriberRow(Subscriber $subscriber, array $attributeDefinitions, array $headers): array - { - $row = $this->normalizerSubscriberData($subscriber); + private function getSubscriberRow( + Subscriber $subscriber, + array $attributeDefinitions, + array $headers, + array $blacklistedEmails, + ): array { + $row = $this->normalizerSubscriberData($subscriber, $blacklistedEmails); foreach ($attributeDefinitions as $definition) { $attrValue = $this->attributeManager->getSubscriberAttribute( @@ -253,14 +265,34 @@ private function configureResponse(BinaryFileResponse $response): Response return $response; } + /** + * Re-verifies the flag against the blacklist table with one query per batch, so stale flags are not exported. + * Subscribers whose stored flag is already set are not looked up. + * + * @param Subscriber[] $subscribers + * @return string[] lowercased blacklisted emails + */ + private function findBlacklistedEmails(array $subscribers): array + { + $emails = []; + foreach ($subscribers as $subscriber) { + if (!$subscriber->isBlacklisted()) { + $emails[] = $subscriber->getEmail(); + } + } + + return $this->blacklistRepository->findBlacklistedEmails($emails); + } + /** @SuppressWarnings("CyclomaticComplexity") */ - private function normalizerSubscriberData(Subscriber $subscriber): array + private function normalizerSubscriberData(Subscriber $subscriber, array $blacklistedEmails): array { return [ 'id' => $subscriber->getId(), 'email' => $subscriber->getEmail(), 'confirmed' => $subscriber->isConfirmed() ? '1' : '0', - 'blacklisted' => $subscriber->isBlacklisted() ? '1' : '0', + 'blacklisted' => $subscriber->isBlacklisted() + || in_array(strtolower($subscriber->getEmail()), $blacklistedEmails, true) ? '1' : '0', 'bounceCount' => $subscriber->getBounceCount(), 'createdAt' => $subscriber->getCreatedAt()?->format('Y-m-d H:i:s') ?? '', 'updatedAt' => $subscriber->getUpdatedAt()->format('Y-m-d H:i:s'), diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index f9db822a..18ee26a1 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -13,6 +13,7 @@ use PhpList\Core\Domain\Subscription\Model\Dto\SubscriberImportOptions; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; @@ -44,6 +45,7 @@ public function __construct( private readonly TranslatorInterface $translator, private readonly MessageBusInterface $messageBus, private readonly SubscriberHistoryManager $subscriberHistoryManager, + private readonly UserBlacklistRepository $blacklistRepository, ) { } @@ -187,13 +189,16 @@ private function processRow( $this->attributeManager->processAttributes($subscriber, $dto->extraAttributes); - [$listLines, $addedNewSubscriberToList] = $this->getHistoryListLines($subscriber, $options); - - // todo: use isBlacklisted logic (grace time) not function from Subscriber class - if ($subscriber->isBlacklisted()) { + // as in phplist3, the CSV blacklisted column is ignored; only the blacklist table decides, and the flag is + // only ever raised, never cleared, by an import + $isBlacklisted = $this->blacklistRepository->isEmailBlacklisted($subscriber->getEmail()); + if ($isBlacklisted) { + $subscriber->setBlacklisted(true); $stats['blacklisted']++; } + [$listLines, $addedNewSubscriberToList] = $this->getHistoryListLines($subscriber, $options, $isBlacklisted); + $this->subscriberHistoryManager->addHistoryFromImport( subscriber: $subscriber, listLines: $listLines, @@ -273,11 +278,14 @@ private function prepareConfirmationMessage( return null; } - private function getHistoryListLines(Subscriber $subscriber, SubscriberImportOptions $options): array - { + private function getHistoryListLines( + Subscriber $subscriber, + SubscriberImportOptions $options, + bool $isBlacklisted, + ): array { $addedNewSubscriberToList = false; $listLines = []; - if (!$subscriber->isBlacklisted() && count($options->listIds) > 0) { + if (!$isBlacklisted && count($options->listIds) > 0) { foreach ($options->listIds as $listId) { $created = $this->subscriptionManager->addSubscriberToAList($subscriber, $listId); if ($created) { 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/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php b/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php index c835207c..48677399 100644 --- a/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php +++ b/tests/Integration/Domain/Subscription/Repository/SubscriberRepositoryTest.php @@ -11,6 +11,7 @@ use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberList; use PhpList\Core\Domain\Subscription\Model\Subscription; +use PhpList\Core\Domain\Subscription\Model\UserBlacklist; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriptionRepository; use PhpList\Core\TestingSupport\Traits\DatabaseTestTrait; @@ -303,6 +304,32 @@ public function testGetSubscribersBySubscribedListIdsReturnsEmptyArrayForEmptyIn self::assertSame([], $this->subscriberRepository->getSubscribersBySubscribedListIds([])); } + public function testGetFilteredAfterIdBlacklistedFilterUsesLiveBlacklistStatus(): void + { + $flagged = (new Subscriber('flagged@example.com'))->setBlacklisted(true); + $stale = new Subscriber('stale@example.com'); + $clean = new Subscriber('clean@example.com'); + foreach ([$flagged, $stale, $clean] as $subscriber) { + $this->entityManager->persist($subscriber); + } + $this->entityManager->persist((new UserBlacklist('stale@example.com'))->setAdded(new DateTime('-1 day'))); + $this->entityManager->flush(); + + $emailsFor = function (bool $isBlacklisted): array { + $result = $this->subscriberRepository->getFilteredAfterId( + new SubscriberFilter(isBlacklisted: $isBlacklisted) + ); + + return array_map( + static fn (Subscriber $subscriber): string => $subscriber->getEmail(), + $result->getItems() + ); + }; + + self::assertEqualsCanonicalizing(['flagged@example.com', 'stale@example.com'], $emailsFor(true)); + self::assertSame(['clean@example.com'], $emailsFor(false)); + } + public function testGetFilteredAfterIdWithoutListFilterReturnsAllSubscribersOnce(): void { $list = (new SubscriberList())->setName('list'); diff --git a/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php b/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php index 9c27c1c9..914cdde8 100644 --- a/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php +++ b/tests/Integration/Domain/Subscription/Service/SubscriberCsvImportManagerTest.php @@ -86,7 +86,7 @@ public function testImportFromCsvCreatesNewSubscribers(): void self::assertInstanceOf(Subscriber::class, $subscriber2); self::assertFalse($subscriber2->isConfirmed()); self::assertFalse($subscriber2->hasHtmlEmail()); - self::assertTrue($subscriber2->isBlacklisted()); + self::assertFalse($subscriber2->isBlacklisted()); self::assertTrue($subscriber2->isDisabled()); self::assertSame('More data', $subscriber2->getExtraData()); @@ -130,7 +130,7 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void self::assertInstanceOf(Subscriber::class, $updatedSubscriber); self::assertTrue($updatedSubscriber->isConfirmed()); self::assertTrue($updatedSubscriber->hasHtmlEmail()); - self::assertFalse($updatedSubscriber->isBlacklisted()); + self::assertTrue($updatedSubscriber->isBlacklisted()); self::assertFalse($updatedSubscriber->isDisabled()); self::assertSame('Updated data', $updatedSubscriber->getExtraData()); diff --git a/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php new file mode 100644 index 00000000..2aa6e120 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php @@ -0,0 +1,198 @@ +configProvider = $this->createMock(ConfigProvider::class); + $this->messageDataLoader = $this->createMock(MessageDataLoader::class); + $this->service = new AnalyticsTrackingService( + $this->configProvider, + $this->messageDataLoader, + new LinkRewriter(), + ); + } + + private function useTracker(?string $tracker): void + { + $this->configProvider->method('getValue')->with(ConfigOption::AnalyticTracker)->willReturn($tracker); + } + + public function testGoogleParametersAreAddedWithTheEmailFormat(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl( + 'https://example.com/page', + 'HTML', + ['utm_source' => 'news', 'utm_medium' => 'mail', 'utm_campaign' => 'spring'] + ); + + self::assertSame( + 'https://example.com/page?utm_source=news&utm_medium=mail&utm_campaign=spring&utm_content=HTML', + $url + ); + } + + public function testMatomoParametersAreUsedWhenConfigured(): void + { + $this->useTracker('matomo'); + + $url = $this->service->addToUrl( + 'https://example.com/page?a=1', + 'text', + ['pk_source' => 'news', 'pk_medium' => 'mail', 'pk_campaign' => 'spring', 'utm_source' => 'ignored'] + ); + + self::assertSame( + 'https://example.com/page?a=1&pk_source=news&pk_medium=mail&pk_campaign=spring&pk_content=text', + $url + ); + } + + public function testCampaignWithoutOwnValuesGetsTheDefaults(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl('https://example.com/', 'HTML', ['subject' => 'Hello World']); + + self::assertSame( + 'https://example.com/?utm_source=phpList&utm_medium=email&utm_campaign=Hello%20World&utm_content=HTML', + str_replace('+', '%20', $url) + ); + } + + public function testParameterSetToEmptyIsLeftOutInsteadOfGettingItsDefault(): void + { + $this->useTracker('google'); + + $url = $this->service->addToUrl( + 'https://example.com/', + 'HTML', + ['utm_source' => '', 'subject' => 'Hi'] + ); + + self::assertSame('https://example.com/?utm_medium=email&utm_campaign=Hi&utm_content=HTML', $url); + } + + public function testUnknownTrackerFallsBackToGoogle(): void + { + $this->useTracker(null); + + self::assertStringContainsString( + 'utm_content=HTML', + $this->service->addToUrl('https://example.com/', 'HTML', []) + ); + } + + public function testOnlyHttpUrlsGetParameters(): void + { + $this->useTracker('google'); + + foreach (['mailto:a@example.com', 'ftp://example.com/f', '[UNSUBSCRIBEURL]', '#top'] as $url) { + self::assertSame($url, $this->service->addToUrl($url, 'HTML', [])); + } + } + + public function testExistingTrackingParametersAreReplacedAndTheFragmentStaysLast(): void + { + $url = $this->service->withParameters( + 'https://example.com/p?a=1&utm_source=old&utm_medium=old&b=2#section', + ['utm_source' => 'new', 'utm_content' => 'HTML'], + 'utm_' + ); + + self::assertSame('https://example.com/p?a=1&b=2&utm_source=new&utm_content=HTML#section', $url); + } + + public function testOnlyQueryParametersStartingWithThePrefixAreRemoved(): void + { + $this->assertSame( + 'https://example.com/utm_page?x_utm_a=1&b=2', + $this->service->withParameters( + 'https://example.com/utm_page?x_utm_a=1&utm_source=old&&b=2&utm_medium=old', + [], + 'utm_' + ) + ); + $this->assertSame( + 'https://example.com/utm_page', + $this->service->withParameters('https://example.com/utm_page?utm_source=old', [], 'utm_') + ); + } + + public function testEmptyParametersAreLeftOut(): void + { + self::assertSame( + 'https://example.com/?utm_content=text', + $this->service->withParameters( + 'https://example.com/', + ['utm_source' => '', 'utm_medium' => null, 'utm_content' => 'text'], + 'utm_' + ) + ); + self::assertSame( + 'https://example.com/#x', + $this->service->withParameters('https://example.com/#x', ['utm_source' => ''], 'utm_') + ); + } + + public function testBodiesGetParametersOnHtmlLinksAndTextUrls(): void + { + $this->useTracker('google'); + $data = ['utm_source' => 'news', 'utm_medium' => 'mail', 'utm_campaign' => 'c']; + + [$html, $text] = $this->service->addToBodies( + 'A m', + 'Read https://example.com/b. Thanks', + $data + ); + + self::assertStringContainsString( + 'href="https://example.com/a?x=1&y=2&utm_source=news&utm_medium=mail' + . '&utm_campaign=c&utm_content=HTML"', + $html + ); + self::assertStringContainsString('href="mailto:a@example.com"', $html); + self::assertSame( + 'Read https://example.com/b?utm_source=news&utm_medium=mail&utm_campaign=c&utm_content=text. Thanks', + $text + ); + } + + public function testClickedUrlGetsParametersOnlyWhenTheCampaignTracksAnalytics(): void + { + $this->useTracker('google'); + $campaign = $this->createMock(Message::class); + $this->messageDataLoader->method('__invoke')->with($campaign)->willReturnOnConsecutiveCalls( + ['google_track' => false, 'utm_source' => 'news'], + ['google_track' => true, 'utm_source' => 'news', 'utm_medium' => 'm', 'utm_campaign' => 'c'], + ); + + self::assertSame( + 'https://example.com/', + $this->service->addToClickedUrl('https://example.com/', $campaign, true) + ); + self::assertSame( + 'https://example.com/?utm_source=news&utm_medium=m&utm_campaign=c&utm_content=text', + $this->service->addToClickedUrl('https://example.com/', $campaign, false) + ); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php new file mode 100644 index 00000000..1f1ada68 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -0,0 +1,253 @@ +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->analyticsTrackingService = $this->createMock(AnalyticsTrackingService::class); + $this->analyticsTrackingService->method('addToClickedUrl')->willReturnArgument(0); + $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->analyticsTrackingService, + ); + + $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 testAnalyticsParametersAreAddedToTheRedirectUrl(): void + { + $this->givenKnownLink(); + $this->givenUserMessage(viewed: true); + + $analytics = $this->createMock(AnalyticsTrackingService::class); + $analytics->expects(self::once()) + ->method('addToClickedUrl') + ->with('https://example.com/a', $this->message, true) + ->willReturn('https://example.com/a?utm_source=phpList'); + $service = new LinkClickService( + $this->tokenService, + $this->forwardRepository, + $this->mlRepository, + $this->umlClickRepository, + $this->messageRepository, + $this->subscriberRepository, + $this->userMessageRepository, + $this->userMessageService, + $analytics, + ); + + self::assertSame('https://example.com/a?utm_source=phpList', $service->trackClick($this->tid(), null, [])); + } + + 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/LinkRewriterTest.php b/tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php new file mode 100644 index 00000000..942f665b --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkRewriterTest.php @@ -0,0 +1,53 @@ +backtrackLimit = (string) ini_get('pcre.backtrack_limit'); + } + + protected function tearDown(): void + { + ini_set('pcre.backtrack_limit', $this->backtrackLimit); + } + + public function testHtmlIsRewritten(): void + { + $result = (new LinkRewriter())->rewriteHtml( + '

link

', + static fn (string $url): string => $url . '?x=1' + ); + + $this->assertSame('

link

', $result); + } + + public function testHtmlIsKeptWhenThePatternFailsWithAPcreError(): void + { + $html = '' . str_repeat('word ', 200) . ''; + ini_set('pcre.backtrack_limit', '10'); + + $result = (new LinkRewriter())->rewriteHtml($html, static fn (string $url): string => $url . '?x=1'); + + $this->assertSame($html, $result); + } + + public function testTextIsRewritten(): void + { + $result = (new LinkRewriter())->rewriteText( + 'see https://example.com/a.', + static fn (string $url): string => $url . '?x=1' + ); + + $this->assertSame('see https://example.com/a?x=1.', $result); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php index c7495f99..d6cfc5c8 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -4,257 +4,281 @@ 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\AnalyticsTrackingService; +use PhpList\Core\Domain\Analytics\Service\LinkRewriter; use PhpList\Core\Domain\Analytics\Service\LinkTrackService; -use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; +use PhpList\Core\Domain\Analytics\Service\LinkTrackTokenService; use PhpList\Core\Domain\Messaging\Model\Message; -use PhpList\Core\Domain\Messaging\Model\Message\MessageContent; +use PhpList\Core\Domain\Messaging\Repository\MessageRepository; +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 MessageRepository&MockObject $messageRepository; + private AnalyticsTrackingService&MockObject $analyticsTrackingService; + 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->messageRepository = $this->createMock(MessageRepository::class); + $this->analyticsTrackingService = $this->createMock(AnalyticsTrackingService::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->messageRepository->method('findById')->with(7)->willReturn($this->campaign); + + $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->messageRepository, + $this->tokenService, + new ParameterProvider(['click_track' => $clickTrack]), + new LinkRewriter(), + $this->analyticsTrackingService, + ); + } - $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; - }); + $html = 'x'; + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService(false)->trackLinks($html, 'https://example.com/', 7, $this->subscriber); - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://test.com', $result[1]->getUrl()); + self::assertSame([$html, 'https://example.com/'], $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); + $repository = $this->createMock(MessageRepository::class); + $repository->method('findById')->willReturn($campaign); + $service = new LinkTrackService( + $this->createMock(LinkTrackRepository::class), + $this->forwardRepository, + $this->mlRepository, + $repository, + $this->tokenService, + new ParameterProvider(['click_track' => true]), + new LinkRewriter(), + $this->analyticsTrackingService, + ); + $html = 'x'; - self::assertCount(2, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); - self::assertSame('https://footer.com', $result[1]->getUrl()); + self::assertSame([$html, ''], $service->trackLinks($html, '', 7, $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; - }); + $html = '

A ' + . "

"; - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); - self::assertCount(1, $result); - self::assertSame('https://example.com', $result[0]->getUrl()); + self::assertStringContainsString('A', $result); + self::assertStringContainsString("createdForwards, 'stored url is decoded'); + self::assertArrayHasKey('http://example.org/b', $this->createdForwards); } - public function testExtractAndSaveLinksWithNullText(): void + public function testTrackingLinkIdentifiesForwardCampaignAndSubscriber(): 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; - }); + [$result] = $this->createService()->trackLinks( + 'A', + '', + 7, + $this->subscriber + ); - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result, $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://footer.com', $result[0]->getUrl()); + public function testLinksThatMustNotBeTrackedAreLeftAlone(): void + { + $html = implode(' ', [ + 'mail', + 'unsubscribe', + 'placeholder', + 'script', + 'https://example.com/other', + 'too long', + 'already tracked', + 'no href', + ]); + + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + $this->mlRepository->expects(self::never())->method('incrementTotal'); + + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); + + self::assertSame($html, $result); } - public function testExtractAndSaveLinksWithMessageWithoutId(): void + public function testPersonalUidAndSessionParametersAreStrippedAndMarkTheForwardPersonal(): void { - $userId = 456; - $htmlContent = '

Link

'; + $this->createService()->trackLinks( + 'P', + '', + 7, + $this->subscriber + ); - $messageContent = new MessageContent('Test Subject', $htmlContent); - $messagePrecacheDto = new MessagePrecacheDto(); - $messagePrecacheDto->subject = 'Test Subject'; - $messagePrecacheDto->content = $htmlContent; + self::assertSame( + ['https://example.com/p?a=1#top' => ['url' => 'https://example.com/p?a=1#top', 'personalise' => true]], + $this->createdForwards + ); + } - $message = $this->createMock(Message::class); - $message->method('getId')->willReturn(null); - $message->method('getContent')->willReturn($messageContent); + public function testTextUrlsAreReplacedWithTextTokens(): void + { + $text = "Read https://example.com/a. Or see http://example.org/b?x=1\nbye\nMore: https://example.com/a"; - $this->expectException(MissingMessageIdException::class); - $this->expectExceptionMessage('Message must have an ID'); + [, $result] = $this->createService()->trackLinks('', $text, 7, $this->subscriber); - $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 + ); + preg_match('~tid=([A-Za-z0-9_-]+)~', $result, $match); + self::assertSame('T', $this->tokenService->parse($match[1])->type); + $uuids = $this->trackedForwardUuids($result); + self::assertSame($uuids[0], $uuids[2], 'the same url gets the same forward'); + self::assertSame("\nbye\n", substr($result, strpos($result, "\n"), 5)); } - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsTrue(): void + public function testLinksFromExpandedPlaceholdersAreTracked(): void { - self::assertTrue($this->subject->isExtractAndSaveLinksApplicable()); + $html = 'Unsubscribe'; + + [$result] = $this->createService()->trackLinks($html, '', 7, $this->subscriber); + + self::assertSame(['forward-uuid-1'], $this->trackedForwardUuids($result)); } - public function testIsExtractAndSaveLinksApplicableWhenClickTrackIsFalse(): void + public function testTotalIsIncrementedOncePerDistinctLinkOfTheEmail(): 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) - ); + $html = '12' + . '3'; + + $incremented = []; + $this->mlRepository->expects(self::exactly(2)) + ->method('incrementTotal') + ->willReturnCallback(function (int $messageId, int $forwardId) use (&$incremented): void { + $incremented[] = [$messageId, $forwardId]; + }); + + $this->createService()->trackLinks($html, 'https://example.com/a', 7, $this->subscriber); - self::assertFalse($subject->isExtractAndSaveLinksApplicable()); + self::assertSame([[7, 1], [7, 2]], $incremented); } - public function testExtractAndSaveLinksWithExistingLink(): void + public function testAnalyticsParametersAreAddedStraightToTheLinksWithoutClickTracking(): void { - $messageId = 123; - $userId = 456; - $url = 'https://example.com'; - $htmlContent = '

Check out this link.

'; - - $messageContent = new MessageContent('Test Subject', $htmlContent); + $this->analyticsTrackingService->expects(self::once()) + ->method('addToBodies') + ->with('x', 'text', ['utm_source' => 'news']) + ->willReturn(['html with utm', 'text with utm']); + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); + + $result = $this->createService(false)->trackLinks( + 'x', + 'text', + 7, + $this->subscriber, + ['utm_source' => 'news'] + ); - $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; + self::assertSame(['html with utm', 'text with utm'], $result); + } - $existingLinkTrack = new LinkTrack(); - $existingLinkTrack->setMessageId($messageId); - $existingLinkTrack->setUserId($userId); - $existingLinkTrack->setUrl($url); + public function testAnalyticsParametersAreNotAddedByTheSendWhenClickTrackingIsOn(): void + { + $this->analyticsTrackingService->expects(self::never())->method('addToBodies'); - $this->linkTrackRepository->expects(self::once()) - ->method('findByUrlUserIdAndMessageId') - ->with($url, $userId, $messageId) - ->willReturn($existingLinkTrack); + $this->createService(true)->trackLinks('x', '', 7, $this->subscriber, []); + } - $this->linkTrackRepository->expects(self::never()) - ->method('persist'); + public function testForwardedMessagesAreNotClickTracked(): void + { + $html = 'x'; + $this->forwardRepository->expects(self::never())->method('findOrCreateByUrl'); - $result = $this->subject->extractAndSaveLinks($messagePrecacheDto, $userId, $messageId); + $result = $this->createService()->trackLinks($html, '', 7, $this->subscriber, null, true); - self::assertCount(1, $result); - self::assertSame($existingLinkTrack, $result[0]); + self::assertSame([$html, ''], $result); } } 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..4e1f3d7e 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 @@ -65,13 +65,36 @@ public function testWriteFileToDirectoryCreatesFileWithExtensionAndContents(): v { $helper = new FileHelper(); - $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + $previousUmask = umask(0022); + try { + $writtenPath = $helper->writeFileToDirectory($this->tmpDir, 'report.txt', 'payload'); + } finally { + umask($previousUmask); + } $this->assertNotNull($writtenPath); $this->assertStringStartsWith($this->tmpDir . '/', $writtenPath); $this->assertTrue(is_file($writtenPath)); $this->assertStringEndsWith('.txt', basename($writtenPath)); $this->assertSame('payload', file_get_contents($writtenPath)); + $this->assertSame(0600, fileperms($writtenPath) & 0777); + } + + 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 diff --git a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php index 19caaada..21992a4a 100644 --- a/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php +++ b/tests/Unit/Domain/Configuration/Service/Placeholder/UserTrackValueResolverTest.php @@ -4,24 +4,14 @@ namespace PhpList\Core\Tests\Unit\Domain\Configuration\Service\Placeholder; -use PhpList\Core\Domain\Configuration\Model\ConfigOption; use PhpList\Core\Domain\Configuration\Model\Dto\PlaceholderContext; use PhpList\Core\Domain\Configuration\Model\OutputFormat; use PhpList\Core\Domain\Configuration\Service\Placeholder\UserTrackValueResolver; -use PhpList\Core\Domain\Configuration\Service\Provider\ConfigProvider; use PhpList\Core\Domain\Subscription\Model\Subscriber; -use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; final class UserTrackValueResolverTest extends TestCase { - private ConfigProvider&MockObject $config; - - protected function setUp(): void - { - $this->config = $this->createMock(ConfigProvider::class); - } - private function makeUser(string $uid = 'U-42'): Subscriber { $u = new Subscriber('user@example.com'); @@ -31,24 +21,20 @@ private function makeUser(string $uid = 'U-42'): Subscriber public function testName(): void { - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example'); $this->assertSame('USERTRACK', $resolver->name()); } public function testReturnsEmptyForTextFormat(): void { - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example'); $ctx = new PlaceholderContext(user: $this->makeUser(), format: OutputFormat::Text); $this->assertSame('', $resolver($ctx)); } - public function testHtmlUsesConfigDomainWhenAvailable(): void + public function testHtmlUsesAbsoluteRestApiBaseUrl(): void { - $this->config->method('getValue') - ->with(ConfigOption::Domain) - ->willReturn('example.com'); - - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); + $resolver = new UserTrackValueResolver('https://api.example/api/v2/'); $ctx = new PlaceholderContext( user: $this->makeUser('UID-XYZ'), @@ -59,33 +45,8 @@ public function testHtmlUsesConfigDomainWhenAvailable(): void messageId: 99, ); - $result = $resolver($ctx); - - $expected = ''; - // Normalize double quotes for comparison - $this->assertSame($expected, $result); - } - - public function testHtmlFallsBackToRestApiDomainWhenConfigMissing(): void - { - $this->config->method('getValue') - ->with(ConfigOption::Domain) - ->willReturn(null); - - $resolver = new UserTrackValueResolver($this->config, 'https://api.example'); - - $ctx = new PlaceholderContext( - user: $this->makeUser('U1'), - format: OutputFormat::Html, - messagePrecacheDto: null, - locale: 'en', - forwardedBy: null, - messageId: 7, - ); - - $result = $resolver($ctx); - - $expected = ''; - $this->assertSame($expected, $result); + $expected = ''; + $this->assertSame($expected, $resolver($ctx)); } } diff --git a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php index 1d79369a..b0aab919 100644 --- a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php +++ b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php @@ -8,7 +8,6 @@ use PhpList\Core\Domain\Configuration\Service\Provider\DefaultConfigProvider; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use ReflectionClass; use Symfony\Contracts\Translation\TranslatorInterface; /** @@ -25,6 +24,14 @@ protected function setUp(): void $this->provider = new DefaultConfigProvider($this->translator); } + public function testWebsiteAndDomainDefaultsComeFromConfiguredWebsite(): void + { + $provider = new DefaultConfigProvider($this->translator, 'www.example.com'); + + $this->assertSame('www.example.com', $provider->get(ConfigOption::Website)['value']); + $this->assertSame('example.com', $provider->get(ConfigOption::Domain)['value']); + } + public function testHasReturnsTrueForKnownKey(): void { $this->assertTrue($this->provider->has(ConfigOption::AdminAddress)); @@ -79,24 +86,23 @@ public function testUnsubscribeUrlDefaults(): void public function testTranslatorIsUsedOnlyOnFirstInit(): void { + $calls = 0; $this->translator - ->expects($this->atLeastOnce()) ->method('trans') - ->willReturnArgument(0); + ->willReturnCallback(function (string $id) use (&$calls): string { + $calls++; + + return $id; + }); $this->provider->get(ConfigOption::AdminAddress); + $callsAfterInit = $calls; + $this->assertGreaterThan(0, $callsAfterInit); // Subsequent calls should not trigger init again - $translator = $this->createMock(TranslatorInterface::class); - $translator - ->expects($this->never()) - ->method('trans'); - - $reflection = new ReflectionClass($this->provider); - $prop = $reflection->getProperty('translator'); - - $prop->setValue($this->provider, $translator); $this->provider->get(ConfigOption::UnsubscribeUrl); $this->provider->has(ConfigOption::BlacklistUrl); + + $this->assertSame($callsAfterInit, $calls); } public function testKnownKeysHaveReasonableTypes(): void diff --git a/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php b/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php index b673f442..22ef8593 100644 --- a/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php +++ b/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php @@ -19,7 +19,6 @@ use PhpList\Core\Domain\Messaging\Service\Builder\EmailBuilder; use PhpList\Core\Domain\Messaging\Service\CampaignEmailSender; use PhpList\Core\Domain\Messaging\Service\MailSizeChecker; -use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Messaging\Service\MessageStatusUpdater; use PhpList\Core\Domain\Messaging\Service\RateLimitedCampaignMailer; use PhpList\Core\Domain\Messaging\Service\SystemNotificationMailer; @@ -37,7 +36,6 @@ class CampaignEmailSenderTest extends TestCase private EmailBuilder|MockObject $campaignEmailBuilder; private RateLimitedCampaignMailer|MockObject $rateLimitedCampaignMailer; private MailSizeChecker|MockObject $mailSizeChecker; - private MessageProcessingPreparator|MockObject $messagePreparator; private MessageRepository|MockObject $messageRepository; private MessageStatusUpdater|MockObject $messageStatusUpdater; private SystemNotificationMailer|MockObject $notificationMailer; @@ -57,7 +55,6 @@ protected function setUp(): void $this->campaignEmailBuilder = $this->createMock(EmailBuilder::class); $this->rateLimitedCampaignMailer = $this->createMock(RateLimitedCampaignMailer::class); $this->mailSizeChecker = $this->createMock(MailSizeChecker::class); - $this->messagePreparator = $this->createMock(MessageProcessingPreparator::class); $this->messageRepository = $this->createMock(MessageRepository::class); $this->messageStatusUpdater = $this->createMock(MessageStatusUpdater::class); $this->notificationMailer = $this->createMock(SystemNotificationMailer::class); @@ -72,7 +69,6 @@ protected function setUp(): void $this->campaignEmailBuilder, $this->rateLimitedCampaignMailer, $this->mailSizeChecker, - $this->messagePreparator, $this->messageRepository, $this->messageStatusUpdater, $this->notificationMailer, @@ -92,8 +88,6 @@ protected function setUp(): void $this->subscriber->method('getEmail')->willReturn('test@example.com'); $this->userMessage = $this->createMock(UserMessage::class); - - $this->messagePreparator->method('processMessageLinks')->willReturn($this->precached); } public function testSendMarksSentAndIncrementsCountsOnSuccess(): void diff --git a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php index df8afd18..d463ac9e 100644 --- a/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Constructor/CampaignMailContentBuilderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service\Constructor; +use PhpList\Core\Domain\Analytics\Service\LinkTrackService; use PhpList\Core\Domain\Common\Html2Text; use PhpList\Core\Domain\Common\RemotePageFetcher; use PhpList\Core\Domain\Common\TextParser; @@ -28,6 +29,7 @@ class CampaignMailContentBuilderTest extends TestCase private Html2Text&MockObject $html2Text; private TextParser&MockObject $textParser; private MessagePlaceholderProcessor&MockObject $placeholderProcessor; + private LinkTrackService&MockObject $linkTrackService; protected function setUp(): void { @@ -47,6 +49,10 @@ protected function setUp(): void ->onlyMethods(['__invoke']) ->getMock(); $this->placeholderProcessor = $this->createMock(MessagePlaceholderProcessor::class); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->method('trackLinks')->willReturnCallback( + static fn (string $html, string $text): array => [$html, $text] + ); $this->configProvider ->method('getValue') @@ -67,6 +73,7 @@ private function makeBuilder(): CampaignMailContentBuilder html2Text: $this->html2Text, textParser: $this->textParser, placeholderProcessor: $this->placeholderProcessor, + linkTrackService: $this->linkTrackService, ); } @@ -276,4 +283,75 @@ static function (...$args): string { 'No text content provided and html2text not used when htmlFormatted and template present' ); } + + private function makeSubscriber(): Subscriber&MockObject + { + $subscriber = $this->getMockBuilder(Subscriber::class) + ->disableOriginalConstructor() + ->onlyMethods(['getId', 'getEmail']) + ->getMock(); + $subscriber->method('getId')->willReturn(5); + $subscriber->method('getEmail')->willReturn('user@example.com'); + $this->placeholderProcessor + ->method('process') + ->willReturnCallback(static fn (...$args): string => (string) $args[0] . '[expanded]'); + + return $subscriber; + } + + public function testLinksAreTrackedAfterPlaceholdersAreExpanded(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber) + ->willReturn(['

tracked

', 'tracked text']); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + + [$html, $text] = ($this->makeBuilder())($dto, $subscriber, 9); + + $this->assertStringContainsString('

tracked

', $html); + $this->assertSame('tracked text', $text); + } + + public function testForwardedMessagesAreMarkedAsForwardedForLinkTracking(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber, null, true) + ->willReturn(['

Hi

', 'Hi']); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + + ($this->makeBuilder())($dto, $subscriber, 9, $this->createMock(Subscriber::class)); + } + + public function testAnalyticsParametersAreHandedOverWhenTheCampaignTracksAnalytics(): void + { + $subscriber = $this->makeSubscriber(); + $this->linkTrackService = $this->createMock(LinkTrackService::class); + $this->linkTrackService->expects($this->once()) + ->method('trackLinks') + ->with('

Hi

[expanded]', 'Hi[expanded]', 9, $subscriber, ['utm_source' => 'news'], false) + ->willReturn(['

Hi

', 'Hi']); + + $dto = new MessagePrecacheDto(); + $dto->content = '

Hi

'; + $dto->textContent = 'Hi'; + $dto->htmlFormatted = true; + $dto->googleTrack = true; + $dto->analyticsParameters = ['utm_source' => 'news']; + + ($this->makeBuilder())($dto, $subscriber, 9); + } } 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..f598d636 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php @@ -4,9 +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; use PhpList\Core\Domain\Messaging\Service\MessageProcessingPreparator; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -21,7 +18,6 @@ class MessageProcessingPreparatorTest extends TestCase { private SubscriberRepository&MockObject $subscriberRepository; private MessageRepository&MockObject $messageRepository; - private LinkTrackService&MockObject $linkTrackService; private OutputInterface&MockObject $output; private MessageProcessingPreparator $preparator; @@ -29,13 +25,11 @@ protected function setUp(): void { $this->subscriberRepository = $this->createMock(SubscriberRepository::class); $this->messageRepository = $this->createMock(MessageRepository::class); - $this->linkTrackService = $this->createMock(LinkTrackService::class); $this->output = $this->createMock(OutputInterface::class); $this->preparator = new MessageProcessingPreparator( subscriberRepository: $this->subscriberRepository, messageRepository: $this->messageRepository, - linkTrackService: $this->linkTrackService, translator: new Translator('en'), ); } @@ -115,88 +109,4 @@ public function testEnsureCampaignsHaveUuidWithCampaigns(): void $this->preparator->ensureCampaignsHaveUuid($this->output); } - - public function testProcessMessageLinksWhenLinkTrackingNotApplicable(): 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') - ->willReturn(false); - - $this->linkTrackService->expects($this->never()) - ->method('extractAndSaveLinks'); - - $result = $this->preparator->processMessageLinks(1, $messageContent, $subscriber); - - $this->assertSame($messageContent, $result); - } - - public function testProcessMessageLinksWhenNoLinksExtracted(): 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([]); - - $result = $this->preparator->processMessageLinks($messageId, $messageContent, $subscriber); - - $this->assertSame($messageContent, $result); - } - - public function testProcessMessageLinksWithLinksExtracted(): void - { - $content = 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'); - - $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'; - - $result = $this->preparator->processMessageLinks(1, $content, $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 - ); - } } diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php index 3c7ae4b3..5bb70054 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvExporterTest.php @@ -11,6 +11,7 @@ use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Resolver\AttributeValueResolver; use PhpList\Core\Domain\Subscription\Service\SubscriberCsvExporter; @@ -37,7 +38,8 @@ protected function setUp(): void $this->createMock(AttributeValueResolver::class), $this->subscriberRepositoryMock, $this->attributeDefinitionRepositoryMock, - $this->createMock(LoggerInterface::class) + $this->createMock(LoggerInterface::class), + $this->createMock(UserBlacklistRepository::class), ); } diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 1b2a7bf6..ec46f9c1 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -17,6 +17,7 @@ use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriptionManager; +use PhpList\Core\Domain\Subscription\Repository\UserBlacklistRepository; use PhpList\Core\Domain\Subscription\Service\SubscriberCsvImporter; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -31,28 +32,34 @@ class SubscriberCsvImporterTest extends TestCase private SubscriberRepository&MockObject $subscriberRepositoryMock; private CsvToDtoImporter&MockObject $csvImporterMock; private SubscriberAttributeDefinitionRepository&MockObject $attributeDefinitionRepositoryMock; + private UserBlacklistRepository&MockObject $blacklistRepositoryMock; + private SubscriptionManager&MockObject $subscriptionManagerMock; + private MessageBusInterface&MockObject $messageBusMock; private SubscriberCsvImporter $subject; protected function setUp(): void { $this->subscriberManagerMock = $this->createMock(SubscriberManager::class); $this->attributeManagerMock = $this->createMock(SubscriberAttributeManager::class); - $subscriptionManagerMock = $this->createMock(SubscriptionManager::class); + $this->subscriptionManagerMock = $this->createMock(SubscriptionManager::class); $this->subscriberRepositoryMock = $this->createMock(SubscriberRepository::class); $this->csvImporterMock = $this->createMock(CsvToDtoImporter::class); $this->attributeDefinitionRepositoryMock = $this->createMock(SubscriberAttributeDefinitionRepository::class); + $this->messageBusMock = $this->createMock(MessageBusInterface::class); $entityManager = $this->createMock(EntityManagerInterface::class); + $this->blacklistRepositoryMock = $this->createMock(UserBlacklistRepository::class); $this->subject = new SubscriberCsvImporter( subscriberManager: $this->subscriberManagerMock, attributeManager: $this->attributeManagerMock, - subscriptionManager: $subscriptionManagerMock, + subscriptionManager: $this->subscriptionManagerMock, subscriberRepository: $this->subscriberRepositoryMock, csvToDtoImporter: $this->csvImporterMock, entityManager: $entityManager, translator: new Translator('en'), - messageBus: $this->createMock(MessageBusInterface::class), + messageBus: $this->messageBusMock, subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class), + blacklistRepository: $this->blacklistRepositoryMock, ); } @@ -97,7 +104,6 @@ public function testImportFromCsvCreatesNewSubscribers(): void $importDto1 = new ImportSubscriberDto( email: 'test@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, ); @@ -108,7 +114,6 @@ public function testImportFromCsvCreatesNewSubscribers(): void $importDto2 = new ImportSubscriberDto( email: 'another@example.com', confirmed: false, - blacklisted: true, htmlEmail: false, disabled: true ); @@ -204,7 +209,6 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void $importDto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, ); @@ -274,7 +278,6 @@ public function testImportResolvesByForeignKeyWhenProvidedAndMatches(): void $dto = new ImportSubscriberDto( email: 'user@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -339,7 +342,6 @@ public function testImportConflictWhenEmailAndForeignKeyReferToDifferentSubscrib $dto = new ImportSubscriberDto( email: 'conflict@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -399,7 +401,6 @@ public function testImportResolvesByEmailWhenForeignKeyNotFound(): void $dto = new ImportSubscriberDto( email: 'existing@example.com', confirmed: true, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -459,7 +460,6 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void $dto = new ImportSubscriberDto( email: 'new@example.com', confirmed: false, - blacklisted: false, htmlEmail: true, disabled: false, extraData: null, @@ -499,4 +499,87 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void unlink($tempFile); } + + public function testImportCountsSubscriberWhoseEmailIsInBlacklistTable(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_blacklist'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled\nbl@example.com,0,1,0,0\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $this->subscriberRepositoryMock->method('findOneByEmail')->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'bl@example.com', + confirmed: false, + htmlEmail: true, + disabled: false, + ); + $this->csvImporterMock->method('parseAndValidate')->willReturn(['valid' => [$dto], 'errors' => []]); + + $created = $this->createMock(Subscriber::class); + $created->method('getId')->willReturn(7); + $created->method('getEmail')->willReturn('bl@example.com'); + $created->method('isBlacklisted')->willReturn(false); + $created->expects($this->once())->method('setBlacklisted')->with(true); + $this->subscriberManagerMock->method('createFromImport')->willReturn($created); + + $this->blacklistRepositoryMock + ->expects($this->once()) + ->method('isEmailBlacklisted') + ->with('bl@example.com') + ->willReturn(true); + + $this->subscriptionManagerMock->expects($this->never())->method('addSubscriberToAList'); + $this->messageBusMock->expects($this->never())->method('dispatch'); + + $result = $this->subject->importFromCsv( + $uploadedFile, + new SubscriberImportOptions(updateExisting: false, listIds: [1, 2]) + ); + + $this->assertSame(1, $result['blacklisted']); + + unlink($tempFile); + } + + public function testImportIgnoresBlacklistedFlagWhenEmailIsNotInBlacklistTable(): void + { + $tempFile = tempnam(sys_get_temp_dir(), 'csv_test_flag_only'); + file_put_contents($tempFile, "email,confirmed,html_email,blacklisted,disabled\nflag@example.com,1,1,1,0\n"); + + $uploadedFile = $this->createMock(UploadedFile::class); + $uploadedFile->method('getRealPath')->willReturn($tempFile); + + $this->subscriberRepositoryMock->method('findOneByEmail')->willReturn(null); + + $dto = new ImportSubscriberDto( + email: 'flag@example.com', + confirmed: true, + htmlEmail: true, + disabled: false, + ); + $this->csvImporterMock->method('parseAndValidate')->willReturn(['valid' => [$dto], 'errors' => []]); + + $created = $this->createMock(Subscriber::class); + $created->method('getId')->willReturn(8); + $created->method('getEmail')->willReturn('flag@example.com'); + $created->method('isBlacklisted')->willReturn(true); + $created->expects($this->never())->method('setBlacklisted'); + $this->subscriberManagerMock->method('createFromImport')->willReturn($created); + + $this->blacklistRepositoryMock->method('isEmailBlacklisted')->willReturn(false); + + $this->subscriptionManagerMock->expects($this->once())->method('addSubscriberToAList'); + + $result = $this->subject->importFromCsv( + $uploadedFile, + new SubscriberImportOptions(updateExisting: false, listIds: [1]) + ); + + $this->assertSame(0, $result['blacklisted']); + + unlink($tempFile); + } }