From d39d09dd72e19b680428ffe89a809cb31d81257e Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Thu, 2 Dec 2021 18:27:21 -0300 Subject: [PATCH] Remove calls to Subscriber::setError() from ConfirmationEmailMailer This commit is part of a task to replace Paris with Doctrine in the class ConfirmationEmailMailer. Specifically, it removes two calls to Subscriber::setError() inside ConfirmationEmailMailer::sendConfirmationEmail(). setError() was used to define an error message that was used only in one of the instances where sendConfirmationEmail() is called (API::subscribeToLists()). setError() was replaced with code that throws an exception when there is an error. Thus it was necessary to change all the places where ConfirmationEmailMailer::sendConfirmationEmail() is called to handle the exception. In some cases, there are some oddities as sendConfirmationEmail() can return false or throw an exception in case of an error and calling code must account for both. I decided to settle with this approach as refactoring the rest of this method to use exceptions instead of returning false seemed outside of the scope of this task. [MAILPOET-3815] --- lib/API/JSON/v1/Subscribers.php | 17 +++++--- lib/API/MP/v1/API.php | 10 +++-- lib/Segments/WP.php | 6 ++- lib/Subscribers/ConfirmationEmailMailer.php | 41 ++++++++++--------- lib/Subscribers/SubscriberActions.php | 7 +++- lib/WooCommerce/Subscription.php | 6 ++- tests/integration/API/MP/APITest.php | 5 +-- .../ConfirmationEmailMailerTest.php | 24 ++++++++++- 8 files changed, 79 insertions(+), 37 deletions(-) diff --git a/lib/API/JSON/v1/Subscribers.php b/lib/API/JSON/v1/Subscribers.php index e093070447..096bf20fdc 100644 --- a/lib/API/JSON/v1/Subscribers.php +++ b/lib/API/JSON/v1/Subscribers.php @@ -199,12 +199,19 @@ class Subscribers extends APIEndpoint { $id = (isset($data['id']) ? (int)$data['id'] : false); $subscriber = $this->subscribersRepository->findOneById($id); if ($subscriber instanceof SubscriberEntity) { - if ($this->confirmationEmailMailer->sendConfirmationEmail($subscriber)) { - return $this->successResponse(); + try { + if ($this->confirmationEmailMailer->sendConfirmationEmail($subscriber)) { + return $this->successResponse(); + } else { + return $this->errorResponse([ + APIError::UNKNOWN => __('There was a problem with your sending method. Please check if your sending method is properly configured.', 'mailpoet'), + ]); + } + } catch (\Exception $e) { + return $this->errorResponse([ + APIError::UNKNOWN => __('There was a problem with your sending method. Please check if your sending method is properly configured.', 'mailpoet'), + ]); } - return $this->errorResponse([ - APIError::UNKNOWN => __('There was a problem with your sending method. Please check if your sending method is properly configured.', 'mailpoet'), - ]); } else { return $this->errorResponse([ APIError::NOT_FOUND => WPFunctions::get()->__('This subscriber does not exist.', 'mailpoet'), diff --git a/lib/API/MP/v1/API.php b/lib/API/MP/v1/API.php index 602a1e9da4..4a4f4d6442 100644 --- a/lib/API/MP/v1/API.php +++ b/lib/API/MP/v1/API.php @@ -148,11 +148,13 @@ class API { // send confirmation email if ($sendConfirmationEmail) { - $result = $this->_sendConfirmationEmail($subscriber); - if (!$result && $subscriber->getErrors()) { + try { + $this->_sendConfirmationEmail($subscriber); + } catch (\Exception $e) { throw new APIException( - __(sprintf('Subscriber added to lists, but confirmation email failed to send: %s', strtolower(implode(', ', $subscriber->getErrors()))), 'mailpoet'), - APIException::CONFIRMATION_FAILED_TO_SEND); + __(sprintf('Subscriber added to lists, but confirmation email failed to send: %s', strtolower($e->getMessage())), 'mailpoet'), + APIException::CONFIRMATION_FAILED_TO_SEND + ); } } diff --git a/lib/Segments/WP.php b/lib/Segments/WP.php index 82b755089a..7c43edc181 100644 --- a/lib/Segments/WP.php +++ b/lib/Segments/WP.php @@ -151,7 +151,11 @@ class WP { $confirmationEmailMailer = ContainerWrapper::getInstance()->get(ConfirmationEmailMailer::class); $subscriberEntity = $this->subscribersRepository->findOneById($subscriber->id); if ($subscriberEntity instanceof SubscriberEntity) { - $confirmationEmailMailer->sendConfirmationEmailOnce($subscriberEntity); + try { + $confirmationEmailMailer->sendConfirmationEmailOnce($subscriberEntity); + } catch (\Exception $e) { + // ignore errors + } } } diff --git a/lib/Subscribers/ConfirmationEmailMailer.php b/lib/Subscribers/ConfirmationEmailMailer.php index ae43c5f864..41090b542f 100644 --- a/lib/Subscribers/ConfirmationEmailMailer.php +++ b/lib/Subscribers/ConfirmationEmailMailer.php @@ -7,7 +7,6 @@ use MailPoet\Entities\SegmentEntity; use MailPoet\Entities\SubscriberEntity; use MailPoet\Mailer\Mailer; use MailPoet\Mailer\MetaInfo; -use MailPoet\Models\Subscriber; use MailPoet\Services\AuthorizedEmailsController; use MailPoet\Services\Bridge; use MailPoet\Settings\SettingsController; @@ -59,6 +58,7 @@ class ConfirmationEmailMailer { * Use this method if you want to make sure the confirmation email * is not sent multiple times within a single request * e.g. if sending confirmation emails from hooks + * @throws \Exception if unable to send the email. */ public function sendConfirmationEmailOnce(SubscriberEntity $subscriber): bool { if (isset($this->sentEmails[$subscriber->getId()])) { @@ -67,6 +67,9 @@ class ConfirmationEmailMailer { return $this->sendConfirmationEmail($subscriber); } + /** + * @throws \Exception if unable to send the email. + */ public function sendConfirmationEmail(SubscriberEntity $subscriber) { $signupConfirmation = $this->settings->get('signup_confirmation'); if ((bool)$signupConfirmation['enabled'] === false) { @@ -119,29 +122,27 @@ class ConfirmationEmailMailer { ], ]; - $subscriberModel = Subscriber::findOne($subscriber->getId()); - // send email + $extraParams = [ + 'meta' => $this->mailerMetaInfo->getConfirmationMetaInfo($subscriber), + ]; try { - $extraParams = [ - 'meta' => $this->mailerMetaInfo->getConfirmationMetaInfo($subscriber), - ]; $result = $this->mailer->send($email, $subscriber, $extraParams); - if ($result['response'] === false) { - $subscriberModel->setError(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); - return false; - }; - - if (!$this->wp->isUserLoggedIn()) { - $subscriber->setConfirmationsCount($subscriber->getConfirmationsCount() + 1); - $this->subscribersRepository->persist($subscriber); - $this->subscribersRepository->flush(); - } - $this->sentEmails[$subscriber->getId()] = true; - return true; } catch (\Exception $e) { - $subscriberModel->setError(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); - return false; + throw new \Exception(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); } + + if ($result['response'] === false) { + throw new \Exception(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); + }; + + if (!$this->wp->isUserLoggedIn()) { + $subscriber->setConfirmationsCount($subscriber->getConfirmationsCount() + 1); + $this->subscribersRepository->persist($subscriber); + $this->subscribersRepository->flush(); + } + $this->sentEmails[$subscriber->getId()] = true; + + return true; } } diff --git a/lib/Subscribers/SubscriberActions.php b/lib/Subscribers/SubscriberActions.php index 7284cd6d6e..8c8c18ca5c 100644 --- a/lib/Subscribers/SubscriberActions.php +++ b/lib/Subscribers/SubscriberActions.php @@ -103,7 +103,12 @@ class SubscriberActions { // link subscriber to segments $segments = $this->segmentsRepository->findBy(['id' => $segmentIds]); $this->subscriberSegmentRepository->subscribeToSegments($subscriber, $segments); - $this->confirmationEmailMailer->sendConfirmationEmailOnce($subscriber); + + try { + $this->confirmationEmailMailer->sendConfirmationEmailOnce($subscriber); + } catch (\Exception $e) { + // ignore errors + } $subscriberModel = Subscriber::findOne($subscriber->getId()); diff --git a/lib/WooCommerce/Subscription.php b/lib/WooCommerce/Subscription.php index 6e5eccaf2e..ec0757cf11 100644 --- a/lib/WooCommerce/Subscription.php +++ b/lib/WooCommerce/Subscription.php @@ -225,7 +225,11 @@ class Subscription { $this->subscribersRepository->persist($subscriberEntity); $this->subscribersRepository->flush(); - $this->confirmationEmailMailer->sendConfirmationEmailOnce($subscriberEntity); + try { + $this->confirmationEmailMailer->sendConfirmationEmailOnce($subscriberEntity); + } catch (\Exception $e) { + // ignore errors + } } } diff --git a/tests/integration/API/MP/APITest.php b/tests/integration/API/MP/APITest.php index c00d11ebdf..14b5b12b4a 100644 --- a/tests/integration/API/MP/APITest.php +++ b/tests/integration/API/MP/APITest.php @@ -576,9 +576,8 @@ class APITest extends \MailPoetTest { $confirmationMailer = $this->createMock(ConfirmationEmailMailer::class); $confirmationMailer->expects($this->once()) ->method('sendConfirmationEmailOnce') - ->willReturnCallback(function (Subscriber $subscriber) { - $subscriber->setError('Big Error'); - return false; + ->willReturnCallback(function () { + throw new \Exception('Big Error'); }); $API = Stub::copy($this->getApi(), [ diff --git a/tests/integration/Subscribers/ConfirmationEmailMailerTest.php b/tests/integration/Subscribers/ConfirmationEmailMailerTest.php index b7358e0de0..06ee329a77 100644 --- a/tests/integration/Subscribers/ConfirmationEmailMailerTest.php +++ b/tests/integration/Subscribers/ConfirmationEmailMailerTest.php @@ -95,7 +95,7 @@ class ConfirmationEmailMailerTest extends \MailPoetTest { expect($this->subscriber->getConfirmationsCount())->equals(1); } - public function testItReturnsFalseWhenConfirmationEmailCannotBeSent() { + public function testItThrowsExceptionWhenConfirmationEmailCannotBeSent() { $mailer = Stub::makeEmpty(Mailer::class, [ 'send' => Stub\Expected::once(function () { @@ -111,7 +111,27 @@ class ConfirmationEmailMailerTest extends \MailPoetTest { $this->diContainer->get(SubscriptionUrlFactory::class) ); - $this->assertFalse($sender->sendConfirmationEmail($this->subscriber)); + $this->expectException(\Exception::class); + $this->expectExceptionMessage(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); + $sender->sendConfirmationEmail($this->subscriber); + } + + public function testSendConfirmationEmailThrowsWhenSendReturnsFalse() { + $mailer = Stub::makeEmpty(Mailer::class, [ + 'send' => ['response' => false], + ], $this); + + $sender = new ConfirmationEmailMailer( + $mailer, + $this->diContainer->get(WPFunctions::class), + $this->diContainer->get(SettingsController::class), + $this->diContainer->get(SubscribersRepository::class), + $this->diContainer->get(SubscriptionUrlFactory::class) + ); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage(__('Something went wrong with your subscription. Please contact the website owner.', 'mailpoet')); + $sender->sendConfirmationEmail($this->subscriber); } public function testItDoesntSendWhenMSSIsActiveAndConfirmationEmailIsNotAuthorized() {