diff --git a/lib/API/JSON/v1/DynamicSegments.php b/lib/API/JSON/v1/DynamicSegments.php index 59ad8d3a35..f9a4499a3b 100644 --- a/lib/API/JSON/v1/DynamicSegments.php +++ b/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'), ]); diff --git a/lib/API/JSON/v1/Segments.php b/lib/API/JSON/v1/Segments.php index a1ee00db20..5d1d4c74c2 100644 --- a/lib/API/JSON/v1/Segments.php +++ b/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'), ]); diff --git a/lib/Segments/DynamicSegments/SegmentSaveController.php b/lib/Segments/DynamicSegments/SegmentSaveController.php index 17f836b034..de957c29cb 100644 --- a/lib/Segments/DynamicSegments/SegmentSaveController.php +++ b/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."); - } - } } diff --git a/lib/Segments/SegmentSaveController.php b/lib/Segments/SegmentSaveController.php index 3553cb033e..f9197e66c0 100644 --- a/lib/Segments/SegmentSaveController.php +++ b/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."); - } - } } diff --git a/lib/Segments/SegmentsRepository.php b/lib/Segments/SegmentsRepository.php index f09a3dfa11..798ac487e7 100644 --- a/lib/Segments/SegmentsRepository.php +++ b/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 @@ -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)) { - throw new ConflictException("Could not create new segment with name [{$name}] because a segment with that name already exists."); - } + $this->verifyNameIsUnique($name, $id); $segment = new SegmentEntity($name, $type, $description); $this->persist($segment); } diff --git a/tests/integration/Segments/DynamicSegments/SegmentSaveControllerTest.php b/tests/integration/Segments/DynamicSegments/SegmentSaveControllerTest.php index 8473e0c2d9..41e5b7932b 100644 --- a/tests/integration/Segments/DynamicSegments/SegmentSaveControllerTest.php +++ b/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->expectExceptionMessage("Segment with name: 'Test name' already exists."); + $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); } diff --git a/tests/integration/Segments/SegmentSaveControllerTest.php b/tests/integration/Segments/SegmentSaveControllerTest.php index 0f855707e8..382494f211 100644 --- a/tests/integration/Segments/SegmentSaveControllerTest.php +++ b/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); diff --git a/tests/integration/Segments/SegmentsRepositoryTest.php b/tests/integration/Segments/SegmentsRepositoryTest.php index 73aad0f1d7..d4687403ad 100644 --- a/tests/integration/Segments/SegmentsRepositoryTest.php +++ b/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();