From 391a47fc248d5f45d5c0ac6b2bb665dabd831249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20=C5=BDold=C3=A1k?= Date: Mon, 21 Aug 2023 08:02:39 +0000 Subject: [PATCH] Added check for `clicked_at` to `mail:unsubscribe-inactive-users` at all times remp/novydenik#1115 --- CHANGELOG.md | 1 + .../UnsubscribeInactiveUsersCommand.php | 14 +- .../UnsubscribeInactiveUsersCommandTest.php | 145 +++++++++++++----- 3 files changed, 117 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 737295e92..145149f25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p - Removed `HermesException` for missing `mail_sender_id` from `ValidateCrmEmailHandler`. remp/remp#1291 - This is valid state. It was introduced as fix by commit https://github.com/remp2020/remp/commit/c2d55b5d7d56f7ba29e3977f33785d77b3ca145a - Added new `Mailer` segment provider which provides segments of users subscribed to mail types. remp/mnt#114 +- Added check for `clicked_at` to `mail:unsubscribe-inactive-users` at all times. remp/novydenik#1115 ## Archive diff --git a/Mailer/extensions/mailer-module/src/Commands/UnsubscribeInactiveUsersCommand.php b/Mailer/extensions/mailer-module/src/Commands/UnsubscribeInactiveUsersCommand.php index 31a9da9c9..a65e2ae3a 100644 --- a/Mailer/extensions/mailer-module/src/Commands/UnsubscribeInactiveUsersCommand.php +++ b/Mailer/extensions/mailer-module/src/Commands/UnsubscribeInactiveUsersCommand.php @@ -26,7 +26,7 @@ class UnsubscribeInactiveUsersCommand extends Command public const COMMAND_NAME = 'mail:unsubscribe-inactive-users'; private const CRM_SEGMENT_NAME = 'unsubscribe_inactive_users_from_newsletters_list'; - private const APPLE_BOT_EMAILS = 'apple_bot_emails'; + public const APPLE_BOT_EMAILS = 'apple_bot_emails'; private array $omitMailTypeCodes = ['system', 'system_optional']; @@ -93,13 +93,15 @@ protected function execute(InputInterface $input, OutputInterface $output): int $logCount = count($logs); if (count($logs) >= 5) { - $columnToCheck = 'opened_at'; - if ($this->redis()->sismember(self::APPLE_BOT_EMAILS, $userEmail)) { - $columnToCheck = 'clicked_at'; - } + $isAppleBotOpenedEmail = $this->redis()->sismember(self::APPLE_BOT_EMAILS, $userEmail); foreach ($logs as $log) { - if ($log->{$columnToCheck}) { + if ($isAppleBotOpenedEmail && $log->clicked_at) { + $output->writeln("Skipping, user is active."); + continue 2; + } + + if (!$isAppleBotOpenedEmail && ($log->opened_at || $log->clicked_at)) { $output->writeln("Skipping, user is active."); continue 2; } diff --git a/Mailer/extensions/mailer-module/src/Tests/Feature/Commands/UnsubscribeInactiveUsersCommandTest.php b/Mailer/extensions/mailer-module/src/Tests/Feature/Commands/UnsubscribeInactiveUsersCommandTest.php index 3f2defd85..36101cd56 100644 --- a/Mailer/extensions/mailer-module/src/Tests/Feature/Commands/UnsubscribeInactiveUsersCommandTest.php +++ b/Mailer/extensions/mailer-module/src/Tests/Feature/Commands/UnsubscribeInactiveUsersCommandTest.php @@ -5,6 +5,8 @@ use Nette\Database\Table\ActiveRow; use Nette\Utils\DateTime; use Remp\MailerModule\Commands\UnsubscribeInactiveUsersCommand; +use Remp\MailerModule\Models\RedisClientFactory; +use Remp\MailerModule\Models\RedisClientTrait; use Remp\MailerModule\Models\Segment\Aggregator; use Remp\MailerModule\Models\Segment\ISegment; use Symfony\Component\Console\Input\StringInput; @@ -14,6 +16,8 @@ class UnsubscribeInactiveUsersCommandTest extends BaseFeatureTestCase { + use RedisClientTrait; + protected ISegment $testSegmentProvider; protected Aggregator $segmentAggregator; @@ -22,6 +26,8 @@ class UnsubscribeInactiveUsersCommandTest extends BaseFeatureTestCase protected ActiveRow $mailLayout; + protected RedisClientFactory $redisClientFactory; + private array $user = [ 'id' => 1, 'email' => 'user1@example.com', @@ -54,6 +60,11 @@ public function setUp(): void $this->segmentAggregator = $this->inject(Aggregator::class); $this->segmentAggregator->register($this->testSegmentProvider); + $this->redisClientFactory = $this->inject(RedisClientFactory::class); + + // remove email from `APPLE_BOT_EMAILS` redis key before every test + $this->redis()->srem(UnsubscribeInactiveUsersCommand::APPLE_BOT_EMAILS, $this->user['email']); + $this->unsubscribeInactiveUsersCommand = $this->inject(UnsubscribeInactiveUsersCommand::class); $this->mailLayout = $this->createMailLayout(); @@ -81,10 +92,11 @@ public function dataProvider() 'TooLittleDeliveredEmails_ShouldNotUnsubscribe' => [ 'subscribe' => ['system', 'test'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], ], + 'isAppleBotOpenedEmail' => false, 'result' => [ 'system' => true, 'test' => true, @@ -93,13 +105,14 @@ public function dataProvider() 'TooManyNotOpenedEmails_ShouldUnsubscribe' => [ 'subscribe' => ['system', 'test', 'test-omit'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], - ['delivered' => '-8 days', 'opened' => null], - ['delivered' => '-7 days', 'opened' => null], - ['delivered' => '-6 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-8 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], ], + 'isAppleBotOpenedEmail' => false, 'result' => [ 'system' => true, 'test' => false, @@ -109,13 +122,14 @@ public function dataProvider() 'TooManyNotOpenedEmails_WithOmit_ShouldUnsubscribe' => [ 'subscribe' => ['system', 'test', 'test-omit'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], - ['delivered' => '-8 days', 'opened' => null], - ['delivered' => '-7 days', 'opened' => null], - ['delivered' => '-6 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-8 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], ], + 'isAppleBotOpenedEmail' => false, 'result' => [ 'system' => true, 'test' => false, @@ -126,13 +140,14 @@ public function dataProvider() 'TooManyNotOpenedEmails_DryRun_ShouldNotUnsubscribe' => [ 'subscribe' => ['system', 'test'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], - ['delivered' => '-8 days', 'opened' => null], - ['delivered' => '-7 days', 'opened' => null], - ['delivered' => '-6 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-8 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], ], + 'isAppleBotOpenedEmail' => false, 'result' => [ 'system' => true, 'test' => true, @@ -143,28 +158,78 @@ public function dataProvider() 'SomeOpenedDeliveries_ShouldNotUnsubscribe' => [ 'subscribe' => ['system', 'test'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], - ['delivered' => '-8 days', 'opened' => '-5 days'], - ['delivered' => '-7 days', 'opened' => null], - ['delivered' => '-6 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-8 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], + ], + 'isAppleBotOpenedEmail' => false, + 'result' => [ + 'system' => true, + 'test' => true, + ], + ], + 'SomeClickedNotOpenedDeliveries_ShouldNotUnsubscribe' => [ + 'subscribe' => ['system', 'test'], + 'logs' => [ + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => '-5 days'], + ['delivered' => '-8 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], + ], + 'isAppleBotOpenedEmail' => false, + 'result' => [ + 'system' => true, + 'test' => true, + ], + ], + 'AppleBotOpenedClickedDeliveries_ShouldNotUnsubscribe' => [ + 'subscribe' => ['system', 'test'], + 'logs' => [ + ['delivered' => '-11 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-10 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-9 days', 'opened' => '-5 days', 'clicked' => '-5 days'], + ['delivered' => '-8 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-7 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-6 days', 'opened' => '-5 days', 'clicked' => null], ], + 'isAppleBotOpenedEmail' => true, 'result' => [ 'system' => true, 'test' => true, ], ], + 'AppleBotOpenedNotClickedDeliveries_ShouldUnsubscribe' => [ + 'subscribe' => ['system', 'test'], + 'logs' => [ + ['delivered' => '-11 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-10 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-9 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-8 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-7 days', 'opened' => '-5 days', 'clicked' => null], + ['delivered' => '-6 days', 'opened' => '-5 days', 'clicked' => null], + ], + 'isAppleBotOpenedEmail' => true, + 'result' => [ + 'system' => true, + 'test' => false, + ], + ], 'NotMatchedDeliveryThresholdWithinSelectedPeriod_ShouldNotUnsubscribe' => [ 'subscribe' => ['system', 'test', 'test-omit'], 'logs' => [ - ['delivered' => '-11 days', 'opened' => null], - ['delivered' => '-10 days', 'opened' => null], - ['delivered' => '-9 days', 'opened' => null], - ['delivered' => '-8 days', 'opened' => null], - ['delivered' => '-7 days', 'opened' => null], - ['delivered' => '-6 days', 'opened' => null], + ['delivered' => '-11 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-10 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-9 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-8 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-7 days', 'opened' => null, 'clicked' => null], + ['delivered' => '-6 days', 'opened' => null, 'clicked' => null], ], + 'isAppleBotOpenedEmail' => false, 'result' => [ 'system' => true, 'test' => true, @@ -180,7 +245,7 @@ public function dataProvider() /** * @dataProvider dataProvider */ - public function testUnsubscribeInactive(array $subscribe, array $logs, array $result, array $omit = [], bool $dryRun = false, int $days = null) + public function testUnsubscribeInactive(array $subscribe, array $logs, bool $isAppleBotOpenedEmail, array $result, array $omit = [], bool $dryRun = false, int $days = null) { foreach ($subscribe as $mailTypeCode) { $this->susbcribeUser($this->user, $mailTypeCode); @@ -190,7 +255,12 @@ public function testUnsubscribeInactive(array $subscribe, array $logs, array $re $template = $this->templatesRepository->findBy('code', 'test'); $delivered = DateTime::from($log['delivered']); $opened = $log['opened'] ? DateTime::from($log['opened']) : null; - $this->addMailLog($this->user, $template, $delivered, $opened); + $clicked = $log['clicked'] ? DateTime::from($log['clicked']) : null; + $this->addMailLog($this->user, $template, $delivered, $opened, $clicked); + } + + if ($isAppleBotOpenedEmail) { + $this->redis()->sadd(UnsubscribeInactiveUsersCommand::APPLE_BOT_EMAILS, [$this->user['email']]); } $input = '--segment-provider ' . TestSegmentProvider::PROVIDER_ALIAS; @@ -220,7 +290,7 @@ private function susbcribeUser(array $user, string $mailTypeCode) $this->userSubscriptionsRepository->subscribeUser($list, $user['id'], $user['email']); } - private function addMailLog(array $user, ActiveRow $template, DateTime $deliveredAt, ?DateTime $openedAt = null) + private function addMailLog(array $user, ActiveRow $template, DateTime $deliveredAt, ?DateTime $openedAt = null, ?DateTime $clickedAt = null) { $mailLog = $this->mailLogsRepository->add( email: $user['email'], @@ -231,7 +301,8 @@ private function addMailLog(array $user, ActiveRow $template, DateTime $delivere $this->mailLogsRepository->update($mailLog, [ 'delivered_at' => $deliveredAt, - 'opened_at' => $openedAt + 'opened_at' => $openedAt, + 'clicked_at' => $clickedAt, ]); } }