diff --git a/mailpoet/lib/API/JSON/v1/Subscribers.php b/mailpoet/lib/API/JSON/v1/Subscribers.php index f298c4e71f..57512392dd 100644 --- a/mailpoet/lib/API/JSON/v1/Subscribers.php +++ b/mailpoet/lib/API/JSON/v1/Subscribers.php @@ -4,9 +4,12 @@ namespace MailPoet\API\JSON\v1; use MailPoet\API\JSON\Endpoint as APIEndpoint; use MailPoet\API\JSON\Error as APIError; +use MailPoet\API\JSON\ErrorResponse; use MailPoet\API\JSON\Response; use MailPoet\API\JSON\ResponseBuilders\SubscribersResponseBuilder; +use MailPoet\API\JSON\SuccessResponse; use MailPoet\Config\AccessControl; +use MailPoet\ConflictException; use MailPoet\Doctrine\Validator\ValidationException; use MailPoet\Entities\SegmentEntity; use MailPoet\Entities\SubscriberEntity; @@ -146,13 +149,24 @@ class Subscribers extends APIEndpoint { ); } - public function save($data = []) { + /** + * @param array $data + * @return ErrorResponse|SuccessResponse + */ + public function save(array $data = []) { try { $subscriber = $this->saveController->save($data); } catch (ValidationException $validationException) { return $this->badRequest([$this->getErrorMessage($validationException)]); + } catch (ConflictException $conflictException) { + return $this->badRequest([ + APIError::BAD_REQUEST => $conflictException->getMessage(), + ]); + } catch (\Exception $unknownException) { + return $this->badRequest([ + APIError::UNKNOWN => __('Saving subscriber failed.', 'mailpoet'), + ]); } - return $this->successResponse( $this->subscribersResponseBuilder->build($subscriber) ); diff --git a/mailpoet/lib/Subscribers/SubscriberSaveController.php b/mailpoet/lib/Subscribers/SubscriberSaveController.php index c8a51fcf11..53345c2155 100644 --- a/mailpoet/lib/Subscribers/SubscriberSaveController.php +++ b/mailpoet/lib/Subscribers/SubscriberSaveController.php @@ -2,7 +2,9 @@ namespace MailPoet\Subscribers; +use MailPoet\ConflictException; use MailPoet\CustomFields\CustomFieldsRepository; +use MailPoet\Doctrine\Validator\ValidationException; use MailPoet\Entities\StatisticsUnsubscribeEntity; use MailPoet\Entities\SubscriberEntity; use MailPoet\Entities\SubscriberSegmentEntity; @@ -69,8 +71,33 @@ class SubscriberSaveController { $this->wp = $wp; } + public function filterOutReservedColumns(array $subscriberData): array { + $reservedColumns = [ + 'id', + 'wp_user_id', + 'is_woocommerce_user', + 'status', + 'subscribed_ip', + 'confirmed_ip', + 'confirmed_at', + 'created_at', + 'updated_at', + 'deleted_at', + 'unconfirmed_data', + ]; + return array_diff_key( + $subscriberData, + array_flip($reservedColumns) + ); + } + + /** + * @throws ConflictException + * @throws ValidationException + * @throws \Exception + */ public function save(array $data): SubscriberEntity { - if (is_array($data) && !empty($data)) { + if (!empty($data)) { $data = $this->wp->stripslashesDeep($data); } @@ -97,6 +124,10 @@ class SubscriberSaveController { ); } + if (isset($data['email']) && $this->isNewEmail($data['email'], $oldSubscriber)) { + $this->verifyEmailIsUnique($data['email']); + } + $subscriber = $this->createOrUpdate($data, $oldSubscriber); $this->updateCustomFields($data, $subscriber); @@ -120,26 +151,6 @@ class SubscriberSaveController { return $subscriber; } - public function filterOutReservedColumns(array $subscriberData): array { - $reservedColumns = [ - 'id', - 'wp_user_id', - 'is_woocommerce_user', - 'status', - 'subscribed_ip', - 'confirmed_ip', - 'confirmed_at', - 'created_at', - 'updated_at', - 'deleted_at', - 'unconfirmed_data', - ]; - return array_diff_key( - $subscriberData, - array_flip($reservedColumns) - ); - } - private function getNonDefaultSubscribedSegments(array $data): array { if (!isset($data['id']) || (int)$data['id'] <= 0) { return []; @@ -175,6 +186,9 @@ class SubscriberSaveController { return array_diff($data['segments'], $oldSegmentIds); } + /** + * @throws ValidationException + */ public function createOrUpdate(array $data, ?SubscriberEntity $subscriber): SubscriberEntity { if (!$subscriber) { $subscriber = $this->createSubscriber(); @@ -204,6 +218,22 @@ class SubscriberSaveController { return $subscriber; } + private function isNewEmail(string $email, ?SubscriberEntity $subscriber): bool { + if ($subscriber && ($subscriber->getEmail() === $email)) return false; + return true; + } + + /** + * @throws ConflictException + */ + private function verifyEmailIsUnique(string $email): void { + $existingSubscriber = $this->subscribersRepository->findOneBy(['email' => $email]); + if ($existingSubscriber) { + $exceptionMessage = __(sprintf("A subscriber with E-mail '%s' already exists.", $email), 'mailpoet'); + throw new ConflictException($exceptionMessage); + } + } + private function createSubscriber(): SubscriberEntity { $subscriber = new SubscriberEntity(); $subscriber->setUnsubscribeToken($this->security->generateUnsubscribeTokenByEntity($subscriber)); diff --git a/mailpoet/tests/integration/API/JSON/v1/SubscribersTest.php b/mailpoet/tests/integration/API/JSON/v1/SubscribersTest.php index 4408c22317..ed8844adac 100644 --- a/mailpoet/tests/integration/API/JSON/v1/SubscribersTest.php +++ b/mailpoet/tests/integration/API/JSON/v1/SubscribersTest.php @@ -5,6 +5,7 @@ namespace MailPoet\Test\API\JSON\v1; use Codeception\Util\Fixtures; use MailPoet\API\JSON\Error; use MailPoet\API\JSON\ErrorResponse; +use MailPoet\API\JSON\SuccessResponse; use MailPoet\API\JSON\Response as APIResponse; use MailPoet\API\JSON\ResponseBuilders\SubscribersResponseBuilder; use MailPoet\API\JSON\v1\Subscribers; @@ -193,6 +194,7 @@ class SubscribersTest extends \MailPoetTest { $response = $this->endpoint->save($validData); expect($response->status)->equals(APIResponse::STATUS_OK); + $this->assertInstanceOf(SuccessResponse::class, $response); $this->entityManager->clear(); $subscriberRepository = $this->diContainer->get(SubscribersRepository::class); $subscriber = $subscriberRepository->findOneBy(['email' => 'raul.doe@mailpoet.com']); @@ -209,6 +211,8 @@ class SubscribersTest extends \MailPoetTest { $this->entityManager->clear(); $response = $this->endpoint->save(/* missing data */); expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST); + $this->assertInstanceOf(ErrorResponse::class, $response); + expect($response->errors[0]['message']) ->equals('Please enter your email address'); @@ -219,6 +223,7 @@ class SubscribersTest extends \MailPoetTest { $this->entityManager->clear(); $response = $this->endpoint->save($invalidData); expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST); + $this->assertInstanceOf(ErrorResponse::class, $response); expect($response->errors[0]['message']) ->equals('Your email address is invalid!'); expect($subscriber->getSource())->equals('administrator'); @@ -239,6 +244,7 @@ class SubscribersTest extends \MailPoetTest { ]; $response = $this->endpoint->save($validData); + $this->assertInstanceOf(SuccessResponse::class, $response); expect($response->status)->equals(APIResponse::STATUS_OK); $this->entityManager->clear(); $subscriberRepository = $this->diContainer->get(SubscribersRepository::class); @@ -261,6 +267,7 @@ class SubscribersTest extends \MailPoetTest { ]; $response = $this->endpoint->save($subscriberData); + $this->assertInstanceOf(SuccessResponse::class, $response); expect($response->status)->equals(APIResponse::STATUS_OK); expect($response->data)->equals( $this->responseBuilder->build($this->subscriber2) @@ -269,6 +276,28 @@ class SubscribersTest extends \MailPoetTest { expect($response->data['source'])->equals('api'); } + public function testItCanUpdateEmailOfAnExistingSubscriber(){ + $subscriberData = $this->responseBuilder->build($this->subscriber2); + $subscriberData['email'] = 'newjane@mailpoet.com'; + $response = $this->endpoint->save($subscriberData); + $this->assertInstanceOf(SuccessResponse::class, $response); + expect($response->status)->equals(APIResponse::STATUS_OK); + expect($response->data)->equals( + $this->responseBuilder->build($this->subscriber2) + ); + expect($response->data['email'])->equals('newjane@mailpoet.com'); + expect($response->data['first_name'])->equals($subscriberData['first_name']); + } + + public function testItCannotUpdateEmailOfAnExistingSubscriberIfEmailIsNotUnique(){ + $subscriberData = $this->responseBuilder->build($this->subscriber2); + $subscriberData['email'] = $this->subscriber1->getEmail(); + $response = $this->endpoint->save($subscriberData); + $this->assertInstanceOf(ErrorResponse::class, $response); + expect($response->status)->equals(APIResponse::STATUS_BAD_REQUEST); + expect($response->errors[0]['message'])->equals("A subscriber with E-mail '{$this->subscriber1->getEmail()}' already exists."); + } + public function testItCanRemoveListsFromAnExistingSubscriber() { $subscriberData = [ 'email' => 'jane@mailpoet.com', @@ -279,6 +308,7 @@ class SubscribersTest extends \MailPoetTest { ]; $response = $this->endpoint->save($subscriberData); + $this->assertInstanceOf(SuccessResponse::class, $response); expect($response->status)->equals(APIResponse::STATUS_OK); expect($response->data)->equals( $this->responseBuilder->build($this->subscriber2) diff --git a/mailpoet/tests/integration/Subscribers/SubscriberSaveControllerTest.php b/mailpoet/tests/integration/Subscribers/SubscriberSaveControllerTest.php index 537399c0ed..7a04cf0366 100644 --- a/mailpoet/tests/integration/Subscribers/SubscriberSaveControllerTest.php +++ b/mailpoet/tests/integration/Subscribers/SubscriberSaveControllerTest.php @@ -2,6 +2,7 @@ namespace MailPoet\Subscribers; +use MailPoet\ConflictException; use MailPoet\Entities\SegmentEntity; use MailPoet\Entities\SubscriberEntity; use MailPoet\Entities\SubscriberSegmentEntity; @@ -87,6 +88,22 @@ class SubscriberSaveControllerTest extends \MailPoetTest { expect($subscriber->getSubscriberSegments())->count(1); } + public function testItThrowsExceptionWhenUpdatingSubscriberEmailIfNotUnique(): void { + $subscriber = $this->createSubscriber('second@test.com', SubscriberEntity::STATUS_UNCONFIRMED); + $subscriber2 = $this->createSubscriber('third@test.com', SubscriberEntity::STATUS_UNCONFIRMED); + + $data = [ + 'id' => $subscriber->getId(), + 'email' => $subscriber2->getEmail(), + ]; + + $this->entityManager->clear(); + $this->expectException(ConflictException::class); + $this->expectExceptionMessage("A subscriber with E-mail '{$subscriber2->getEmail()}' already exists."); + + $this->saveController->save($data); + } + public function testItDeletesOrphanSubscriberSegmentsOnUpdate(): void { $subscriber = $this->createSubscriber('second@test.com', SubscriberEntity::STATUS_UNCONFIRMED); $segmentOne = $this->segmentsRepository->createOrUpdate('Segment One');