Verify that new email is unique when editing a subscriber

Throws an exception with information message if email already exists.

[MAILPOET-4259]

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)
     );

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));

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)

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');