diff --git a/.env.dist b/.env.dist index 07a41b6a8..80161df9a 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/.github/workflows/ci.yml b/.github/workflows/ci.yml index 865965232..986afd9cf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,7 +33,7 @@ jobs: strategy: fail-fast: false matrix: - php-versions: ['8.1'] + php-versions: ['8.2'] steps: - name: Checkout uses: actions/checkout@v3 diff --git a/.github/workflows/core-docs.yml b/.github/workflows/core-docs.yml index dc3a5bdfc..c9de94268 100644 --- a/.github/workflows/core-docs.yml +++ b/.github/workflows/core-docs.yml @@ -10,7 +10,7 @@ jobs: - name: Setup PHP, with composer and extensions uses: shivammathur/setup-php@v2 with: - php-version: 8.1 + php-version: 8.2 extensions: mbstring, dom, fileinfo, mysql - name: Get composer cache directory id: composer-cache diff --git a/.github/workflows/i18n-validate.yml b/.github/workflows/i18n-validate.yml index 4e49efa84..b9f88131e 100644 --- a/.github/workflows/i18n-validate.yml +++ b/.github/workflows/i18n-validate.yml @@ -14,7 +14,7 @@ jobs: strategy: fail-fast: false matrix: - php: ['8.1'] + php: ['8.2'] steps: - name: Checkout diff --git a/composer.json b/composer.json index 5c6f9e059..3f2f2db7f 100644 --- a/composer.json +++ b/composer.json @@ -45,7 +45,7 @@ "source": "https://github.com/phpList/core" }, "require": { - "php": "^8.1", + "php": "^8.2", "symfony/framework-bundle": "^6.4", "symfony/dependency-injection": "^6.4", "symfony/config": "^6.4", diff --git a/config/parameters.yml b/config/parameters.yml index 28d4ed9cc..d902b6395 100644 --- a/config/parameters.yml +++ b/config/parameters.yml @@ -20,9 +20,12 @@ parameters: app.dev_email: '%env(APP_DEV_EMAIL)%' app.powered_by_phplist: '%env(APP_POWERED_BY_PHPLIST)%' app.preference_page_show_private_lists: '%env(PREFERENCEPAGE_SHOW_PRIVATE_LISTS)%' - app.rest_api_base_url: '%env(API_BASE_URL)%/api/v2' + 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.yml b/config/services.yml index bfb068236..c24bd36ff 100644 --- a/config/services.yml +++ b/config/services.yml @@ -55,6 +55,10 @@ services: arguments: $tablePrefix: '%database.prefix%' + PhpList\Core\Core\Doctrine\SkippedMigrationRecorder: + autowire: true + autoconfigure: true + PhpList\Core\Core\Doctrine\SearchIndexDoctrineListener: arguments: $enabled: '%elasticsearch.enabled%' diff --git a/config/services/parameters.yml b/config/services/parameters.yml index 18aa6ccf9..9a9b4aea5 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 481b23a31..830abb143 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 959522801..739b52543 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 ac841afa6..5b19db155 100644 --- a/config/services/services.yml +++ b/config/services/services.yml @@ -26,6 +26,10 @@ services: autowire: true autoconfigure: true + PhpList\Core\Domain\Messaging\EventSubscriber\InjectedByHeaderSubscriber: + autowire: true + autoconfigure: true + PhpList\Core\Domain\Messaging\Service\ForwardContentService: autowire: true autoconfigure: true @@ -61,6 +65,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 @@ -249,6 +268,11 @@ services: autoconfigure: true public: true + PhpList\Core\Domain\Identity\Service\Notification\: + resource: '../../src/Domain/Identity/Service/Notification/*.php' + autowire: true + autoconfigure: true + PhpList\Core\Domain\Identity\Service\AdminNotifier: autowire: true autoconfigure: true diff --git a/public/favicon-48.png b/public/favicon-48.png index 215a9efe8..83e65adb0 100644 Binary files a/public/favicon-48.png and b/public/favicon-48.png differ diff --git a/public/favicon.ico b/public/favicon.ico index 5a82bbab7..e219715e3 100644 Binary files a/public/favicon.ico and b/public/favicon.ico differ diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index 03d7912e8..f3f356e30 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -394,7 +394,7 @@ Please confirm your subscription by clicking the link below: %confirmation_link% If you did not request this subscription, please ignore this email. - __Thank you for subscribing! + Thank you for subscribing! Please confirm your subscription by clicking the link below: @@ -404,7 +404,7 @@ If you did not request this subscription, please ignore this email. <p>Thank you for subscribing!</p><p>Please confirm your subscription by clicking the link below:</p><p><a href="%confirmation_link%">Confirm Subscription</a></p><p>If you did not request this subscription, please ignore this email.</p> - Thank you for subscribing!

Please confirm your subscription by clicking the link below:

Confirm Subscription

If you did not request this subscription, please ignore this email.

]]> + Thank you for subscribing!

Please confirm your subscription by clicking the link below:

Confirm Subscription

If you did not request this subscription, please ignore this email.

]]> Hello, @@ -417,7 +417,7 @@ Please use the following token to reset your password: If you did not request this password reset, please ignore this email. Thank you. - __Hello, + Hello, A password reset has been requested for your account. Please use the following token to reset your password: @@ -430,417 +430,417 @@ Thank you. <p>Password Reset Request!</p><p>Hello! A password reset has been requested for your account.</p><p>Please use the following token to reset your password:</p><p><a href="%confirmation_link%">Reset Password</a></p><p>If you did not request this password reset, please ignore this email.</p><p>Thank you.</p> - Password Reset Request!

Hello! A password reset has been requested for your account.

Please use the following token to reset your password:

Reset Password

If you did not request this password reset, please ignore this email.

Thank you.

]]> + Password Reset Request!

Hello! A password reset has been requested for your account.

Please use the following token to reset your password:

Reset Password

If you did not request this password reset, please ignore this email.

Thank you.

]]> Person in charge of this system (one email address) - __Person in charge of this system (one email address) + Person in charge of this system (one email address) Name of the organisation - __Name of the organisation + Name of the organisation Logo of the organisation - __Logo of the organisation + Logo of the organisation Date format - __Date format + Date format Show notification for Release Candidates - __Show notification for Release Candidates + Show notification for Release Candidates Secret for remote processing - __Secret for remote processing + Secret for remote processing Notify admin on login from new location - __Notify admin on login from new location + Notify admin on login from new location List of email addresses to CC in system messages (separate by commas) - __List of email addresses to CC in system messages (separate by commas) + List of email addresses to CC in system messages (separate by commas) Default for 'From:' in a campaign - __Default for 'From:' in a campaign + Default for 'From:' in a campaign Default for 'address to alert when sending starts' - __Default for 'address to alert when sending starts' + Default for 'address to alert when sending starts' Default for 'address to alert when sending finishes' - __Default for 'address to alert when sending finishes' + Default for 'address to alert when sending finishes' Always add analytics tracking code to campaigns - __Always add analytics tracking code to campaigns + Always add analytics tracking code to campaigns Analytics tracking code to add to campaign URLs - __Analytics tracking code to add to campaign URLs + Analytics tracking code to add to campaign URLs Who gets the reports (email address, separate multiple emails with a comma) - __Who gets the reports (email address, separate multiple emails with a comma) + Who gets the reports (email address, separate multiple emails with a comma) From email address for system messages - __From email address for system messages + From email address for system messages Webmaster - __Webmaster + Webmaster Name for system messages - __Name for system messages + Name for system messages Reply-to email address for system messages - __Reply-to email address for system messages + Reply-to email address for system messages If there is only one visible list, should it be hidden in the page and automatically subscribe users who sign up - __If there is only one visible list, should it be hidden in the page and automatically subscribe users who sign up + If there is only one visible list, should it be hidden in the page and automatically subscribe users who sign up Categories for lists. Separate with commas. - __Categories for lists. Separate with commas. + Categories for lists. Separate with commas. Display list categories on subscribe page - __Display list categories on subscribe page + Display list categories on subscribe page Width of a textline field (numerical) - __Width of a textline field (numerical) + Width of a textline field (numerical) Dimensions of a textarea field (rows,columns) - __Dimensions of a textarea field (rows,columns) + Dimensions of a textarea field (rows,columns) Send notifications about subscribe, update and unsubscribe - __Send notifications about subscribe, update and unsubscribe + Send notifications about subscribe, update and unsubscribe The default subscribe page when there are multiple - __The default subscribe page when there are multiple + The default subscribe page when there are multiple The default HTML template to use when sending a message - __The default HTML template to use when sending a message + The default HTML template to use when sending a message The HTML wrapper template for system messages - __The HTML wrapper template for system messages + The HTML wrapper template for system messages URL where subscribers can sign up - __URL where subscribers can sign up + URL where subscribers can sign up URL where subscribers can unsubscribe - __URL where subscribers can unsubscribe + URL where subscribers can unsubscribe URL where unknown users can unsubscribe (do-not-send-list) - __URL where unknown users can unsubscribe (do-not-send-list) + URL where unknown users can unsubscribe (do-not-send-list) URL where subscribers have to confirm their subscription - __URL where subscribers have to confirm their subscription + URL where subscribers have to confirm their subscription URL where subscribers can update their details - __URL where subscribers can update their details + URL where subscribers can update their details URL for forwarding messages - __URL for forwarding messages + URL for forwarding messages URL for downloading vcf card - __URL for downloading vcf card + URL for downloading vcf card <h3>Thanks, you have been added to our newsletter</h3><p>You will receive an email to confirm your subscription. Please click the link in the email to confirm</p> - Thanks, you have been added to our newsletter

You will receive an email to confirm your subscription. Please click the link in the email to confirm

]]> + Thanks, you have been added to our newsletter

You will receive an email to confirm your subscription. Please click the link in the email to confirm

]]> Text to display when subscription with an AJAX request was successful - __Text to display when subscription with an AJAX request was successful + Text to display when subscription with an AJAX request was successful Subject of the message subscribers receive when they sign up - __Subject of the message subscribers receive when they sign up + Subject of the message subscribers receive when they sign up Message subscribers receive when they sign up - __Message subscribers receive when they sign up + Message subscribers receive when they sign up Goodbye from our Newsletter - __Goodbye from our Newsletter + Goodbye from our Newsletter Subject of the message subscribers receive when they unsubscribe - __Subject of the message subscribers receive when they unsubscribe + Subject of the message subscribers receive when they unsubscribe Message subscribers receive when they unsubscribe - __Message subscribers receive when they unsubscribe + Message subscribers receive when they unsubscribe Welcome to our Newsletter - __Welcome to our Newsletter + Welcome to our Newsletter Subject of the message subscribers receive after confirming their email address - __Subject of the message subscribers receive after confirming their email address + Subject of the message subscribers receive after confirming their email address Message subscribers receive after confirming their email address - __Message subscribers receive after confirming their email address + Message subscribers receive after confirming their email address [notify] Change of List-Membership details - __[notify] Change of List-Membership details + [notify] Change of List-Membership details Subject of the message subscribers receive when they have changed their details - __Subject of the message subscribers receive when they have changed their details + Subject of the message subscribers receive when they have changed their details Message subscribers receive when they have changed their details - __Message subscribers receive when they have changed their details + Message subscribers receive when they have changed their details Part of the message that is sent to their new email address when subscribers change their information, and the email address has changed - __Part of the message that is sent to their new email address when subscribers change their information, and the email address has changed + Part of the message that is sent to their new email address when subscribers change their information, and the email address has changed Part of the message that is sent to their old email address when subscribers change their information, and the email address has changed - __Part of the message that is sent to their old email address when subscribers change their information, and the email address has changed + Part of the message that is sent to their old email address when subscribers change their information, and the email address has changed Your personal location - __Your personal location + Your personal location Subject of message when subscribers request their personal location - __Subject of message when subscribers request their personal location + Subject of message when subscribers request their personal location Default footer for sending a campaign - __Default footer for sending a campaign + Default footer for sending a campaign Footer used when a message has been forwarded - __Footer used when a message has been forwarded + Footer used when a message has been forwarded Message to send when they request their personal location - __Message to send when they request their personal location + Message to send when they request their personal location String to always append to remote URL when using send-a-webpage - __String to always append to remote URL when using send-a-webpage + String to always append to remote URL when using send-a-webpage Width for Wordwrap of Text messages - __Width for Wordwrap of Text messages + Width for Wordwrap of Text messages CSS for HTML messages without a template - __CSS for HTML messages without a template + CSS for HTML messages without a template Domains that only accept text emails, one per line - __Domains that only accept text emails, one per line + Domains that only accept text emails, one per line last time TLDs were fetched - __last time TLDs were fetched + last time TLDs were fetched Top level domains - __Top level domains + Top level domains Header of public pages. - __Header of public pages. + Header of public pages. Footer of public pages - __Footer of public pages + Footer of public pages Please confirm your subscription - __Please confirm your subscription + Please confirm your subscription No user details changed - __No user details changed + No user details changed %field% = %new% *changed* from %old% - __%field% = %new% *changed* from %old% + %field% = %new% *changed* from %old% Subscribed to %list% - __Subscribed to %list% + Subscribed to %list% Subscriber marked unconfirmed for invalid email address - __Subscriber marked unconfirmed for invalid email address + Subscriber marked unconfirmed for invalid email address Marked unconfirmed while sending campaign %message_id% - __Marked unconfirmed while sending campaign %message_id% + Marked unconfirmed while sending campaign %message_id% Update by %admin% - __Update by %admin% + Update by %admin% (no data) - __(no data) + (no data) %attribute% = %new_value% changed from %old_value% - __%attribute% = %new_value% + %attribute% = %new_value% changed from %old_value% No data changed - __No data changed + No data changed Campaign not found or not in submitted status - __Campaign not found or not in submitted status + Campaign not found or not in submitted status Conflict: email and foreign key refer to different subscribers. - __Conflict: email and foreign key refer to different subscribers. + Conflict: email and foreign key refer to different subscribers. Invalid email: %email% - __Invalid email: %email% + Invalid email: %email% Value must be an AttributeTypeEnum or string. - __Value must be an AttributeTypeEnum or string. + Value must be an AttributeTypeEnum or string. Campaign started - __Campaign started + Campaign started phplist has started sending the campaign with subject %s - __phplist has started sending the campaign with subject %s + phplist has started sending the campaign with subject %s phplist has started sending the campaign with subject %subject% - __phplist has started sending the campaign with subject %subject% + phplist has started sending the campaign with subject %subject% Unsubscribe - __Unsubscribe + Unsubscribe This link - __This link + This link Confirm - __Confirm + Confirm Update preferences - __Update preferences + Update preferences Sorry, you are not subscribed to any of our newsletters with this email address. - __Sorry, you are not subscribed to any of our newsletters with this email address. + Sorry, you are not subscribed to any of our newsletters with this email address. This message contains attachments that can be viewed with a webbrowser - __This message contains attachments that can be viewed with a webbrowser + This message contains attachments that can be viewed with a webbrowser Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit% - __Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit% + Insufficient memory to add attachment to campaign %campaignId% %totalSize% - %memLimit% Add us to your address book - __Add us to your address book + Add us to your address book phpList system error - __phpList system error + phpList system error Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions. - __Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions. + Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be copied to the repository. Check for permissions. failed to open attachment (%remoteFile%) to add to campaign %campaignId% - __failed to open attachment (%remoteFile%) to add to campaign %campaignId% + failed to open attachment (%remoteFile%) to add to campaign %campaignId% Attachment %remoteFile% does not exist - __Attachment %remoteFile% does not exist + Attachment %remoteFile% does not exist Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository. - __Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository. + Error, when trying to send campaign %campaignId% the attachment (%remoteFile%) could not be found in the repository. Location - __Location + Location Fwd - __Fwd + Fwd (test) - __(test) + (test) Message Forwarded - __Message Forwarded + Message Forwarded %subscriber% tried forwarding message %campaignId% to %email% but failed - __%subscriber% tried forwarding message %campaignId% to %email% but failed + %subscriber% tried forwarding message %campaignId% to %email% but failed %subscriber% has forwarded message %campaignId% to %email% - __%subscriber% has forwarded message %campaignId% to %email% + %subscriber% has forwarded message %campaignId% to %email% Error precaching campaign message: %error% - __Error precaching campaign message: %error% + Error precaching campaign message: %error% phplist system error - __phplist system error + phplist system error Failed to send to: %subscriber_id% - __Failed to send to: %subscriber_id% + Failed to send to: %subscriber_id% Invalid email, marking unconfirmed: %subscriber_id% - __Invalid email, marking unconfirmed: %subscriber_id% + Invalid email, marking unconfirmed: %subscriber_id% diff --git a/src/Core/Doctrine/SkippedMigrationRecorder.php b/src/Core/Doctrine/SkippedMigrationRecorder.php new file mode 100644 index 000000000..7efadcf58 --- /dev/null +++ b/src/Core/Doctrine/SkippedMigrationRecorder.php @@ -0,0 +1,43 @@ +getPlan(); + $result = $plan->getResult(); + + if ($result === null + || $plan->getDirection() !== Direction::UP + || $args->getMigratorConfiguration()->isDryRun() + ) { + return; + } + + $this->dependencyFactory->getMetadataStorage()->complete($result); + } +} diff --git a/src/Domain/Analytics/Model/Dto/LinkTrackToken.php b/src/Domain/Analytics/Model/Dto/LinkTrackToken.php new file mode 100644 index 000000000..4b6746a65 --- /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 66240feea..f5eeb46c4 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 38de3f747..1db61194e 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 6914105b6..3e1292b41 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 5662d6722..2f3254c9e 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 000000000..a9e19ecf5 --- /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 000000000..a6d48ab28 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkClickService.php @@ -0,0 +1,105 @@ + $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); + 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. Like in + // phplist3, a subscriber without a record of receiving the campaign (a test) counts every time. + 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): ?string + { + $url = $forward->getUrl(); + if ($url === null) { + return null; + } + + if ($forward->isPersonalise()) { + // 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 000000000..8da1e3057 --- /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 dc478e84b..7153c40ac 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 000000000..8e3f86169 --- /dev/null +++ b/src/Domain/Analytics/Service/LinkTrackTokenService.php @@ -0,0 +1,167 @@ +/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. + * + * Tokens of links in campaigns sent by phplist3 can be parsed as well, so that those links keep working when + * lt.php is served by this application. Their HTML tokens are masked with the `xormask` setting, which is shared + * through the config table; their text tokens are the three uuids packed as binary. phplist3 signs its links with + * a hash of the whole url, which cannot be checked here, so with signing enabled they are refused. + */ +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, + private readonly ConfigProvider $configProvider, + ) { + } + + 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; + } + + return $this->parseOwnFormat($tid) ?? $this->parsePhplist3Format($tid); + } + + private function parseOwnFormat(string $tid): ?LinkTrackToken + { + $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]); + } + + /** The tokens of phplist3's lt.php, see its sendemaillib.php. */ + private function parsePhplist3Format(string $tid): ?LinkTrackToken + { + // a query string turns the + of the base64 into a space + $tid = str_replace(' ', '+', $tid); + + if (strlen($tid) === 64) { + $track = $this->unpackPhplist3TextToken($tid); + } else { + $track = $this->unmaskPhplist3HtmlToken($tid); + } + + $uuid = '([a-f0-9]{8}-?[a-f0-9]{4}-?[a-f0-9]{4}-?[89ab][a-f0-9]{3}-?[a-f0-9]{12})'; + $pattern = '/^(H|T)\|' . $uuid . '\|' . $uuid . '\|' . $uuid . '$/'; + if ($track === null || !preg_match($pattern, $track, $matches)) { + return null; + } + + return new LinkTrackToken($matches[1], $matches[2], $matches[3], $matches[4]); + } + + /** Text links carry the forward, message and subscriber uuids as 48 binary bytes. */ + private function unpackPhplist3TextToken(string $tid): ?string + { + $binary = base64_decode($tid, true); + if ($binary === false || strlen($binary) !== 48) { + return null; + } + + $hex = bin2hex($binary); + $uuids = []; + foreach ([0, 32, 64] as $offset) { + // the version nibble of the uuid was replaced by random data, uuids are version 4 + $uuids[] = substr($hex, $offset, 8) . '-' . substr($hex, $offset + 8, 4) + . '-4' . substr($hex, $offset + 13, 3) . '-' . substr($hex, $offset + 16, 4) + . '-' . substr($hex, $offset + 20, 12); + } + + return 'T|' . implode('|', $uuids); + } + + /** HTML links carry `H|forward|message|subscriber` XOR-ed with the repeated xormask setting. */ + private function unmaskPhplist3HtmlToken(string $tid): ?string + { + $xorMask = $this->configProvider->getValue(ConfigOption::XorMask); + $masked = base64_decode($tid); + if ($xorMask === null || $xorMask === '' || $masked === '') { + return null; + } + + return $masked ^ str_repeat($xorMask, 20); + } + + 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 000000000..befaab1a4 --- /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 000000000..1ad2a4c28 --- /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 000000000..ce975b674 --- /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/Analytics/Service/UserMessageService.php b/src/Domain/Analytics/Service/UserMessageService.php index c5a1cc5b0..7f4694e04 100644 --- a/src/Domain/Analytics/Service/UserMessageService.php +++ b/src/Domain/Analytics/Service/UserMessageService.php @@ -29,12 +29,9 @@ public function trackUserMessageView(string $uid, int $messageId, array $metadat return; } + // like phplist3, a view is counted even without a record of the campaign being sent to the subscriber $userMessage = $this->userMessageRepository->findByUserAndMessage($subscriber, $message); - if ($userMessage === null) { - return; - } - - $userMessage->setViewedNow(); + $userMessage?->setViewedNow(); $message->getMetadata()->incrementViews(); $data = []; diff --git a/src/Domain/Common/FileHelper.php b/src/Domain/Common/FileHelper.php index b8995b357..fed3649f0 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 fdeb01db8..5605c96dc 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 63d9f35cf..4f2314e2d 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 bc731088c..b08bedb41 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 1c4a33764..dda10f8f0 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'; @@ -47,4 +47,5 @@ enum ConfigOption: string case AutoCreateAttributes = 'autocreate_attributes'; case RemoteProcessingSecret = 'remote_processing_secret'; case NotifyAdminLogin = 'notify_admin_login'; + case XorMask = 'xormask'; } diff --git a/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php b/src/Domain/Configuration/Service/Placeholder/UserTrackValueResolver.php index 8a5b626d4..f99efa2c0 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/ConfigProvider.php b/src/Domain/Configuration/Service/Provider/ConfigProvider.php index 2890a86d8..ed182c0bd 100644 --- a/src/Domain/Configuration/Service/Provider/ConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/ConfigProvider.php @@ -65,11 +65,27 @@ public function getValue(ConfigOption $key): ?string $this->cache->set($cacheKey, $value, $this->ttlSeconds); } - if ($value !== null) { + if ($value === null && $this->defaultConfigs->has($key)) { + $value = (string) $this->defaultConfigs->get($key)['value']; + } + + return $value === null ? null : $this->replaceBuiltInPlaceholders($value, $key); + } + + private function replaceBuiltInPlaceholders(string $value, ConfigOption $key): string + { + if ($key === ConfigOption::Website || $key === ConfigOption::Domain) { return $value; } - return $this->defaultConfigs->has($key) ? (string) $this->defaultConfigs->get($key)['value'] : null; + if (stripos($value, '[WEBSITE]') !== false) { + $value = str_ireplace('[WEBSITE]', $this->getValue(ConfigOption::Website) ?? '', $value); + } + if (stripos($value, '[DOMAIN]') !== false) { + $value = str_ireplace('[DOMAIN]', $this->getValue(ConfigOption::Domain) ?? '', $value); + } + + return $value; } public function getValueWithNamespace(ConfigOption|string $key): ?string diff --git a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php index 3222acc98..c1aea578e 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/Identity/Model/AccessLevel.php b/src/Domain/Identity/Model/AccessLevel.php new file mode 100644 index 000000000..e706a64da --- /dev/null +++ b/src/Domain/Identity/Model/AccessLevel.php @@ -0,0 +1,12 @@ +getId() === null) { + $owner = $resource->getOwner(); + if ($this->getId() === null || $owner === null) { return false; } - return $resource->getOwner()->getId() === $this->getId(); + return $owner->getId() === $this->getId(); } #[ORM\PreUpdate] diff --git a/src/Domain/Identity/Model/Dto/AdminNotification.php b/src/Domain/Identity/Model/Dto/AdminNotification.php new file mode 100644 index 000000000..c2b803602 --- /dev/null +++ b/src/Domain/Identity/Model/Dto/AdminNotification.php @@ -0,0 +1,26 @@ + $context machine readable details, e.g. counts + */ + public function __construct( + public readonly string $code, + public readonly string $level, + public readonly string $message, + public readonly array $context = [], + ) { + } +} diff --git a/src/Domain/Identity/Service/Notification/AdminNotificationCollector.php b/src/Domain/Identity/Service/Notification/AdminNotificationCollector.php new file mode 100644 index 000000000..776fa0b9c --- /dev/null +++ b/src/Domain/Identity/Service/Notification/AdminNotificationCollector.php @@ -0,0 +1,31 @@ + $providers */ + public function __construct( + #[AutowireIterator('phplist.admin_notification_provider')] private readonly iterable $providers, + ) { + } + + /** @return AdminNotification[] */ + public function collect(Administrator $administrator): array + { + $notifications = []; + foreach ($this->providers as $provider) { + foreach ($provider->getNotifications($administrator) as $notification) { + $notifications[] = $notification; + } + } + + return $notifications; + } +} diff --git a/src/Domain/Identity/Service/Notification/AdminNotificationProviderInterface.php b/src/Domain/Identity/Service/Notification/AdminNotificationProviderInterface.php new file mode 100644 index 000000000..b2d7474ce --- /dev/null +++ b/src/Domain/Identity/Service/Notification/AdminNotificationProviderInterface.php @@ -0,0 +1,16 @@ +isSuperUser()) { + return []; + } + + if (is_dir($this->attachmentRepositoryPath) && is_writable($this->attachmentRepositoryPath)) { + return []; + } + + return [ + new AdminNotification( + code: 'attachment_repository_not_writable', + level: AdminNotification::LEVEL_WARNING, + message: $this->translator->trans('The attachment repository does not exist or is not writable'), + ), + ]; + } +} diff --git a/src/Domain/Identity/Service/Notification/DevModeNotificationProvider.php b/src/Domain/Identity/Service/Notification/DevModeNotificationProvider.php new file mode 100644 index 000000000..882a23b73 --- /dev/null +++ b/src/Domain/Identity/Service/Notification/DevModeNotificationProvider.php @@ -0,0 +1,56 @@ +devVersion = filter_var($devVersion, FILTER_VALIDATE_BOOLEAN); + } + + public function getNotifications(Administrator $administrator): array + { + if (!$this->devVersion || !$administrator->isSuperUser()) { + return []; + } + + if ($this->devEmail === '') { + return [ + new AdminNotification( + code: 'dev_mode_without_email', + level: AdminNotification::LEVEL_INFO, + message: $this->translator->trans('Running DEV version, but developer email is not set'), + ), + ]; + } + + return [ + new AdminNotification( + code: 'dev_mode', + level: AdminNotification::LEVEL_INFO, + message: $this->translator->trans( + 'Running DEV version. All emails will be sent to %email%', + ['%email%' => $this->devEmail] + ), + context: ['email' => $this->devEmail], + ), + ]; + } +} diff --git a/src/Domain/Identity/Service/Notification/MailerDisabledNotificationProvider.php b/src/Domain/Identity/Service/Notification/MailerDisabledNotificationProvider.php new file mode 100644 index 000000000..8109d4e42 --- /dev/null +++ b/src/Domain/Identity/Service/Notification/MailerDisabledNotificationProvider.php @@ -0,0 +1,46 @@ +isSuperUser() || !$this->usesNullTransport()) { + return []; + } + + return [ + new AdminNotification( + code: 'mailer_disabled', + level: AdminNotification::LEVEL_WARNING, + message: $this->translator->trans( + 'The mailer uses the "null" transport, no emails will be sent. Check MAILER_DSN.' + ), + ), + ]; + } + + private function usesNullTransport(): bool + { + return preg_match('#^\s*null(\+[a-z0-9]+)?://#i', $this->mailerDsn) === 1; + } +} diff --git a/src/Domain/Identity/Service/Notification/PendingMigrationsNotificationProvider.php b/src/Domain/Identity/Service/Notification/PendingMigrationsNotificationProvider.php new file mode 100644 index 000000000..e92e6d1c3 --- /dev/null +++ b/src/Domain/Identity/Service/Notification/PendingMigrationsNotificationProvider.php @@ -0,0 +1,60 @@ +isSuperUser()) { + return []; + } + + try { + $count = count($this->dependencyFactory->getMigrationStatusCalculator()->getNewMigrations()); + } catch (Throwable $e) { + $this->logger->warning('Unable to determine pending migrations', ['exception' => $e]); + + return []; + } + + if ($count === 0) { + return []; + } + + return [ + new AdminNotification( + code: 'pending_migrations', + level: AdminNotification::LEVEL_WARNING, + message: $this->translator->trans( + '%count% database migration(s) have not been run. ' + . 'Run "bin/console doctrine:migrations:migrate" to update the database', + ['%count%' => $count] + ), + context: ['count' => $count], + ), + ]; + } +} diff --git a/src/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProvider.php b/src/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProvider.php new file mode 100644 index 000000000..3c8ffcff6 --- /dev/null +++ b/src/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProvider.php @@ -0,0 +1,47 @@ +isSuperUser()) { + return []; + } + + $count = $this->messageRepository->countQueued(new DateTimeImmutable()); + if ($count === 0) { + return []; + } + + return [ + new AdminNotification( + code: 'campaigns_in_queue', + level: AdminNotification::LEVEL_INFO, + message: $this->translator->trans( + 'You have %count% campaign(s) in the queue, ready for processing', + ['%count%' => $count] + ), + context: ['count' => $count], + ), + ]; + } +} diff --git a/src/Domain/Identity/Service/PermissionChecker.php b/src/Domain/Identity/Service/PermissionChecker.php index dffbd6266..30c12d5d9 100644 --- a/src/Domain/Identity/Service/PermissionChecker.php +++ b/src/Domain/Identity/Service/PermissionChecker.php @@ -6,6 +6,7 @@ use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; use PhpList\Core\Domain\Common\Model\Interfaces\OwnableInterface; +use PhpList\Core\Domain\Identity\Model\AccessLevel; use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Identity\Model\PrivilegeFlag; use PhpList\Core\Domain\Messaging\Model\ListMessage; @@ -26,42 +27,36 @@ class PermissionChecker Message::class => SubscriberList::class ]; - public function canManage(Administrator $actor, DomainModel $resource): bool + /** + * @param class-string $resourceClass + */ + public function accessLevel(Administrator $actor, string $resourceClass): AccessLevel { if ($actor->isSuperUser()) { - return true; + return AccessLevel::All; } - $required = $this->resolveRequiredPrivilegeForClass(get_class($resource)); + $required = $this->resolveRequiredPrivilegeForClass($resourceClass); if ($required !== null && !$actor->getPrivileges()->has($required)) { - return false; - } - - if ($resource instanceof OwnableInterface) { - return $actor->owns($resource); + return AccessLevel::None; } - $notRestricted = true; - foreach (self::OWNERSHIP_MAP as $resourceClass => $relatedClass) { - if ($resource instanceof $resourceClass) { - $related = $this->resolveRelatedEntity($resource, $relatedClass); - $notRestricted = $this->checkRelatedResources($related, $actor); - } - } + return AccessLevel::Owner; + } - return $notRestricted; + public function canManage(Administrator $actor, DomainModel $resource): bool + { + return match ($this->accessLevel($actor, get_class($resource))) { + AccessLevel::All => true, + AccessLevel::None => false, + AccessLevel::Owner => $this->ownsResource($actor, $resource), + }; } /** @param class-string $resourceClass */ public function canCreate(Administrator $actor, string $resourceClass): bool { - if ($actor->isSuperUser()) { - return true; - } - - $required = $this->resolveRequiredPrivilegeForClass($resourceClass); - - return $required === null || $actor->getPrivileges()->has($required); + return $this->accessLevel($actor, $resourceClass) !== AccessLevel::None; } public function canList(Administrator $actor, string $resourceClass): bool @@ -84,6 +79,21 @@ public function canDelete(Administrator $actor, DomainModel $resource): bool return $this->canManage($actor, $resource); } + private function ownsResource(Administrator $actor, DomainModel $resource): bool + { + if ($resource instanceof OwnableInterface) { + return $actor->owns($resource); + } + + foreach (self::OWNERSHIP_MAP as $resourceClass => $relatedClass) { + if ($resource instanceof $resourceClass) { + return $this->checkRelatedResources($this->resolveRelatedEntity($resource, $relatedClass), $actor); + } + } + + return true; + } + /** @param class-string $resourceClass */ private function resolveRequiredPrivilegeForClass(string $resourceClass): ?PrivilegeFlag { diff --git a/src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php b/src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php index e74abff11..d3286b99b 100644 --- a/src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php +++ b/src/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriber.php @@ -4,14 +4,19 @@ namespace PhpList\Core\Domain\Messaging\EventSubscriber; +use PhpList\Core\Domain\Messaging\Service\Builder\HttpReceivedStampBuilder; use Symfony\Component\EventDispatcher\EventSubscriberInterface; -use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Mailer\Event\MessageEvent; use Symfony\Component\Mime\Email; +/** + * When the email is generated from a web request, add a "Received" line to identify the origin + * (same as phplist3's PHPlistMailer::add_timestamp()). Mails sent from the CLI or by async + * workers have no request, so they are not stamped. + */ class InjectedByHeaderSubscriber implements EventSubscriberInterface { - public function __construct(private readonly RequestStack $requestStack) + public function __construct(private readonly HttpReceivedStampBuilder $stampBuilder) { } @@ -22,31 +27,18 @@ public static function getSubscribedEvents(): array public function onMessage(MessageEvent $event): void { - $request = $this->requestStack->getCurrentRequest(); - if (!$request) { - return; - } - - // todo: add custom header only to messages sent individually not from campaigns - // when the email is generated from a webpage (quite possible :-) add a "received line" to identify the origin $msg = $event->getMessage(); if (!$msg instanceof Email) { return; } - // Only when triggered by HTTP request context (not CLI workers) - if (PHP_SAPI === 'cli') { + $stamp = $this->stampBuilder->buildStamp(); + if ($stamp === null) { return; } - $ip = $request->getClientIp() ?? 'unknown'; - $host = gethostname() ?: 'unknown-host'; - - $timestamp = $request->server->get('REQUEST_TIME') ?? time(); - - $msg->getHeaders()->addTextHeader( - 'X-phpList-Injected-By', - sprintf('from [%s] by %s with HTTP; %s', $ip, $host, $timestamp) - ); + $headers = $msg->getHeaders(); + $headers->remove('Received'); + $headers->addTextHeader('Received', preg_replace('/[\r\n]+/', ' ', $stamp)); } } diff --git a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php index af7d08d2f..c3b8ad078 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 61f473848..218954574 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 57cbc32be..928a5262c 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 9a350f880..efb6ea615 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 e3e0ab4fa..1ff480f8f 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 c5698775d..ec86673a8 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; @@ -63,9 +63,8 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes $queryBuilder = $this->createQueryBuilder('m'); if ($filter instanceof MessageFilter && $filter->getOwner() !== null) { - // Legacy/imported messages have no owner recorded - treat them as shared rather - // than invisible, instead of excluding them outright via a strict owner match. - $queryBuilder->andWhere('(m.owner IS NULL OR IDENTITY(m.owner) = :ownerId)') + // Same as phplist3: non-superusers only see the campaigns they own. + $queryBuilder->andWhere('IDENTITY(m.owner) = :ownerId') ->setParameter('ownerId', $filter->getOwner()->getId()); } @@ -137,6 +136,22 @@ public function incrementBounceCount(int $messageId): void ->execute(); } + /** Campaigns that are submitted or in process and whose embargo has passed. */ + public function countQueued(DateTimeImmutable $now): int + { + return (int) $this->createQueryBuilder('m') + ->select('COUNT(m.id)') + ->where('m.metadata.status IN (:statuses)') + ->andWhere('m.schedule.embargo IS NULL OR m.schedule.embargo <= :now') + ->setParameter('statuses', [ + Message\MessageStatus::Submitted->value, + Message\MessageStatus::InProcess->value, + ]) + ->setParameter('now', $now) + ->getQuery() + ->getSingleScalarResult(); + } + /** @return Message[] */ public function getByStatusAndEmbargo(Message\MessageStatus $status, DateTimeImmutable $embargo): array { @@ -175,7 +190,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 +204,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 +218,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 +287,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 9bc94e1fa..6bca9465a 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 bf3077373..b92fa4709 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/Builder/ForwardEmailBuilder.php b/src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php index 778e62af8..b31d2685c 100644 --- a/src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php +++ b/src/Domain/Messaging/Service/Builder/ForwardEmailBuilder.php @@ -43,7 +43,6 @@ public function __construct( PdfGenerator $pdfGenerator, AttachmentAdder $attachmentAdder, TranslatorInterface $translator, - private readonly HttpReceivedStampBuilder $httpReceivedStampBuilder, string $googleSenderId, bool $useAmazonSes, bool $usePrecedenceHeader, @@ -129,17 +128,4 @@ public function buildForwardEmail( return [$email, $sentAs]; } - - public function applyCampaignHeaders(Email $email, Subscriber $subscriber): Email - { - $email = parent::applyCampaignHeaders($email, $subscriber); - - $receivedLine = $this->httpReceivedStampBuilder->buildStamp(); - if ($receivedLine !== null) { - $receivedLine = preg_replace('/[\r\n]+/', ' ', $receivedLine); - $email->getHeaders()->addTextHeader('Received', $receivedLine); - } - - return $email; - } } diff --git a/src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php b/src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php index fe06efca6..2662f5022 100644 --- a/src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php +++ b/src/Domain/Messaging/Service/Builder/HttpReceivedStampBuilder.php @@ -5,15 +5,12 @@ namespace PhpList\Core\Domain\Messaging\Service\Builder; use DateTimeImmutable; -use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\HttpFoundation\RequestStack; class HttpReceivedStampBuilder { - public function __construct( - private readonly RequestStack $requestStack, - #[Autowire('%app.rest_api_base_url%')] private readonly string $restApiBaseUrl, - ) { + public function __construct(private readonly RequestStack $requestStack) + { } public function buildStamp(): ?string @@ -40,7 +37,7 @@ public function buildStamp(): ?string $requestTime = $request->server->get('REQUEST_TIME') ?? time(); $date = (new DateTimeImmutable('@' . $requestTime))->format(\DATE_RFC2822); - return sprintf('from %s by %s with HTTP; %s', $from, $this->restApiBaseUrl, $date); + return sprintf('from %s by %s with HTTP; %s', $from, $request->getHost(), $date); } private function getHostByAddr(string $ipAddress): ?string diff --git a/src/Domain/Messaging/Service/CampaignEmailSender.php b/src/Domain/Messaging/Service/CampaignEmailSender.php index 7cb590b00..e4babbfb9 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 c7398a203..3d752bb29 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 2fa0e17d3..01d27388a 100644 --- a/src/Domain/Messaging/Service/ForwardContentService.php +++ b/src/Domain/Messaging/Service/ForwardContentService.php @@ -13,14 +13,16 @@ use PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder; use PhpList\Core\Domain\Subscription\Model\Subscriber; use Psr\SimpleCache\CacheInterface; +use Symfony\Component\DependencyInjection\Attribute\Autowire; use Symfony\Component\Mime\Email; class ForwardContentService { public function __construct( private readonly CacheInterface $cache, - private readonly MessageProcessingPreparator $messagePreparator, private readonly ForwardEmailBuilder $forwardEmailBuilder, + private readonly MessagePrecacheService $precacheService, + #[Autowire('%messaging.forward_alternative_content%')] private readonly bool $forwardAlternativeContent = false, ) { } @@ -33,24 +35,21 @@ public function getContents( string $friendEmail, MessageForwardDto $forwardDto ): array { - $messagePrecacheDto = $this->cache->get(sprintf('messaging.message.base.%d.%d.%d', $campaign->getId(), 1, 0)); + $messagePrecacheDto = $this->cache->get( + $this->precacheService->getCacheKey($campaign->getId(), $this->forwardAlternativeContent) + ); if ($messagePrecacheDto === null) { 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 760fda603..c46af070c 100644 --- a/src/Domain/Messaging/Service/Manager/MessageDataManager.php +++ b/src/Domain/Messaging/Service/Manager/MessageDataManager.php @@ -6,12 +6,31 @@ use Doctrine\ORM\EntityManagerInterface; use HTMLPurifier; +use InvalidArgumentException; use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Messaging\Model\MessageData; use PhpList\Core\Domain\Messaging\Repository\MessageDataRepository; class MessageDataManager { + /** + * The only settings storeMessageData() accepts. Everything else in messagedata is either kept on the message + * itself (subject, from field, schedule, ...) or maintained by the system (status, sizes, ETA, statistics, + * ...) and must not be overwritten by a caller-supplied value. + */ + public const WRITABLE_SETTINGS = [ + 'google_track', + 'utm_source', + 'utm_medium', + 'utm_campaign', + 'pk_source', + 'pk_medium', + 'pk_campaign', + 'notify_start', + 'notify_end', + 'excludelist', + ]; + public function __construct( private readonly MessageDataRepository $messageDataRepository, private readonly EntityManagerInterface $entityManager, @@ -34,34 +53,6 @@ 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); -// } -// } -// } -// } - if (is_array($value) || is_object($value)) { $value = 'SER:' . serialize($value); } @@ -70,6 +61,38 @@ public function setMessageData(Message $campaign, string $name, mixed $value): v $entity->setData($value !== null ? (string) $value : null); } + /** + * Stores a set of campaign settings the way phplist3's storemessage.php does for the campaign form. + * + * Checkboxes that are not submitted when unchecked must be listed in $checkboxes: they are all reset to '0' + * first and then set again by $data when they were submitted. Every entry of $data is stored under its own + * name, booleans as '1' / '0'. + * + * Only the names in WRITABLE_SETTINGS are accepted; nothing is stored when any other name is given. + * The campaign must already have an id, so a new campaign has to be flushed first. + * + * @param array $data setting name => value + * @param string[] $checkboxes names of the checkbox settings that are part of this save + * @throws InvalidArgumentException when a name is not in WRITABLE_SETTINGS + */ + public function storeMessageData(Message $campaign, array $data, array $checkboxes = []): void + { + $notWritable = array_diff([...array_keys($data), ...$checkboxes], self::WRITABLE_SETTINGS); + if ($notWritable !== []) { + throw new InvalidArgumentException( + 'Message data cannot be set for: ' . implode(', ', array_map('strval', $notWritable)) + ); + } + + foreach ($checkboxes as $name) { + $this->setMessageData($campaign, $name, '0'); + } + + foreach ($data as $name => $value) { + $this->setMessageData($campaign, (string) $name, is_bool($value) ? ($value ? '1' : '0') : $value); + } + } + private function normalizeValueByName(string $name, mixed $value) { return match ($name) { diff --git a/src/Domain/Messaging/Service/Manager/TemplateImageManager.php b/src/Domain/Messaging/Service/Manager/TemplateImageManager.php index bb6c07715..15cfbc6c6 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 161d302d5..c1db86b57 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 b93f5d763..ec4062cbc 100644 --- a/src/Domain/Messaging/Service/MessageForwardService.php +++ b/src/Domain/Messaging/Service/MessageForwardService.php @@ -14,6 +14,7 @@ use PhpList\Core\Domain\Messaging\Model\Message; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Repository\SubscriberListRepository; +use Symfony\Component\DependencyInjection\Attribute\Autowire; class MessageForwardService { @@ -26,6 +27,7 @@ public function __construct( private readonly MessagePrecacheService $precacheService, private readonly AdminNotifier $adminNotifier, private readonly ForwardingStatsService $forwardingStatsService, + #[Autowire('%messaging.forward_alternative_content%')] private readonly bool $forwardAlternativeContent = false, ) { } @@ -54,7 +56,12 @@ public function forward(MessageForwardDto $messageForwardDto, Message $campaign) continue; } - if (!$this->precacheService->precacheMessage($campaign, $loadedMessageData, true)) { + $messagePrecached = $this->precacheService->precacheMessage( + $campaign, + $loadedMessageData, + $this->forwardAlternativeContent + ); + if (!$messagePrecached) { $forwardingRecipientResult = $this->handleFailure( campaign: $campaign, forwardingSubscriber: $forwardingSubscriber, @@ -78,7 +85,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 59641c5ff..86ed5b318 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 05f044ea7..acdd1914c 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/SendRateLimiter.php b/src/Domain/Messaging/Service/SendRateLimiter.php index 2590e7214..40617bd86 100644 --- a/src/Domain/Messaging/Service/SendRateLimiter.php +++ b/src/Domain/Messaging/Service/SendRateLimiter.php @@ -46,7 +46,7 @@ private function initializeLimits(): void $ispMinPeriod = $isp->minBatchPeriod ?? 0; $cfgThrottle = $this->mailqueueThrottle ?? 0; - $ispMinThrottle = (int)($isp->minThrottle ?? 0); + $ispMinThrottle = (int)($isp->raw['minthrottle'] ?? 0); if ($cfgBatch <= 0) { $this->batchSize = $ispMax !== null ? max(0, $ispMax) : 0; diff --git a/src/Domain/Messaging/Service/SystemNotificationMailer.php b/src/Domain/Messaging/Service/SystemNotificationMailer.php index 62c606283..160ad6696 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 ee468fa1d..c76c34309 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 e29a9b7d6..7e8ce0e4e 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/SubscriberListRepository.php b/src/Domain/Subscription/Repository/SubscriberListRepository.php index 270ab7f68..d3da1835e 100644 --- a/src/Domain/Subscription/Repository/SubscriberListRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberListRepository.php @@ -83,6 +83,11 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes $queryBuilder = $this->createQueryBuilder('l'); + // Same as phplist3: when an owner is given (non-superuser), only the lists they own are visible. + if ($filter->getOwner() !== null) { + $queryBuilder->where('l.owner = :admin')->setParameter('admin', $filter->getOwner()); + } + $countQb = clone $queryBuilder; $total = (int) $countQb ->select('COUNT(DISTINCT l.id)') @@ -91,8 +96,6 @@ public function getFilteredAfterId(FilterRequestInterface $filter): PaginatedRes /** @var list $items */ $items = $queryBuilder - ->where($queryBuilder->expr()->orX('l.owner = :admin', 'l.public = true')) - ->setParameter('admin', $filter->getOwner()) ->andWhere('l.id > :id') ->setParameter('id', $filter->getLastId()) ->setMaxResults($filter->getLimit()) diff --git a/src/Domain/Subscription/Repository/SubscriberRepository.php b/src/Domain/Subscription/Repository/SubscriberRepository.php index b55164c36..706e76922 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 9dabc9ff0..5651c20c6 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 dcd2f6e86..52e8be472 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/SubscriberAttributeManager.php b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php index 8eb1d3634..d0d847dce 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberAttributeManager.php @@ -43,7 +43,12 @@ public function createOrUpdate( SubscriberAttributeDefinition $definition, ?string $value = null ): SubscriberAttributeValue { - // todo: clarify which attributes can be created/updated + // todo: phplist3 saves the value as given (empty allowed) and checks required fields at form level; + // here a missing value without a default throws. Check whether that is intended. + // todo: phplist3 maps a select attribute's value to its option (adding the option when missing); + // here the value is stored as given without checking it against the options. + // Like phplist3's saveUserAttribute(): only attributes that have a definition get a value. Built-in + // subscriber fields (email, password, ...) are columns on the subscriber, not attribute values. $subscriberAttribute = $this->attributeRepository ->findOneBySubscriberAndAttribute($subscriber, $definition); diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index 189a28817..6c9e6f6db 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 48802fe07..1420fe9ec 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 f9db822a7..18ee26a13 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/src/TestingSupport/Traits/DatabaseTestTrait.php b/src/TestingSupport/Traits/DatabaseTestTrait.php index d645894c3..c231fdc39 100644 --- a/src/TestingSupport/Traits/DatabaseTestTrait.php +++ b/src/TestingSupport/Traits/DatabaseTestTrait.php @@ -93,14 +93,22 @@ protected function loadSchema(): void private function runForMySql($metadata, $schemaTool): void { - try { - $schemaTool->createSchema($metadata); - } catch (ToolsException $e) { - $missing = $this->filterMissingTables($metadata); + $missing = $this->filterMissingTables($metadata); - if ($missing !== []) { - $schemaTool->createSchema($missing); + if ($missing === []) { + return; + } + + $connection = $this->entityManager->getConnection(); + $messengerTableExists = $this->tableExistsIgnoringSchemaFilter('messenger_messages'); + + // Symfony's Doctrine messenger schema listener appends CREATE TABLE messenger_messages to every + // schema it builds, so skip it when the table is already there (e.g. from a previous run). + foreach ($schemaTool->getCreateSchemaSql($missing) as $sql) { + if ($messengerTableExists && str_starts_with($sql, 'CREATE TABLE messenger_messages')) { + continue; } + $connection->executeStatement($sql); } } diff --git a/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php b/tests/Integration/Domain/Analytics/Repository/LinkTrackClickRepositoriesTest.php new file mode 100644 index 000000000..c20d1a1f4 --- /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/Identity/Service/PermissionCheckerTest.php b/tests/Integration/Domain/Identity/Service/PermissionCheckerTest.php index 7f2d0d67d..5fcdb7512 100644 --- a/tests/Integration/Domain/Identity/Service/PermissionCheckerTest.php +++ b/tests/Integration/Domain/Identity/Service/PermissionCheckerTest.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Tests\Integration\Domain\Identity\Service; use PhpList\Core\Domain\Configuration\Model\Config; +use PhpList\Core\Domain\Identity\Model\AccessLevel; use PhpList\Core\Domain\Identity\Model\Administrator; use PhpList\Core\Domain\Identity\Model\PrivilegeFlag; use PhpList\Core\Domain\Identity\Model\Privileges; @@ -89,4 +90,53 @@ public function testNonSuperUserCanCreateResourceWithNoRequiredPrivilege(): void $this->assertTrue($this->checker->canCreate($admin, Config::class)); } + + public function testAccessLevelIsAllForSuperUser(): void + { + $admin = (new Administrator())->setSuperUser(true); + + self::assertSame(AccessLevel::All, $this->checker->accessLevel($admin, SubscriberList::class)); + } + + public function testAccessLevelIsNoneWithoutRequiredPrivilege(): void + { + $admin = (new Administrator())->setPrivileges(new Privileges()); + + self::assertSame(AccessLevel::None, $this->checker->accessLevel($admin, Message::class)); + } + + public function testAccessLevelIsOwnerWithRequiredPrivilege(): void + { + $admin = (new Administrator())->setPrivileges((new Privileges())->grant(PrivilegeFlag::Campaigns)); + + self::assertSame(AccessLevel::Owner, $this->checker->accessLevel($admin, Message::class)); + } + + public function testCannotManageListWithoutPrivilegeEvenIfOwner(): void + { + $admin = $this->adminWithId(1, new Privileges()); + $list = (new SubscriberList())->setOwner($admin); + + self::assertFalse($this->checker->canManage($admin, $list)); + } + + public function testCanManageOwnedListButNotForeignOrOwnerlessList(): void + { + $admin = $this->adminWithId(1, (new Privileges())->grant(PrivilegeFlag::Subscribers)); + $other = $this->adminWithId(2, new Privileges()); + + self::assertTrue($this->checker->canManage($admin, (new SubscriberList())->setOwner($admin))); + self::assertFalse($this->checker->canManage($admin, (new SubscriberList())->setOwner($other))); + self::assertFalse($this->checker->canManage($admin, new SubscriberList())); + } + + private function adminWithId(int $id, Privileges $privileges): Administrator + { + $admin = (new Administrator())->setPrivileges($privileges); + $property = new \ReflectionProperty(Administrator::class, 'id'); + $property->setAccessible(true); + $property->setValue($admin, $id); + + return $admin; + } } diff --git a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php index a384457e5..dbcd34c2a 100644 --- a/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php +++ b/tests/Integration/Domain/Messaging/Repository/MessageRepositoryTest.php @@ -152,7 +152,7 @@ private function persistMessage( return $message; } - public function testGetFilteredAfterIdIncludesOwnerlessMessagesForAnyAdmin(): void + public function testGetFilteredAfterIdExcludesOwnerlessAndForeignMessagesForOwnerFilter(): void { $admin = (new Administrator())->setLoginName('owner-test-admin'); $otherAdmin = (new Administrator())->setLoginName('other-admin'); @@ -173,7 +173,7 @@ public function testGetFilteredAfterIdIncludesOwnerlessMessagesForAnyAdmin(): vo static fn (Message $message) => $message->getContent()->getSubject(), $result->getItems() ); - self::assertContains('Legacy unowned campaign', $subjects); + self::assertNotContains('Legacy unowned campaign', $subjects); self::assertContains('My own campaign', $subjects); self::assertNotContains("Someone else's campaign", $subjects); } @@ -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/SubscriberListRepositoryTest.php b/tests/Integration/Domain/Messaging/Repository/SubscriberListRepositoryTest.php index 93e99a810..f6b5ae978 100644 --- a/tests/Integration/Domain/Messaging/Repository/SubscriberListRepositoryTest.php +++ b/tests/Integration/Domain/Messaging/Repository/SubscriberListRepositoryTest.php @@ -6,6 +6,7 @@ use DateTime; use Doctrine\ORM\Tools\SchemaTool; +use PhpList\Core\Domain\Messaging\Model\Filter\SubscriberListFilter; use PhpList\Core\Domain\Identity\Repository\AdministratorRepository; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberList; @@ -109,6 +110,23 @@ public function testSavePersistsAndFlushesModel() self::assertSame($model, $this->subscriberListRepository->find($model->getId())); } + public function testGetFilteredAfterIdReturnsOnlyOwnedListsForOwnerFilter() + { + $this->loadFixtures([SubscriberListFixture::class, AdministratorFixture::class]); + + $owner = $this->administratorRepository->find(1); + $filter = (new SubscriberListFilter())->setOwner($owner)->setLimit(100); + + $result = $this->subscriberListRepository->getFilteredAfterId($filter); + + self::assertNotEmpty($result->getItems()); + foreach ($result->getItems() as $list) { + self::assertInstanceOf(SubscriberList::class, $list); + self::assertSame($owner->getId(), $list->getOwner()->getId()); + } + self::assertSame(count($result->getItems()), $result->getTotal()); + } + public function testFindByOwnerFindsSubscriberListWithTheGivenOwner() { $this->loadFixtures([SubscriberListFixture::class, AdministratorFixture::class]); diff --git a/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php b/tests/Integration/Domain/Messaging/Repository/UserMessageBounceElasticsearchHybridReaderTest.php index 4459a4dad..0a28f547b 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 c835207c3..486773995 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 9c27c1c9b..914cdde8a 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/Core/Doctrine/SkippedMigrationRecorderTest.php b/tests/Unit/Core/Doctrine/SkippedMigrationRecorderTest.php new file mode 100644 index 000000000..b9b49f0da --- /dev/null +++ b/tests/Unit/Core/Doctrine/SkippedMigrationRecorderTest.php @@ -0,0 +1,67 @@ +createMock(AbstractMigration::class), $direction); + if ($withResult) { + $plan->markAsExecuted(new ExecutionResult($version, $direction, new DateTimeImmutable())); + } + + return new MigrationsVersionEventArgs( + $this->createMock(Connection::class), + $plan, + (new MigratorConfiguration())->setDryRun($dryRun) + ); + } + + private function recorder(int $expectedCalls): SkippedMigrationRecorder + { + $storage = $this->createMock(MetadataStorage::class); + $storage->expects(self::exactly($expectedCalls))->method('complete'); + $factory = $this->createMock(DependencyFactory::class); + $factory->method('getMetadataStorage')->willReturn($storage); + + return new SkippedMigrationRecorder($factory); + } + + public function testRecordsSkippedUpMigration(): void + { + $this->recorder(1)->onMigrationsVersionSkipped($this->args(Direction::UP, false)); + } + + public function testDoesNotRecordOnDryRun(): void + { + $this->recorder(0)->onMigrationsVersionSkipped($this->args(Direction::UP, true)); + } + + public function testDoesNotRecordSkippedDownMigration(): void + { + $this->recorder(0)->onMigrationsVersionSkipped($this->args(Direction::DOWN, false)); + } + + public function testDoesNothingWithoutResult(): void + { + $this->recorder(0)->onMigrationsVersionSkipped($this->args(Direction::UP, false, false)); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php b/tests/Unit/Domain/Analytics/Service/AnalyticsTrackingServiceTest.php new file mode 100644 index 000000000..2aa6e1209 --- /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 000000000..bc2ed7e04 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkClickServiceTest.php @@ -0,0 +1,259 @@ +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->createMock(ConfigProvider::class) + ); + + $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 testPersonalisedLinkWorksForSubscribersWithoutRecordOfTheCampaign(): void + { + $this->givenKnownLink('https://example.com/p', personalise: true); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->mlRepository->expects(self::once())->method('recordClick'); + + self::assertSame('https://example.com/p?uid=uniq-3', $this->service->trackClick($this->tid(), null, [])); + } + + public function testNonPersonalisedLinkWorksWithoutUserMessageAndCountsEveryClickAsView(): void + { + $this->givenKnownLink(); + $this->userMessageRepository->method('findByUserAndMessage')->willReturn(null); + + $this->userMessageService->expects(self::once())->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 000000000..942f665b0 --- /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 c7495f993..c523afa41 100644 --- a/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php @@ -4,257 +4,283 @@ 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\Configuration\Service\Provider\ConfigProvider; 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->createMock(ConfigProvider::class) + ); + + $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 000000000..9e2c8def1 --- /dev/null +++ b/tests/Unit/Domain/Analytics/Service/LinkTrackTokenServiceTest.php @@ -0,0 +1,174 @@ +createMock(ConfigProvider::class); + $configProvider->method('getValue')->with(ConfigOption::XorMask)->willReturn($xorMask); + + return new LinkTrackTokenService( + new ParameterProvider(['click_track_sign_with_hmac' => $sign]), + 'https://example.com/api/v2/', + $secret, + $configProvider + ); + } + + /** What phplist3's sendemaillib.php puts in the link of an HTML email. */ + private function phplist3HtmlToken(): string + { + $track = 'H|' . self::FORWARD_UUID . '|' . self::MESSAGE_UUID . '|' . self::SUBSCRIBER_UUID; + $masked = base64_encode($track ^ str_repeat(self::XOR_MASK, 20)); + + return preg_replace('/=$/', '', preg_replace('/=$/', '', $masked)); + } + + /** What phplist3's sendemaillib.php puts in the link of a text email. */ + private function phplist3TextToken(): string + { + $masked = self::FORWARD_UUID . self::MESSAGE_UUID . self::SUBSCRIBER_UUID; + foreach ([14, 36 + 14, 72 + 14] as $position) { + $masked[$position] = 'c'; + } + + return str_replace('=', '', base64_encode(hex2bin(str_replace('-', '', $masked)))); + } + + /** @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); + } + + public function testParsesTheHtmlTokensOfPhplist3(): void + { + $token = $this->createService()->parse($this->phplist3HtmlToken()); + + self::assertNotNull($token); + self::assertTrue($token->isHtml()); + self::assertSame(self::FORWARD_UUID, $token->forwardUuid); + self::assertSame(self::MESSAGE_UUID, $token->messageUuid); + self::assertSame(self::SUBSCRIBER_UUID, $token->subscriberUuid); + } + + public function testParsesTheHtmlTokensOfPhplist3AfterAQueryStringTurnedPlusIntoSpace(): void + { + $tid = str_replace('+', ' ', $this->phplist3HtmlToken()); + + self::assertNotNull($this->createService()->parse($tid)); + } + + public function testParsesTheTextTokensOfPhplist3(): void + { + $token = $this->createService()->parse($this->phplist3TextToken()); + + self::assertNotNull($token); + self::assertSame('T', $token->type); + self::assertSame(self::FORWARD_UUID, $token->forwardUuid); + self::assertSame(self::MESSAGE_UUID, $token->messageUuid); + self::assertSame(self::SUBSCRIBER_UUID, $token->subscriberUuid); + } + + public function testPhplist3HtmlTokensNeedTheSameXorMask(): void + { + $tid = $this->phplist3HtmlToken(); + + self::assertNull($this->createService(xorMask: 'fedcba9876543210fedcba9876543210')->parse($tid)); + self::assertNull($this->createService(xorMask: null)->parse($tid)); + } + + public function testPhplist3TokensCannotBeVerifiedWhenSigningIsEnabled(): void + { + $service = $this->createService(sign: true); + + self::assertNull($service->parse($this->phplist3HtmlToken(), 'whatever')); + self::assertNull($service->parse($this->phplist3TextToken(), 'whatever')); + } +} diff --git a/tests/Unit/Domain/Analytics/Service/UserMessageServiceTest.php b/tests/Unit/Domain/Analytics/Service/UserMessageServiceTest.php index 47da15206..61d8b8cfd 100644 --- a/tests/Unit/Domain/Analytics/Service/UserMessageServiceTest.php +++ b/tests/Unit/Domain/Analytics/Service/UserMessageServiceTest.php @@ -79,9 +79,10 @@ public function testReturnsEarlyWhenMessageNotFound(): void $this->subject->trackUserMessageView('sub-uid', 123, []); } - public function testReturnsEarlyWhenUserMessageNotFound(): void + public function testViewIsCountedEvenWhenTheCampaignWasNeverSentToTheSubscriber(): void { $subscriber = $this->createMock(Subscriber::class); + $subscriber->method('getId')->willReturn(5); $message = $this->getMockBuilder(Message::class) ->disableOriginalConstructor() ->getMock(); @@ -97,8 +98,8 @@ public function testReturnsEarlyWhenUserMessageNotFound(): void ->with($subscriber, $message) ->willReturn(null); - $metadata->expects(self::never())->method('incrementViews'); - $this->entityManager->expects(self::never())->method('persist'); + $metadata->expects(self::once())->method('incrementViews'); + $this->entityManager->expects(self::once())->method('persist'); $this->subject->trackUserMessageView('sub-uid', 321, []); } diff --git a/tests/Unit/Domain/Common/FileHelperTest.php b/tests/Unit/Domain/Common/FileHelperTest.php index 9f652623b..4e1f3d7ee 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 19caaadaf..21992a4ac 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/ConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php index bd7eee08c..d51d33f96 100644 --- a/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php +++ b/tests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.php @@ -248,4 +248,22 @@ public function testGetValueWithNamespaceSupportsStringNamespacedKey(): void $this->assertSame('PARENT', $this->provider->getValueWithNamespace('subscribemessage:1')); } + + public function testGetValueReplacesWebsiteAndDomainPlaceholders(): void + { + $this->cache->method('get')->willReturn(null); + $this->repo->method('findValueByItem')->willReturnCallback( + fn (string $item) => match ($item) { + ConfigOption::SubscribeUrl->value => 'https://[WEBSITE]/lists/?p=subscribe&d=[DOMAIN]', + ConfigOption::Website->value => 'lists.example.org', + ConfigOption::Domain->value => 'example.org', + default => null, + } + ); + + self::assertSame( + 'https://lists.example.org/lists/?p=subscribe&d=example.org', + $this->provider->getValue(ConfigOption::SubscribeUrl) + ); + } } diff --git a/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php b/tests/Unit/Domain/Configuration/Service/Provider/DefaultConfigProviderTest.php index 1d79369a2..b0aab9198 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/Identity/Service/Notification/AdminNotificationCollectorTest.php b/tests/Unit/Domain/Identity/Service/Notification/AdminNotificationCollectorTest.php new file mode 100644 index 000000000..baa80464a --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/AdminNotificationCollectorTest.php @@ -0,0 +1,30 @@ +createMock(AdminNotificationProviderInterface::class); + $p1->method('getNotifications')->willReturn([$a]); + $p2 = $this->createMock(AdminNotificationProviderInterface::class); + $p2->method('getNotifications')->willReturn([]); + $p3 = $this->createMock(AdminNotificationProviderInterface::class); + $p3->method('getNotifications')->willReturn([$b]); + + $result = (new AdminNotificationCollector([$p1, $p2, $p3]))->collect($this->createMock(Administrator::class)); + + self::assertSame([$a, $b], $result); + } +} diff --git a/tests/Unit/Domain/Identity/Service/Notification/AttachmentRepositoryNotificationProviderTest.php b/tests/Unit/Domain/Identity/Service/Notification/AttachmentRepositoryNotificationProviderTest.php new file mode 100644 index 000000000..3ab3bbdd8 --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/AttachmentRepositoryNotificationProviderTest.php @@ -0,0 +1,48 @@ +createMock(TranslatorInterface::class); + $translator->method('trans')->willReturnArgument(0); + + return new AttachmentRepositoryNotificationProvider($translator, $path); + } + + private function admin(bool $super): Administrator + { + $admin = $this->createMock(Administrator::class); + $admin->method('isSuperUser')->willReturn($super); + + return $admin; + } + + public function testWritableDirectoryProducesNothing(): void + { + self::assertSame([], $this->provider(sys_get_temp_dir())->getNotifications($this->admin(true))); + } + + public function testMissingDirectoryProducesWarning(): void + { + $result = $this->provider('/nonexistent/attachments')->getNotifications($this->admin(true)); + + self::assertCount(1, $result); + self::assertSame('attachment_repository_not_writable', $result[0]->code); + self::assertSame('warning', $result[0]->level); + } + + public function testNonSuperUserGetsNothing(): void + { + self::assertSame([], $this->provider('/nonexistent')->getNotifications($this->admin(false))); + } +} diff --git a/tests/Unit/Domain/Identity/Service/Notification/DevModeNotificationProviderTest.php b/tests/Unit/Domain/Identity/Service/Notification/DevModeNotificationProviderTest.php new file mode 100644 index 000000000..d4f70e3a1 --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/DevModeNotificationProviderTest.php @@ -0,0 +1,57 @@ +createMock(TranslatorInterface::class); + $translator->method('trans')->willReturnArgument(0); + + return new DevModeNotificationProvider($translator, $dev, $email); + } + + private function admin(bool $super): Administrator + { + $admin = $this->createMock(Administrator::class); + $admin->method('isSuperUser')->willReturn($super); + + return $admin; + } + + public function testDisabledProducesNothing(): void + { + self::assertSame([], $this->provider('0', 'dev@example.com')->getNotifications($this->admin(true))); + self::assertSame([], $this->provider(false, 'dev@example.com')->getNotifications($this->admin(true))); + } + + public function testEnabledShowsRedirectEmail(): void + { + $result = $this->provider('1', 'dev@example.com')->getNotifications($this->admin(true)); + + self::assertCount(1, $result); + self::assertSame('dev_mode', $result[0]->code); + self::assertSame('info', $result[0]->level); + self::assertSame(['email' => 'dev@example.com'], $result[0]->context); + } + + public function testEnabledWithoutEmail(): void + { + $result = $this->provider(true, '')->getNotifications($this->admin(true)); + + self::assertSame('dev_mode_without_email', $result[0]->code); + } + + public function testNonSuperUserGetsNothing(): void + { + self::assertSame([], $this->provider('1', 'dev@example.com')->getNotifications($this->admin(false))); + } +} diff --git a/tests/Unit/Domain/Identity/Service/Notification/MailerDisabledNotificationProviderTest.php b/tests/Unit/Domain/Identity/Service/Notification/MailerDisabledNotificationProviderTest.php new file mode 100644 index 000000000..436054d0c --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/MailerDisabledNotificationProviderTest.php @@ -0,0 +1,60 @@ +createMock(TranslatorInterface::class); + $translator->method('trans')->willReturnArgument(0); + + return new MailerDisabledNotificationProvider($translator, $dsn); + } + + private function admin(bool $super): Administrator + { + $admin = $this->createMock(Administrator::class); + $admin->method('isSuperUser')->willReturn($super); + + return $admin; + } + + /** @dataProvider nullDsns */ + public function testNullTransportProducesWarning(string $dsn): void + { + $result = $this->provider($dsn)->getNotifications($this->admin(true)); + + self::assertCount(1, $result); + self::assertSame('mailer_disabled', $result[0]->code); + self::assertSame('warning', $result[0]->level); + } + + public static function nullDsns(): array + { + return [['null://null'], ['NULL://default'], ['null+smtp://x']]; + } + + /** @dataProvider realDsns */ + public function testRealTransportProducesNothing(string $dsn): void + { + self::assertSame([], $this->provider($dsn)->getNotifications($this->admin(true))); + } + + public static function realDsns(): array + { + return [['smtp://user:null@host:25'], ['sendgrid://KEY@default'], ['native://default'], ['']]; + } + + public function testNonSuperUserGetsNothing(): void + { + self::assertSame([], $this->provider('null://null')->getNotifications($this->admin(false))); + } +} diff --git a/tests/Unit/Domain/Identity/Service/Notification/PendingMigrationsNotificationProviderTest.php b/tests/Unit/Domain/Identity/Service/Notification/PendingMigrationsNotificationProviderTest.php new file mode 100644 index 000000000..cb53d0ded --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/PendingMigrationsNotificationProviderTest.php @@ -0,0 +1,91 @@ +createMock(DependencyFactory::class); + if ($calculator instanceof RuntimeException) { + $factory->method('getMigrationStatusCalculator')->willThrowException($calculator); + } else { + $factory->method('getMigrationStatusCalculator')->willReturn($calculator); + } + + $translator = $this->createMock(TranslatorInterface::class); + $translator->method('trans')->willReturnArgument(0); + + return new PendingMigrationsNotificationProvider( + $factory, + $translator, + $this->createMock(LoggerInterface::class) + ); + } + + private function calculator(int $pending): MigrationStatusCalculator + { + $migrations = []; + for ($i = 1; $i <= $pending; $i++) { + $migrations[] = new AvailableMigration( + new Version('Version' . $i), + $this->createMock(AbstractMigration::class) + ); + } + $calculator = $this->createMock(MigrationStatusCalculator::class); + $calculator->method('getNewMigrations')->willReturn(new AvailableMigrationsList($migrations)); + + return $calculator; + } + + private function admin(bool $super): Administrator + { + $admin = $this->createMock(Administrator::class); + $admin->method('isSuperUser')->willReturn($super); + + return $admin; + } + + public function testNoPendingMigrationsProducesNothing(): void + { + self::assertSame([], $this->provider($this->calculator(0))->getNotifications($this->admin(true))); + } + + public function testPendingMigrationsProduceWarning(): void + { + $result = $this->provider($this->calculator(2))->getNotifications($this->admin(true)); + + self::assertCount(1, $result); + self::assertSame('pending_migrations', $result[0]->code); + self::assertSame('warning', $result[0]->level); + self::assertSame(['count' => 2], $result[0]->context); + } + + public function testNonSuperUserGetsNothing(): void + { + self::assertSame([], $this->provider($this->calculator(2))->getNotifications($this->admin(false))); + } + + public function testFailureIsSwallowed(): void + { + $provider = $this->provider(new RuntimeException('db down')); + + self::assertSame([], $provider->getNotifications($this->admin(true))); + } +} diff --git a/tests/Unit/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProviderTest.php b/tests/Unit/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProviderTest.php new file mode 100644 index 000000000..22ec8f374 --- /dev/null +++ b/tests/Unit/Domain/Identity/Service/Notification/QueuedCampaignsNotificationProviderTest.php @@ -0,0 +1,63 @@ +repository = $this->createMock(MessageRepository::class); + $translator = $this->createMock(TranslatorInterface::class); + $translator->method('trans')->willReturnCallback( + fn (string $id, array $params = []) => strtr($id, $params) + ); + $this->subject = new QueuedCampaignsNotificationProvider($this->repository, $translator); + } + + private function admin(bool $super): Administrator + { + $admin = $this->createMock(Administrator::class); + $admin->method('isSuperUser')->willReturn($super); + + return $admin; + } + + public function testNonSuperUserGetsNothing(): void + { + $this->repository->expects(self::never())->method('countQueued'); + + self::assertSame([], $this->subject->getNotifications($this->admin(false))); + } + + public function testEmptyQueueGetsNothing(): void + { + $this->repository->method('countQueued')->willReturn(0); + + self::assertSame([], $this->subject->getNotifications($this->admin(true))); + } + + public function testQueuedCampaignsProduceInfoNotification(): void + { + $this->repository->method('countQueued')->willReturn(3); + + $result = $this->subject->getNotifications($this->admin(true)); + + self::assertCount(1, $result); + self::assertSame('campaigns_in_queue', $result[0]->code); + self::assertSame('info', $result[0]->level); + self::assertSame(['count' => 3], $result[0]->context); + self::assertStringContainsString('3 campaign(s)', $result[0]->message); + } +} diff --git a/tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php b/tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php index 78358a92d..2bd88e2ee 100644 --- a/tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php +++ b/tests/Unit/Domain/Messaging/EventSubscriber/InjectedByHeaderSubscriberTest.php @@ -5,9 +5,9 @@ namespace PhpList\Core\Tests\Unit\Domain\Messaging\EventSubscriber; use PhpList\Core\Domain\Messaging\EventSubscriber\InjectedByHeaderSubscriber; +use PhpList\Core\Domain\Messaging\Service\Builder\HttpReceivedStampBuilder; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\Mailer\Envelope; use Symfony\Component\Mailer\Event\MessageEvent; use Symfony\Component\Mime\Address; @@ -16,74 +16,64 @@ class InjectedByHeaderSubscriberTest extends TestCase { - public function testNoHeaderWhenNoCurrentRequest(): void + private HttpReceivedStampBuilder&MockObject $stampBuilder; + + protected function setUp(): void { - $requestStack = new RequestStack(); - $subscriber = new InjectedByHeaderSubscriber($requestStack); + $this->stampBuilder = $this->createMock(HttpReceivedStampBuilder::class); + } - $email = (new Email()) - ->from('from@example.com') - ->to('to@example.com') - ->subject('Subject') - ->text('Body'); + private function email(): Email + { + return (new Email())->from('from@example.com')->to('to@example.com')->subject('S')->text('B'); + } - $event = new MessageEvent($email, Envelope::create($email), 'test'); + public function testNoHeaderWhenNoStamp(): void + { + $this->stampBuilder->method('buildStamp')->willReturn(null); + $email = $this->email(); - $subscriber->onMessage($event); + (new InjectedByHeaderSubscriber($this->stampBuilder)) + ->onMessage(new MessageEvent($email, Envelope::create($email), 'test')); - $this->assertFalse( - $email->getHeaders()->has('X-phpList-Injected-By'), - 'Header must not be added when there is no current Request.' - ); + self::assertFalse($email->getHeaders()->has('Received')); } - public function testNoHeaderWhenMessageIsNotEmail(): void + public function testIgnoresNonEmailMessages(): void { - $requestStack = new RequestStack(); - // Push a Request to ensure the early return is due to non-Email message, not missing request - $requestStack->push(new Request(server: ['REQUEST_TIME' => time()])); - - $subscriber = new InjectedByHeaderSubscriber($requestStack); - - $raw = new RawMessage('raw'); - // Create an arbitrary envelope; it does not need to match the message class + $this->stampBuilder->expects(self::never())->method('buildStamp'); $envelope = new Envelope(new Address('from@example.com'), [new Address('to@example.com')]); - $event = new MessageEvent($raw, $envelope, 'test'); - // RawMessage has no headers; the subscriber should return early - $subscriber->onMessage($event); - // sanity check to use the variable - $this->assertSame('raw', $raw->toString()); - // Nothing to assert on headers (RawMessage has none), but the lack of exceptions is a success + (new InjectedByHeaderSubscriber($this->stampBuilder)) + ->onMessage(new MessageEvent(new RawMessage('raw'), $envelope, 'test')); + $this->addToAssertionCount(1); } - public function testNoHeaderWhenRunningInCliEvenWithRequestAndEmail(): void + public function testAddsReceivedHeader(): void { - // In PHPUnit, PHP_SAPI is typically "cli"; ensure we have a Request to pass other guards - $request = new Request(server: [ - 'REQUEST_TIME' => time(), - 'REMOTE_ADDR' => '127.0.0.1', - ]); - $requestStack = new RequestStack(); - $requestStack->push($request); - - $subscriber = new InjectedByHeaderSubscriber($requestStack); - - $email = (new Email()) - ->from('from@example.com') - ->to('to@example.com') - ->subject('Subject') - ->text('Body'); - - $event = new MessageEvent($email, Envelope::create($email), 'test'); - - $subscriber->onMessage($event); - - // Because tests run under CLI SAPI, the header must not be added - $this->assertFalse( - $email->getHeaders()->has('X-phpList-Injected-By'), - 'Header must not be added when running under CLI.' + $this->stampBuilder->method('buildStamp')->willReturn('from [1.2.3.4] by host with HTTP; date'); + $email = $this->email(); + + (new InjectedByHeaderSubscriber($this->stampBuilder)) + ->onMessage(new MessageEvent($email, Envelope::create($email), 'test')); + + self::assertSame( + 'from [1.2.3.4] by host with HTTP; date', + $email->getHeaders()->get('Received')->getBodyAsString() ); } + + public function testReplacesExistingReceivedHeader(): void + { + $this->stampBuilder->method('buildStamp')->willReturn('new'); + $email = $this->email(); + $email->getHeaders()->addTextHeader('Received', 'old'); + + (new InjectedByHeaderSubscriber($this->stampBuilder)) + ->onMessage(new MessageEvent($email, Envelope::create($email), 'test')); + + self::assertCount(1, iterator_to_array($email->getHeaders()->all('Received'))); + self::assertSame('new', $email->getHeaders()->get('Received')->getBodyAsString()); + } } diff --git a/tests/Unit/Domain/Messaging/Service/Builder/ForwardEmailBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Builder/ForwardEmailBuilderTest.php index 2ca82e741..69c598d76 100644 --- a/tests/Unit/Domain/Messaging/Service/Builder/ForwardEmailBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Builder/ForwardEmailBuilderTest.php @@ -15,7 +15,6 @@ use PhpList\Core\Domain\Messaging\Model\Dto\MessagePrecacheDto; use PhpList\Core\Domain\Messaging\Service\AttachmentAdder; use PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder; -use PhpList\Core\Domain\Messaging\Service\Builder\HttpReceivedStampBuilder; use PhpList\Core\Domain\Messaging\Service\Constructor\CampaignMailContentBuilder; use PhpList\Core\Domain\Messaging\Service\TemplateImageEmbedder; use PhpList\Core\Domain\Subscription\Model\Subscriber; @@ -42,7 +41,6 @@ class ForwardEmailBuilderTest extends TestCase private PdfGenerator&MockObject $pdfGenerator; private AttachmentAdder&MockObject $attachmentAdder; private TranslatorInterface&MockObject $translator; - private HttpReceivedStampBuilder&MockObject $httpReceivedStampBuilder; protected function setUp(): void { @@ -61,7 +59,6 @@ protected function setUp(): void $this->pdfGenerator = $this->createMock(PdfGenerator::class); $this->attachmentAdder = $this->createMock(AttachmentAdder::class); $this->translator = $this->createMock(TranslatorInterface::class); - $this->httpReceivedStampBuilder = $this->createMock(HttpReceivedStampBuilder::class); // Defaults for config values used in headers $this->configProvider->method('getValue')->willReturnMap([ @@ -97,7 +94,6 @@ private function makeBuilder( pdfGenerator: $this->pdfGenerator, attachmentAdder: $this->attachmentAdder, translator: $this->translator, - httpReceivedStampBuilder: $this->httpReceivedStampBuilder, googleSenderId: $googleSenderId, useAmazonSes: $useAmazonSes, usePrecedenceHeader: $usePrecedenceHeader, @@ -139,10 +135,6 @@ public function testBuildsForwardEmailWithSubjectPrefixHeadersAndReplyTo(): void ->with($this->isInstanceOf(Email::class), 99, OutputFormat::Html, true) ->willReturn(true); - $this->httpReceivedStampBuilder - ->method('buildStamp') - ->willReturn('from host [127.0.0.1] by example.org with HTTP; Wed, 01 Jan 2025 00:00:00 +0000'); - $builder = $this->makeBuilder(devVersion: true, devEmail: 'dev@example.com'); [$email, $sentAs] = $builder->buildForwardEmail( messageId: 99, @@ -164,9 +156,6 @@ public function testBuildsForwardEmailWithSubjectPrefixHeadersAndReplyTo(): void $this->assertSame($fromEmail, $email->getReplyTo()[0]->getAddress()); $this->assertSame($fromName, $email->getReplyTo()[0]->getName()); - // Received header present - $this->assertNotNull($email->getHeaders()->get('Received')); - // Dev mode reroutes recipient $this->assertSame('dev@example.com', $email->getTo()[0]->getAddress()); } diff --git a/tests/Unit/Domain/Messaging/Service/Builder/HttpReceivedStampBuilderTest.php b/tests/Unit/Domain/Messaging/Service/Builder/HttpReceivedStampBuilderTest.php index aeb0d73c8..155b87d95 100644 --- a/tests/Unit/Domain/Messaging/Service/Builder/HttpReceivedStampBuilderTest.php +++ b/tests/Unit/Domain/Messaging/Service/Builder/HttpReceivedStampBuilderTest.php @@ -14,7 +14,7 @@ class HttpReceivedStampBuilderTest extends TestCase public function testReturnsNullWhenNoRequest(): void { $stack = new RequestStack(); - $builder = new HttpReceivedStampBuilder($stack, 'api.example.test'); + $builder = new HttpReceivedStampBuilder($stack); self::assertNull($builder->buildStamp()); } @@ -26,7 +26,7 @@ public function testReturnsNullWhenNoClientIp(): void // Do not set REMOTE_ADDR to simulate missing client IP $stack->push($request); - $builder = new HttpReceivedStampBuilder($stack, 'api.example.test'); + $builder = new HttpReceivedStampBuilder($stack); self::assertNull($builder->buildStamp()); } @@ -40,9 +40,10 @@ public function testBuildsStampWithRemoteHostAndFixedTime(): void $request->server->set('REMOTE_HOST', 'client.example.org'); // Fix the request time for deterministic output (Unix epoch start) $request->server->set('REQUEST_TIME', 0); + $request->headers->set('Host', 'api.example.test'); $stack->push($request); - $builder = new HttpReceivedStampBuilder($stack, 'api.example.test'); + $builder = new HttpReceivedStampBuilder($stack); $stamp = $builder->buildStamp(); @@ -61,9 +62,10 @@ public function testBuildsStampWithIpOnlyNoReverseDns(): void // Ensure no REMOTE_HOST so builder attempts reverse DNS, which should fail and fallback to IP only $request->server->remove('REMOTE_HOST'); $request->server->set('REQUEST_TIME', 0); + $request->headers->set('Host', 'api.example.test'); $stack->push($request); - $builder = new HttpReceivedStampBuilder($stack, 'api.example.test'); + $builder = new HttpReceivedStampBuilder($stack); $stamp = $builder->buildStamp(); diff --git a/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php b/tests/Unit/Domain/Messaging/Service/CampaignEmailSenderTest.php index b673f442c..22ef85933 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 df8afd18c..d463ac9e0 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 28891f785..c038f7edc 100644 --- a/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php +++ b/tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php @@ -11,7 +11,7 @@ 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\MessagePrecacheService; use PhpList\Core\Domain\Messaging\Service\Builder\ForwardEmailBuilder; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PHPUnit\Framework\MockObject\MockObject; @@ -22,22 +22,31 @@ class ForwardContentServiceTest extends TestCase { private CacheInterface&MockObject $cache; - private MessageProcessingPreparator&MockObject $preparator; private ForwardEmailBuilder&MockObject $builder; + private MessagePrecacheService&MockObject $precacheService; protected function setUp(): void { $this->cache = $this->createMock(CacheInterface::class); - $this->preparator = $this->createMock(MessageProcessingPreparator::class); $this->builder = $this->createMock(ForwardEmailBuilder::class); + $this->precacheService = $this->createMock(MessagePrecacheService::class); + $this->precacheService->method('getCacheKey')->willReturnCallback( + fn (int $id, bool $forward = false, bool $test = false) => sprintf( + 'messaging.message.base.%d.%d.%d', + $id, + (int) $forward, + (int) $test + ) + ); } public function testThrowsWhenCacheMissing(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, + precacheService: $this->precacheService, + forwardAlternativeContent: true, ); $campaign = $this->createMock(Message::class); @@ -66,12 +75,13 @@ public function testThrowsWhenCacheMissing(): void ); } - public function testProcessesLinksAndDelegatesToBuilder(): void + public function testDelegatesCachedContentToBuilderWithoutClickTracking(): void { $service = new ForwardContentService( cache: $this->cache, - messagePreparator: $this->preparator, forwardEmailBuilder: $this->builder, + precacheService: $this->precacheService, + forwardAlternativeContent: true, ); $campaign = $this->createMock(Message::class); @@ -80,7 +90,6 @@ public function testProcessesLinksAndDelegatesToBuilder(): void $subscriber->setHtmlEmail(true); $cached = new MessagePrecacheDto(); - $processed = new MessagePrecacheDto(); $this->cache ->expects(self::once()) @@ -88,16 +97,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 +105,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/MessageDataManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/MessageDataManagerTest.php new file mode 100644 index 000000000..c4aa55905 --- /dev/null +++ b/tests/Unit/Domain/Messaging/Service/Manager/MessageDataManagerTest.php @@ -0,0 +1,118 @@ + */ + private array $stored = []; + + protected function setUp(): void + { + $this->repository = $this->createMock(MessageDataRepository::class); + $this->repository->method('findByIdAndName')->willReturnCallback( + fn (int $id, string $name): ?MessageData => $this->stored[$name] ?? null + ); + $entityManager = $this->createMock(EntityManagerInterface::class); + $entityManager->method('persist')->willReturnCallback(function (MessageData $data): void { + $this->stored[$data->getName()] = $data; + }); + + $this->manager = new MessageDataManager( + $this->repository, + $entityManager, + $this->createMock(HTMLPurifier::class) + ); + $this->campaign = $this->createMock(Message::class); + $this->campaign->method('getId')->willReturn(7); + } + + /** @return array */ + private function storedValues(): array + { + return array_map(static fn (MessageData $data): ?string => $data->getData(), $this->stored); + } + + public function testEveryEntryIsStoredUnderItsOwnName(): void + { + $this->manager->storeMessageData($this->campaign, [ + 'utm_source' => 'newsletter', + 'notify_start' => 'a@example.com', + 'excludelist' => [3, 'x', 5], + ]); + + $values = $this->storedValues(); + self::assertSame('newsletter', $values['utm_source']); + self::assertSame('a@example.com', $values['notify_start']); + self::assertSame('SER:' . serialize([0 => 3, 2 => 5]), $values['excludelist']); + self::assertSame(7, $this->stored['utm_source']->getId()); + } + + public function testBooleansAreStoredAsOneAndZero(): void + { + $this->manager->storeMessageData($this->campaign, ['google_track' => true, 'pk_source' => false]); + + self::assertSame(['google_track' => '1', 'pk_source' => '0'], $this->storedValues()); + } + + public function testCheckboxesAreResetBeforeTheSubmittedValuesAreStored(): void + { + $this->manager->storeMessageData( + $this->campaign, + ['google_track' => '1'], + ['google_track', 'pk_source'] + ); + + self::assertSame(['google_track' => '1', 'pk_source' => '0'], $this->storedValues()); + } + + public function testExistingValuesAreOverwritten(): void + { + $this->manager->storeMessageData($this->campaign, ['utm_source' => 'old']); + $this->manager->storeMessageData($this->campaign, ['utm_source' => 'new']); + + self::assertSame(['utm_source' => 'new'], $this->storedValues()); + } + + /** @return array, 1: string[]}> */ + public static function notWritableProvider(): array + { + return [ + 'system setting' => [['status' => 'submitted'], []], + 'message column' => [['subject' => 'x'], []], + 'session name' => [['PHPSESSID' => 'abc'], []], + 'checkbox' => [[], ['resetstats']], + ]; + } + + /** + * @dataProvider notWritableProvider + * @param array $data + * @param string[] $checkboxes + */ + public function testNothingIsStoredWhenAnyNameIsNotWritable(array $data, array $checkboxes): void + { + $this->expectException(InvalidArgumentException::class); + + try { + $this->manager->storeMessageData($this->campaign, ['utm_medium' => 'email'] + $data, $checkboxes); + } finally { + self::assertSame([], $this->storedValues()); + } + } +} diff --git a/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/TemplateImageManagerTest.php index 296e0c1db..4d7fb0472 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 b3d945aeb..b14466409 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/MessageForwardServiceTest.php b/tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php index 6eeab8901..0cd69c49d 100644 --- a/tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php +++ b/tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php @@ -58,6 +58,7 @@ private function createService(): MessageForwardService precacheService: $this->precache, adminNotifier: $this->notifier, forwardingStatsService: $this->stats, + forwardAlternativeContent: true, ); } diff --git a/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php b/tests/Unit/Domain/Messaging/Service/MessageProcessingPreparatorTest.php index 4cd2e800e..f598d636e 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 3c7ae4b3f..5bb700547 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 1b2a7bf67..ec46f9c1c 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); + } }