Cavemanon/piratepoet
7
Fork 0
Code Pull Requests Actions Releases 16 Activity

Improve comments and tests for inactive subscribers

Browse Source 
[MAILPOET-4081]
This commit is contained in:
Brezo Cordero
2022-03-09 17:14:23 -06:00
committed by Veljko V
parent 6e6720d9f1
commit 5dc215efa7
2 changed files with 36 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
mailpoet/lib/Subscribers/InactiveSubscribersController.php
View File

@@ -16,7 +16,7 @@ class InactiveSubscribersController {
const UNOPENED_EMAILS_THRESHOLD = 3; const UNOPENED_EMAILS_THRESHOLD = 3;
private $inactiveTaskIdsTableCreated = false; private $processedTaskIdsTableCreated = false;
/** @var SettingsRepository */ /** @var SettingsRepository */
private $settingsRepository; private $settingsRepository;
@@ -79,24 +79,25 @@ class InactiveSubscribersController {
return false; return false;
} }
$inactiveTaskIdsTable = 'inactive_task_ids'; // Temporary table with processed tasks from threshold date up to yesterday
if (!$this->inactiveTaskIdsTableCreated) { $processedTaskIdsTable = 'inactive_task_ids';
$inactiveTaskIdsTableSql = " if (!$this->processedTaskIdsTableCreated) {
CREATE TEMPORARY TABLE IF NOT EXISTS {$inactiveTaskIdsTable} $processedTaskIdsTableSql = "
CREATE TEMPORARY TABLE IF NOT EXISTS {$processedTaskIdsTable}
(INDEX task_id_ids (id)) (INDEX task_id_ids (id))
SELECT DISTINCT task_id as id FROM {$sendingQueuesTable} as sq SELECT DISTINCT task_id as id FROM {$sendingQueuesTable} as sq
JOIN {$scheduledTasksTable} as st ON sq.task_id = st.id JOIN {$scheduledTasksTable} as st ON sq.task_id = st.id
WHERE st.processed_at > :thresholdDate WHERE st.processed_at > :thresholdDate
AND st.processed_at < :dayAgo AND st.processed_at < :dayAgo
"; ";
$connection->executeQuery($inactiveTaskIdsTableSql, [ $connection->executeQuery($processedTaskIdsTableSql, [
'thresholdDate' => $thresholdDateIso, 'thresholdDate' => $thresholdDateIso,
'dayAgo' => $dayAgoIso, 'dayAgo' => $dayAgoIso,
]); ]);
$this->inactiveTaskIdsTableCreated = true; $this->processedTaskIdsTableCreated = true;
} }
// Select subscribers who received at least a number of emails after threshold date // Select subscribers who received at least a number of emails after threshold date and subscribed before that
$startId = (int)$startId; $startId = (int)$startId;
$endId = $startId + $batchSize; $endId = $startId + $batchSize;
$inactiveSubscriberIdsTmpTable = 'inactive_subscriber_ids'; $inactiveSubscriberIdsTmpTable = 'inactive_subscriber_ids';
@@ -105,7 +106,7 @@ class InactiveSubscribersController {
(UNIQUE subscriber_id (id)) (UNIQUE subscriber_id (id))
SELECT s.id FROM {$subscribersTable} as s SELECT s.id FROM {$subscribersTable} as s
JOIN {$scheduledTaskSubscribersTable} as sts USE INDEX (subscriber_id) ON s.id = sts.subscriber_id JOIN {$scheduledTaskSubscribersTable} as sts USE INDEX (subscriber_id) ON s.id = sts.subscriber_id
JOIN {$inactiveTaskIdsTable} task_ids ON task_ids.id = sts.task_id JOIN {$processedTaskIdsTable} task_ids ON task_ids.id = sts.task_id
WHERE s.last_subscribed_at < :thresholdDate WHERE s.last_subscribed_at < :thresholdDate
AND s.status = :status AND s.status = :status
AND s.id >= :startId AND s.id >= :startId

62
mailpoet/tests/integration/Subscribers/InactiveSubscribersControllerTest.php
View File

@@ -28,7 +28,7 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
const INACTIVITY_DAYS_THRESHOLD = 5; const INACTIVITY_DAYS_THRESHOLD = 5;
const PROCESS_BATCH_SIZE = 100; const PROCESS_BATCH_SIZE = 100;
const UNOPENED_EMAILS_THRESHOLD = 1; const UNOPENED_EMAILS_THRESHOLD = InactiveSubscribersController::UNOPENED_EMAILS_THRESHOLD;
public function _before() { public function _before() {
$this->controller = new InactiveSubscribersController( $this->controller = new InactiveSubscribersController(
@@ -52,19 +52,12 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
parent::_before(); parent::_before();
} }
public function testItDeactivatesOldSubscribersOnlyWhenUnopenedEmailsReachDefaultThreshold(): void { public function testItDeactivatesOldSubscribersOnlyWhenUnopenedEmailsReachThreshold(): void {
// Create three completed sending tasks
[$task] = $this->createCompletedSendingTask(3);
[$task2] = $this->createCompletedSendingTask(3);
[$task3] = $this->createCompletedSendingTask(3);
$subscriber1 = $this->createSubscriber('s1@email.com', 10); $subscriber1 = $this->createSubscriber('s1@email.com', 10);
$this->addSubscriberToTask($subscriber1, $task); $this->createCompletedSendingTasksForSubscriber($subscriber1, self::UNOPENED_EMAILS_THRESHOLD, 3);
$this->addSubscriberToTask($subscriber1, $task2);
$this->addSubscriberToTask($subscriber1, $task3);
$subscriber2 = $this->createSubscriber('s2@email.com', 10); $subscriber2 = $this->createSubscriber('s2@email.com', 10);
$this->addSubscriberToTask($subscriber2, $task); $this->createCompletedSendingTasksForSubscriber($subscriber2, self::UNOPENED_EMAILS_THRESHOLD - 1, 3);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(1); expect($result)->equals(1);
@@ -78,15 +71,13 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
} }
public function testItDeactivatesLimitedAmountOfSubscribers(): void { public function testItDeactivatesLimitedAmountOfSubscribers(): void {
[$task] = $this->createCompletedSendingTask(3);
$subscriber1 = $this->createSubscriber('s1@email.com', 10); $subscriber1 = $this->createSubscriber('s1@email.com', 10);
$this->addSubscriberToTask($subscriber1, $task); $this->createCompletedSendingTasksForSubscriber($subscriber1, self::UNOPENED_EMAILS_THRESHOLD, 3);
$subscriber2 = $this->createSubscriber('s2@email.com', 10); $subscriber2 = $this->createSubscriber('s2@email.com', 10);
$this->addSubscriberToTask($subscriber2, $task); $this->createCompletedSendingTasksForSubscriber($subscriber2, self::UNOPENED_EMAILS_THRESHOLD, 3);
$batchSize = 1; $batchSize = 1;
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, $batchSize, $subscriber1->getId(), self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, $batchSize, $subscriber1->getId());
$this->entityManager->clear(); $this->entityManager->clear();
expect($result)->equals(1); expect($result)->equals(1);
$subscriber1 = $this->subscribersRepository->findOneById($subscriber1->getId()); $subscriber1 = $this->subscribersRepository->findOneById($subscriber1->getId());
@@ -108,12 +99,10 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
} }
public function testItDoesNotDeactivateNewSubscriberWithUnopenedEmail(): void { public function testItDoesNotDeactivateNewSubscriberWithUnopenedEmail(): void {
[$task] = $this->createCompletedSendingTask(3);
$subscriber = $this->createSubscriber('s1@email.com', 3); $subscriber = $this->createSubscriber('s1@email.com', 3);
$this->addSubscriberToTask($subscriber, $task); $this->createCompletedSendingTasksForSubscriber($subscriber, self::UNOPENED_EMAILS_THRESHOLD, 3);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -121,15 +110,13 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
} }
public function testItDoesNotDeactivateNewlyResubscribedSubscriberWithUnopenedEmail(): void { public function testItDoesNotDeactivateNewlyResubscribedSubscriberWithUnopenedEmail(): void {
[$task] = $this->createCompletedSendingTask(3);
$subscriber = $this->createSubscriber('s1@email.com', 10); $subscriber = $this->createSubscriber('s1@email.com', 10);
$lastSubscribedAt = (new Carbon())->subDays(2); $lastSubscribedAt = (new Carbon())->subDays(2);
$subscriber->setLastSubscribedAt($lastSubscribedAt); $subscriber->setLastSubscribedAt($lastSubscribedAt);
$this->entityManager->flush(); $this->entityManager->flush();
$this->addSubscriberToTask($subscriber, $task); $this->createCompletedSendingTasksForSubscriber($subscriber, self::UNOPENED_EMAILS_THRESHOLD, 3);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -139,7 +126,7 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
public function testItDoesNotDeactivateSubscriberWithoutSentEmail(): void { public function testItDoesNotDeactivateSubscriberWithoutSentEmail(): void {
$this->createCompletedSendingTask(3); $this->createCompletedSendingTask(3);
$subscriber = $this->createSubscriber('s1@email.com', 10); $subscriber = $this->createSubscriber('s1@email.com', 10);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -151,9 +138,8 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
$subscriber = $this->createSubscriber('s1@email.com', 10); $subscriber = $this->createSubscriber('s1@email.com', 10);
$this->addSubscriberToTask($subscriber, $task); $this->addSubscriberToTask($subscriber, $task);
$this->addEmailOpenedRecord($subscriber, $queue, 2); $this->addEmailOpenedRecord($subscriber, $queue, 2);
[$task2] = $this->createCompletedSendingTask(2); $this->createCompletedSendingTasksForSubscriber($subscriber, self::UNOPENED_EMAILS_THRESHOLD, 3);
$this->addSubscriberToTask($subscriber, $task2); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -161,10 +147,9 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
} }
public function testItDoesNotDeactivateSubscriberWhoReceivedEmailRecently(): void { public function testItDoesNotDeactivateSubscriberWhoReceivedEmailRecently(): void {
[$task] = $this->createCompletedSendingTask(0);
$subscriber = $this->createSubscriber('s1@email.com', 10); $subscriber = $this->createSubscriber('s1@email.com', 10);
$this->addSubscriberToTask($subscriber, $task); $this->createCompletedSendingTasksForSubscriber($subscriber, self::UNOPENED_EMAILS_THRESHOLD, 0);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -172,14 +157,12 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
} }
public function testItDoesNotDeactivatesSubscribersWhenMP2MigrationHappenedWithinInterval(): void { public function testItDoesNotDeactivatesSubscribersWhenMP2MigrationHappenedWithinInterval(): void {
[$task] = $this->createCompletedSendingTask(3);
$this->createSetting(MP2Migrator::MIGRATION_COMPLETE_SETTING_KEY, true, (new Carbon())->subDays(3)); $this->createSetting(MP2Migrator::MIGRATION_COMPLETE_SETTING_KEY, true, (new Carbon())->subDays(3));
$subscriber = $this->createSubscriber('s1@email.com', 10); $subscriber = $this->createSubscriber('s1@email.com', 10);
$this->addSubscriberToTask($subscriber, $task); $this->createCompletedSendingTasksForSubscriber($subscriber, self::UNOPENED_EMAILS_THRESHOLD, 3);
$result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE, null, self::UNOPENED_EMAILS_THRESHOLD); $result = $this->controller->markInactiveSubscribers(self::INACTIVITY_DAYS_THRESHOLD, self::PROCESS_BATCH_SIZE);
expect($result)->equals(0); expect($result)->equals(0);
$subscriber = $this->subscribersRepository->findOneById($subscriber->getId()); $subscriber = $this->subscribersRepository->findOneById($subscriber->getId());
assert($subscriber instanceof SubscriberEntity); assert($subscriber instanceof SubscriberEntity);
@@ -288,6 +271,13 @@ class InactiveSubscribersControllerTest extends \MailPoetTest {
return $subscriber; return $subscriber;
} }
private function createCompletedSendingTasksForSubscriber(SubscriberEntity $subscriber, int $numTasks = 1, int $processedDaysAgo = 0): void {
for ($i = 0; $i < $numTasks; $i++) {
[$task] = $this->createCompletedSendingTask($processedDaysAgo);
$this->addSubscriberToTask($subscriber, $task);
}
}
private function createCompletedSendingTask(int $processedDaysAgo = 0): array { private function createCompletedSendingTask(int $processedDaysAgo = 0): array {
$processedAt = (new Carbon())->subDays($processedDaysAgo); $processedAt = (new Carbon())->subDays($processedDaysAgo);
$task = new ScheduledTaskEntity(); $task = new ScheduledTaskEntity();