Refactor duplicate segment name checks

This creates a single method we can rely on to check for the uniqueness
of a segment name and refactors places where we had duplicate code to
use the new method.

[MAILPOET-3998]

lib/API/JSON/v1/DynamicSegments.php

@@ -2,12 +2,12 @@
 
 namespace MailPoet\API\JSON\v1;
 
-use InvalidArgumentException;
 use MailPoet\API\JSON\Endpoint as APIEndpoint;
 use MailPoet\API\JSON\Error;
 use MailPoet\API\JSON\Response;
 use MailPoet\API\JSON\ResponseBuilders\DynamicSegmentsResponseBuilder;
 use MailPoet\Config\AccessControl;
+use MailPoet\ConflictException;
 use MailPoet\Doctrine\Validator\ValidationException;
 use MailPoet\Entities\SegmentEntity;
 use MailPoet\Listing\Handler;
@@ -112,7 +112,7 @@ class DynamicSegments extends APIEndpoint {
       return $this->errorResponse([
         Error::BAD_REQUEST => $this->getErrorString($e),
       ], [], Response::STATUS_BAD_REQUEST);
-    } catch (InvalidArgumentException $e) {
+    } catch (ConflictException $e) {
       return $this->badRequest([
         Error::BAD_REQUEST => __('Another record already exists. Please specify a different "name".', 'mailpoet'),
       ]);

lib/API/JSON/v1/Segments.php

@@ -3,12 +3,12 @@
 namespace MailPoet\API\JSON\v1;
 
 use Exception;
-use InvalidArgumentException;
 use MailPoet\API\JSON\Endpoint as APIEndpoint;
 use MailPoet\API\JSON\Error as APIError;
 use MailPoet\API\JSON\Response;
 use MailPoet\API\JSON\ResponseBuilders\SegmentsResponseBuilder;
 use MailPoet\Config\AccessControl;
+use MailPoet\ConflictException;
 use MailPoet\Cron\CronWorkerScheduler;
 use MailPoet\Cron\Workers\WooCommerceSync;
 use MailPoet\Doctrine\Validator\ValidationException;
@@ -125,7 +125,7 @@ class Segments extends APIEndpoint {
       return $this->badRequest([
         APIError::BAD_REQUEST  => __('Please specify a name.', 'mailpoet'),
       ]);
-    } catch (InvalidArgumentException $exception) {
+    } catch (ConflictException $exception) {
       return $this->badRequest([
         APIError::BAD_REQUEST  => __('Another record already exists. Please specify a different "name".', 'mailpoet'),
       ]);

lib/Segments/DynamicSegments/SegmentSaveController.php

@@ -2,9 +2,11 @@
 
 namespace MailPoet\Segments\DynamicSegments;
 
-use InvalidArgumentException;
+use MailPoet\ConflictException;
 use MailPoet\Entities\SegmentEntity;
+use MailPoet\NotFoundException;
 use MailPoet\Segments\SegmentsRepository;
+use MailPoetVendor\Doctrine\ORM\ORMException;
 
 class SegmentSaveController {
   /** @var SegmentsRepository */
@@ -21,20 +23,18 @@ class SegmentSaveController {
     $this->filterDataMapper = $filterDataMapper;
   }
 
+  /**
+   * @throws ConflictException
+   * @throws NotFoundException
+   * @throws Exceptions\InvalidFilterException
+   * @throws ORMException
+   */
   public function save(array $data = []): SegmentEntity {
     $id = isset($data['id']) ? (int)$data['id'] : null;
     $name = $data['name'] ?? '';
     $description = $data['description'] ?? '';
     $filtersData = $this->filterDataMapper->map($data);
 
-    $this->checkSegmentUniqueName($name, $id);
-
     return $this->segmentsRepository->createOrUpdate($name, $description, SegmentEntity::TYPE_DYNAMIC, $filtersData, $id);
   }
-
-  private function checkSegmentUniqueName(string $name, ?int $id): void {
-    if (!$this->segmentsRepository->isNameUnique($name, $id)) {
-      throw new InvalidArgumentException("Segment with name: '{$name}' already exists.");
-    }
-  }
 }

lib/Segments/SegmentSaveController.php

@@ -2,10 +2,12 @@
 
 namespace MailPoet\Segments;
 
-use InvalidArgumentException;
+use MailPoet\ConflictException;
 use MailPoet\Entities\SegmentEntity;
 use MailPoet\Entities\SubscriberSegmentEntity;
+use MailPoet\NotFoundException;
 use MailPoetVendor\Doctrine\ORM\EntityManager;
+use MailPoetVendor\Doctrine\ORM\ORMException;
 
 class SegmentSaveController {
   /** @var SegmentsRepository */
@@ -22,21 +24,27 @@ class SegmentSaveController {
     $this->entityManager = $entityManager;
   }
 
+  /**
+   * @throws ConflictException
+   * @throws NotFoundException
+   * @throws ORMException
+   */
   public function save(array $data = []): SegmentEntity {
     $id = isset($data['id']) ? (int)$data['id'] : null;
     $name = $data['name'] ?? '';
     $description = $data['description'] ?? '';
 
-    $this->checkSegmentUniqueName($name, $id);
-
     return $this->segmentsRepository->createOrUpdate($name, $description, SegmentEntity::TYPE_DEFAULT, [], $id);
   }
 
+  /**
+   * @throws ConflictException
+   */
   public function duplicate(SegmentEntity $segmentEntity): SegmentEntity {
     $duplicate = clone $segmentEntity;
     $duplicate->setName(sprintf(__('Copy of %s', 'mailpoet'), $segmentEntity->getName()));
 
-    $this->checkSegmentUniqueName($duplicate->getName(), $duplicate->getId());
+    $this->segmentsRepository->verifyNameIsUnique($duplicate->getName(), $duplicate->getId());
 
     $this->entityManager->transactional(function (EntityManager $entityManager) use ($duplicate, $segmentEntity) {
       $entityManager->persist($duplicate);
@@ -58,10 +66,4 @@ class SegmentSaveController {
 
     return $duplicate;
   }
-
-  private function checkSegmentUniqueName(string $name, ?int $id): void {
-    if (!$this->segmentsRepository->isNameUnique($name, $id)) {
-      throw new InvalidArgumentException("Segment with name: '{$name}' already exists.");
-    }
-  }
 }

lib/Segments/SegmentsRepository.php

@@ -16,6 +16,7 @@ use MailPoet\WP\Functions as WPFunctions;
 use MailPoetVendor\Carbon\Carbon;
 use MailPoetVendor\Doctrine\DBAL\Connection;
 use MailPoetVendor\Doctrine\ORM\EntityManager;
+use MailPoetVendor\Doctrine\ORM\ORMException;
 
 /**
  * @extends Repository<SegmentEntity>
@@ -93,10 +94,20 @@ class SegmentsRepository extends Repository {
     return count($results) === 0;
   }
 
+  /**
+   * @throws ConflictException
+   */
+  public function verifyNameIsUnique(string $name, ?int $id): void {
+    if (!$this->isNameUnique($name, $id)) {
+      throw new ConflictException("Could not create new segment with name [{$name}] because a segment with that name already exists.");
+    }
+  }
+
   /**
    * @param DynamicSegmentFilterData[] $filtersData
    * @throws ConflictException
    * @throws NotFoundException
+   * @throws ORMException
    */
   public function createOrUpdate(
     string $name,
@@ -113,9 +124,7 @@ class SegmentsRepository extends Repository {
       $segment->setName($name);
       $segment->setDescription($description);
     } else {
-      if (!$this->isNameUnique($name, $id)) {
+      $this->verifyNameIsUnique($name, $id);
-        throw new ConflictException("Could not create new segment with name [{$name}] because a segment with that name already exists.");
-      }
       $segment = new SegmentEntity($name, $type, $description);
       $this->persist($segment);
     }

tests/integration/Segments/DynamicSegments/SegmentSaveControllerTest.php

@@ -2,6 +2,7 @@
 
 namespace MailPoet\Segments\DynamicSegments;
 
+use MailPoet\ConflictException;
 use MailPoet\Entities\DynamicSegmentFilterData;
 use MailPoet\Entities\DynamicSegmentFilterEntity;
 use MailPoet\Entities\SegmentEntity;
@@ -135,8 +136,8 @@ class SegmentSaveControllerTest extends \MailPoetTest {
         'action' => UserRole::TYPE,
       ]],
     ];
-    $this->expectException(\InvalidArgumentException::class);
+    $this->expectException(ConflictException::class);
-    $this->expectExceptionMessage("Segment with name: 'Test name' already exists.");
+    $this->expectExceptionMessage("Could not create new segment with name [Test name] because a segment with that name already exists.");
     $this->saveController->save($segmentData);
   }
 

tests/integration/Segments/SegmentSaveControllerTest.php

@@ -2,9 +2,11 @@
 
 namespace MailPoet\Segments;
 
+use MailPoet\ConflictException;
 use MailPoet\Entities\SegmentEntity;
 use MailPoet\Entities\SubscriberEntity;
 use MailPoet\Entities\SubscriberSegmentEntity;
+use MailPoet\Segments\DynamicSegments\Filters\UserRole;
 use MailPoet\Subscribers\SubscriberSegmentRepository;
 
 class SegmentSaveControllerTest extends \MailPoetTest {
@@ -52,14 +54,31 @@ class SegmentSaveControllerTest extends \MailPoetTest {
     expect($subscriberDuplicate1->getStatus())->equals($subscriberSegment1->getStatus());
     expect($subscriberDuplicate2->getStatus())->equals($subscriberSegment2->getStatus());
   }
-  
+
+  public function testItCheckDuplicateSegment() {
+    $name = 'Test name';
+    $this->createSegment($name);
+    $segmentData = [
+      'name' => $name,
+      'description' => 'Description',
+      'filters' => [[
+        'segmentType' => SegmentEntity::TYPE_DEFAULT,
+        'wordpressRole' => 'editor',
+        'action' => UserRole::TYPE,
+      ]],
+    ];
+    $this->expectException(ConflictException::class);
+    $this->expectExceptionMessage("Could not create new segment with name [Test name] because a segment with that name already exists.");
+    $this->saveController->save($segmentData);
+  }
+
   private function createSegment(string $name): SegmentEntity {
     $segment = new SegmentEntity($name, SegmentEntity::TYPE_DEFAULT, 'description');
     $this->entityManager->persist($segment);
     $this->entityManager->flush();
     return $segment;
   }
-  
+
   private function createSubscriber(string $email): SubscriberEntity {
     $subscriber = new SubscriberEntity();
     $subscriber->setStatus(SubscriberEntity::STATUS_SUBSCRIBED);

tests/integration/Segments/SegmentsRepositoryTest.php

@@ -108,6 +108,27 @@ class SegmentsRepositoryTest extends \MailPoetTest {
     expect($count)->equals(2);
   }
 
+  public function testItCanCheckForUniqueNames() {
+    $this->createDefaultSegment('Test');
+    $this->segmentsRepository->flush();
+    expect($this->segmentsRepository->isNameUnique('Test', null))->false();
+    expect($this->segmentsRepository->isNameUnique('Unique Name', null))->true();
+  }
+
+  public function testItCanForcefullyVerifyUniquenessOfName() {
+    $this->createDefaultSegment('Test');
+    $this->segmentsRepository->flush();
+    try {
+      $this->segmentsRepository->verifyNameIsUnique('Unique', null);
+      $this->addToAssertionCount(1);
+    } catch (ConflictException $exception) {
+      $this->fail();
+    }
+    $this->expectException(ConflictException::class);
+    $this->expectExceptionMessage('Could not create new segment with name [Test] because a segment with that name already exists.');
+    $this->segmentsRepository->verifyNameIsUnique('Test', null);
+  }
+
   public function testItChecksForDuplicateNameWhenCreatingNewSegment() {
     $this->createDefaultSegment('Existing Segment');
     $this->segmentsRepository->flush();