diff --git a/mailpoet/lib/Segments/WP.php b/mailpoet/lib/Segments/WP.php index 2cdb95a01f..32680c9deb 100644 --- a/mailpoet/lib/Segments/WP.php +++ b/mailpoet/lib/Segments/WP.php @@ -6,7 +6,6 @@ use MailPoet\Config\SubscriberChangesNotifier; use MailPoet\DI\ContainerWrapper; use MailPoet\Entities\SegmentEntity; use MailPoet\Entities\SubscriberEntity; -use MailPoet\Models\Segment; use MailPoet\Models\Subscriber; use MailPoet\Models\SubscriberSegment; use MailPoet\Newsletter\Scheduler\WelcomeScheduler; @@ -43,6 +42,8 @@ class WP { /** @var Validator */ private $validator; + private $segmentsRepository; + public function __construct( WPFunctions $wp, WelcomeScheduler $welcomeScheduler, @@ -50,7 +51,8 @@ class WP { SubscribersRepository $subscribersRepository, SubscriberSegmentRepository $subscriberSegmentRepository, SubscriberChangesNotifier $subscriberChangesNotifier, - Validator $validator + Validator $validator, + SegmentsRepository $segmentsRepository ) { $this->wp = $wp; $this->welcomeScheduler = $welcomeScheduler; @@ -59,6 +61,7 @@ class WP { $this->subscriberSegmentRepository = $subscriberSegmentRepository; $this->subscriberChangesNotifier = $subscriberChangesNotifier; $this->validator = $validator; + $this->segmentsRepository = $segmentsRepository; } /** @@ -102,8 +105,7 @@ class WP { */ private function createOrUpdateSubscriber(string $currentFilter, \WP_User $wpUser, $subscriber = false, $oldWpUserData = false): void { // Add or update - $wpSegment = Segment::getWPSegment(); - if (!$wpSegment) return; + $wpSegment = $this->segmentsRepository->getWPUsersSegment(); // find subscriber by email when is false if (!$subscriber) { @@ -138,7 +140,7 @@ class WP { unset($data['source']); // don't override status for existing users } - $addingNewUserToDisabledWPSegment = $wpSegment->deletedAt !== null && $currentFilter === 'user_register'; + $addingNewUserToDisabledWPSegment = $wpSegment->getDeletedAt() !== null && $currentFilter === 'user_register'; $otherActiveSegments = []; if ($subscriber) { @@ -160,13 +162,13 @@ class WP { // add subscriber to the WP Users segment SubscriberSegment::subscribeToSegments( $subscriber, - [$wpSegment->id] + [$wpSegment->getId()] ); if (!$signupConfirmationEnabled && $subscriber->status === Subscriber::STATUS_SUBSCRIBED && $currentFilter === 'user_register') { $subscriberSegment = $this->subscriberSegmentRepository->findOneBy([ 'subscriber' => $subscriber->id(), - 'segment' => $wpSegment->id(), + 'segment' => $wpSegment->getId(), ]); if (!is_null($subscriberSegment)) { @@ -264,9 +266,9 @@ class WP { private function insertSubscribers(): array { global $wpdb; - $wpSegment = Segment::getWPSegment(); - if (!$wpSegment) return []; - if ($wpSegment->deletedAt !== null) { + $wpSegment = $this->segmentsRepository->getWPUsersSegment(); + + if ($wpSegment->getDeletedAt() !== null) { $subscriberStatus = SubscriberEntity::STATUS_UNCONFIRMED; $deletedAt = 'CURRENT_TIMESTAMP()'; } else { @@ -338,28 +340,17 @@ class WP { } private function insertUsersToSegment(): void { - $wpSegment = Segment::getWPSegment(); + $wpSegment = $this->segmentsRepository->getWPUsersSegment(); $subscribersTable = Subscriber::$_table; $wpMailpoetSubscriberSegmentTable = SubscriberSegment::$_table; Subscriber::rawExecute(sprintf(' INSERT IGNORE INTO %s(subscriber_id, segment_id, created_at) SELECT mps.id, "%s", CURRENT_TIMESTAMP() FROM %s mps WHERE mps.wp_user_id > 0 - ', $wpMailpoetSubscriberSegmentTable, $wpSegment->id, $subscribersTable)); + ', $wpMailpoetSubscriberSegmentTable, $wpSegment->getId(), $subscribersTable)); } private function removeOrphanedSubscribers(): void { - // remove orphaned wp segment subscribers (not having a matching wp user id), - // e.g. if wp users were deleted directly from the database - global $wpdb; - - $wpSegment = Segment::getWPSegment(); - - $wpSegment->subscribers() - ->leftOuterJoin($wpdb->users, [MP_SUBSCRIBERS_TABLE . '.wp_user_id', '=', 'wu.id'], 'wu') - ->whereRaw('(wu.id IS NULL OR ' . MP_SUBSCRIBERS_TABLE . '.email = "")') - ->findResultSet() - ->set('wp_user_id', null) - ->delete(); + $this->subscribersRepository->removeOrphanedSubscribersFromWpSegment(); } } diff --git a/mailpoet/lib/Subscribers/SubscribersRepository.php b/mailpoet/lib/Subscribers/SubscribersRepository.php index 79284d6766..8489242b43 100644 --- a/mailpoet/lib/Subscribers/SubscribersRepository.php +++ b/mailpoet/lib/Subscribers/SubscribersRepository.php @@ -12,6 +12,7 @@ use MailPoet\Entities\SubscriberEntity; use MailPoet\Entities\SubscriberSegmentEntity; use MailPoet\Entities\SubscriberTagEntity; use MailPoet\Entities\TagEntity; +use MailPoet\Segments\SegmentsRepository; use MailPoet\Util\License\Features\Subscribers; use MailPoet\WP\Functions as WPFunctions; use MailPoetVendor\Carbon\Carbon; @@ -38,14 +39,19 @@ class SubscribersRepository extends Repository { /** @var SubscriberChangesNotifier */ private $changesNotifier; + /** @var SegmentsRepository */ + private $segmentsRepository; + public function __construct( EntityManager $entityManager, SubscriberChangesNotifier $changesNotifier, - WPFunctions $wp + WPFunctions $wp, + SegmentsRepository $segmentsRepository ) { $this->wp = $wp; parent::__construct($entityManager); $this->changesNotifier = $changesNotifier; + $this->segmentsRepository = $segmentsRepository; } protected function getEntityClassName() { @@ -553,6 +559,24 @@ class SubscribersRepository extends Repository { return $count; } + public function removeOrphanedSubscribersFromWpSegment(): void { + global $wpdb; + + $segmentId = $this->segmentsRepository->getWpUsersSegment()->getId(); + + $subscribersTable = $this->entityManager->getClassMetadata(SubscriberEntity::class)->getTableName(); + $subscriberSegmentsTable = $this->entityManager->getClassMetadata(SubscriberSegmentEntity::class)->getTableName(); + + $this->entityManager->getConnection()->executeStatement( + "DELETE s + FROM {$subscribersTable} s + INNER JOIN {$subscriberSegmentsTable} ss ON s.id = ss.subscriber_id + LEFT JOIN {$wpdb->users} u ON s.wp_user_id = u.id + WHERE ss.segment_id = :segmentId AND (u.id IS NULL OR s.email = '')", + ['segmentId' => $segmentId], ['segmentId' => \PDO::PARAM_INT] + ); + } + /** * @return int - number of processed ids */ diff --git a/mailpoet/tests/_support/IntegrationTester.php b/mailpoet/tests/_support/IntegrationTester.php index f7b32309d8..46412548e3 100644 --- a/mailpoet/tests/_support/IntegrationTester.php +++ b/mailpoet/tests/_support/IntegrationTester.php @@ -83,6 +83,19 @@ class IntegrationTester extends \Codeception\Actor { return $userId; } + /** + * Deletes a WP user directly from the database without triggering any hooks. + * Needed to be able to test deleting orphaned subscribers. + */ + public function deleteWPUserFromDatabase(int $id): void { + global $wpdb; + + $this->entityManager->getConnection()->executeStatement( + "DELETE FROM {$wpdb->users} WHERE id = :id", + ['id' => $id], ['id' => \PDO::PARAM_INT] + ); + } + public function createWordPressTerm(string $term, string $taxonomy, array $args = []): int { $term = wp_insert_term($term, $taxonomy, $args); if ($term instanceof WP_Error) { diff --git a/mailpoet/tests/integration/Segments/WPTest.php b/mailpoet/tests/integration/Segments/WPTest.php index 3f0dcfa03b..79c75172ca 100644 --- a/mailpoet/tests/integration/Segments/WPTest.php +++ b/mailpoet/tests/integration/Segments/WPTest.php @@ -246,7 +246,7 @@ class WPTest extends \MailPoetTest { $this->updateWPUserEmail($id, ''); $this->wpSegment->synchronizeUsers(); verify($this->subscribersRepository->findOneBy(['wpUserId' => $id]))->null(); - $this->deleteWPUser($id); + $this->tester->deleteWPUserFromDatabase($id); } public function testRemovesUsersWithInvalidEmailsFromSunscribersDuringSynchronization(): void { @@ -255,7 +255,7 @@ class WPTest extends \MailPoetTest { $this->updateWPUserEmail($id, 'ivalid.@email.com'); $this->wpSegment->synchronizeUsers(); verify($this->subscribersRepository->findOneBy(['wpUserId' => $id]))->null(); - $this->deleteWPUser($id); + $this->tester->deleteWPUserFromDatabase($id); } public function testItDoesNotSynchronizeEmptyEmailsForNewUsers(): void { @@ -263,7 +263,7 @@ class WPTest extends \MailPoetTest { $this->updateWPUserEmail($id, ''); $this->wpSegment->synchronizeUsers(); verify($this->subscribersRepository->findOneBy(['wpUserId' => $id]))->null(); - $this->deleteWPUser($id); + $this->tester->deleteWPUserFromDatabase($id); } public function testItSynchronizeFirstNames(): void { @@ -372,7 +372,7 @@ class WPTest extends \MailPoetTest { $this->insertUser(); $id = $this->insertUser(); $this->wpSegment->synchronizeUsers(); - $this->deleteWPUser($id); + $this->tester->deleteWPUserFromDatabase($id); $this->wpSegment->synchronizeUsers(); $subscribers = $this->subscribersRepository->findBy(['wpUserId' => $this->userIds]); verify(count($subscribers))->equals(1); @@ -722,17 +722,6 @@ class WPTest extends \MailPoetTest { ', $wpdb->users, $name, $id)); } - private function deleteWPUser(int $id): void { - global $wpdb; - $db = ORM::getDb(); - $db->exec(sprintf(' - DELETE FROM - %s - WHERE - id = %s - ', $wpdb->users, $id)); - } - private function clearEmail(SubscriberEntity $subscriber): void { $this->connection->executeStatement( 'UPDATE ' . MP_SUBSCRIBERS_TABLE . ' diff --git a/mailpoet/tests/integration/Statistics/Track/ClicksTest.php b/mailpoet/tests/integration/Statistics/Track/ClicksTest.php index e3f73173ae..fe0dedc800 100644 --- a/mailpoet/tests/integration/Statistics/Track/ClicksTest.php +++ b/mailpoet/tests/integration/Statistics/Track/ClicksTest.php @@ -509,7 +509,12 @@ class ClicksTest extends \MailPoetTest { $clicksRepository = $this->diContainer->get(StatisticsClicksRepository::class); $data = $this->trackData; $data->userAgent = 'User Agent'; - $subscribersRepository = new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock); + $subscribersRepository = $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ); $statisticsOpensRepository = $this->diContainer->get(StatisticsOpensRepository::class); $opens = new Opens( $statisticsOpensRepository, @@ -547,7 +552,12 @@ class ClicksTest extends \MailPoetTest { $clicksRepository = $this->diContainer->get(StatisticsClicksRepository::class); $data = $this->trackData; $data->userAgent = null; - $subscribersRepository = new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock); + $subscribersRepository = $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ); $statisticsOpensRepository = $this->diContainer->get(StatisticsOpensRepository::class); $opens = new Opens( $statisticsOpensRepository, @@ -585,7 +595,12 @@ class ClicksTest extends \MailPoetTest { $clicksRepository = $this->diContainer->get(StatisticsClicksRepository::class); $data = $this->trackData; $data->userAgent = UserAgentEntity::MACHINE_USER_AGENTS[0]; - $subscribersRepository = new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock); + $subscribersRepository = $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ); $statisticsOpensRepository = $this->diContainer->get(StatisticsOpensRepository::class); $opens = new Opens( $statisticsOpensRepository, diff --git a/mailpoet/tests/integration/Statistics/Track/OpensTest.php b/mailpoet/tests/integration/Statistics/Track/OpensTest.php index 088bf493eb..a1c1578c61 100644 --- a/mailpoet/tests/integration/Statistics/Track/OpensTest.php +++ b/mailpoet/tests/integration/Statistics/Track/OpensTest.php @@ -366,7 +366,12 @@ class OpensTest extends \MailPoetTest { $opens = Stub::construct($this->opens, [ $this->diContainer->get(StatisticsOpensRepository::class), $this->diContainer->get(UserAgentsRepository::class), - new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock), + $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ), ], [ 'returnResponse' => null, ], $this); @@ -390,7 +395,12 @@ class OpensTest extends \MailPoetTest { $opens = Stub::construct($this->opens, [ $this->diContainer->get(StatisticsOpensRepository::class), $this->diContainer->get(UserAgentsRepository::class), - new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock), + $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ), ], [ 'returnResponse' => null, ], $this); @@ -414,7 +424,12 @@ class OpensTest extends \MailPoetTest { $opens = Stub::construct($this->opens, [ $this->diContainer->get(StatisticsOpensRepository::class), $this->diContainer->get(UserAgentsRepository::class), - new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($wpMock), $wpMock), + $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($wpMock), + 'wp' => $wpMock, + ] + ), ], [ 'returnResponse' => null, ], $this); diff --git a/mailpoet/tests/integration/Subscribers/SubscribersRepositoryTest.php b/mailpoet/tests/integration/Subscribers/SubscribersRepositoryTest.php index fdd8c9f05a..612d26832d 100644 --- a/mailpoet/tests/integration/Subscribers/SubscribersRepositoryTest.php +++ b/mailpoet/tests/integration/Subscribers/SubscribersRepositoryTest.php @@ -398,6 +398,23 @@ class SubscribersRepositoryTest extends \MailPoetTest { verify($this->repository->getMaxSubscriberId())->equals($subscriberTwo->getId()); } + public function testRemoveOrphanedSubscribersFromWpSegment(): void { + $wpUserId1 = $this->tester->createWordPressUser('subscriber1@email.com', 'author'); + $wpUserId2 = $this->tester->createWordPressUser('subscriber2@email.com', 'author'); + $wpUserId3 = $this->tester->createWordPressUser('subscriber3@email.com', 'author'); + $subscriber2 = $this->repository->findOneBy(['wpUserId' => $wpUserId2]); + $subscriber3 = $this->repository->findOneBy(['wpUserId' => $wpUserId3]); + + $this->tester->deleteWPUserFromDatabase($wpUserId1); + + $this->repository->removeOrphanedSubscribersFromWpSegment(); + + $subscribers = $this->repository->findAll(); + $this->assertCount(2, $subscribers); + $this->assertSame($subscribers[0], $subscriber2); + $this->assertSame($subscribers[1], $subscriber3); + } + private function createSubscriber(string $email, ?DateTimeImmutable $deletedAt = null): SubscriberEntity { $subscriber = new SubscriberEntity(); $subscriber->setEmail($email); diff --git a/mailpoet/tests/integration/WooCommerce/SubscriberEngagementTest.php b/mailpoet/tests/integration/WooCommerce/SubscriberEngagementTest.php index 1fed9c5978..305f250654 100644 --- a/mailpoet/tests/integration/WooCommerce/SubscriberEngagementTest.php +++ b/mailpoet/tests/integration/WooCommerce/SubscriberEngagementTest.php @@ -28,9 +28,15 @@ class SubscriberEngagementTest extends \MailPoetTest { public function _before() { $this->wooCommerceHelperMock = $this->createMock(Helper::class); $this->wpMock = $this->createMock(WPFunctions::class); + $subscribersRepository = $this->getServiceWithOverrides(SubscribersRepository::class, + [ + 'changesNotifier' => new SubscriberChangesNotifier($this->wpMock), + 'wp' => $this->wpMock, + ] + ); $this->subscriberEngagement = new SubscriberEngagement( $this->wooCommerceHelperMock, - new SubscribersRepository($this->entityManager, new SubscriberChangesNotifier($this->wpMock), $this->wpMock) + $subscribersRepository ); }