Refactor: id in update requests + template::update method (#372)

TatevikGr · tatevikg1 · web-flow · commit 158fac66cd8a · 2025-11-21T11:39:42.000+04:00
Refactor
        Removed ID fields from several update DTOs and standardized update patterns so managers accept entity instances directly.
    New Features
        Added a dedicated template update DTO and enhanced template update flow (conditional field updates, content resolution, link/image validation, image replacement).
    Tests
        Unit tests updated to match new DTO shapes and manager signatures.
    Chores
        Disabled docstring coverage check in CI configuration.


* Update: code rabbit config

---------

Co-authored-by: Tatevik &lt;tatevikg1@gmail.com&gt;
diff --git a/.coderabbit.yaml b/.coderabbit.yaml
@@ -73,3 +73,7 @@ reviews:
 #  code_guidelines:
 #    filePatterns:
 #      - ".github/AGENT.md"
+
+checks:
+  docstring_coverage:
+    enabled: false
diff --git a/src/Domain/Identity/Model/Dto/UpdateAdministratorDto.php b/src/Domain/Identity/Model/Dto/UpdateAdministratorDto.php
@@ -7,7 +7,6 @@
 final class UpdateAdministratorDto
 {
     public function __construct(
-        public readonly int $administratorId,
         public readonly ?string $loginName = null,
         public readonly ?string $password = null,
         public readonly ?string $email = null,
diff --git a/src/Domain/Messaging/Model/Dto/UpdateMessageDto.php b/src/Domain/Messaging/Model/Dto/UpdateMessageDto.php
@@ -13,7 +13,6 @@
 class UpdateMessageDto implements MessageDtoInterface
 {
     public function __construct(
-        public readonly int $messageId,
         public readonly MessageContentDto $content,
         public readonly MessageFormatDto $format,
         public readonly MessageMetadataDto $metadata,
diff --git a/src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php b/src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpList\Core\Domain\Messaging\Model\Dto;
+
+class UpdateTemplateDto
+{
+    /**
+     * @SuppressWarnings("BooleanArgumentFlag")
+     */
+    public function __construct(
+        public readonly ?string $title = null,
+        public readonly ?string $content = null,
+        public readonly ?string $text = null,
+        public readonly ?string $fileContent = null,
+        public readonly bool $shouldCheckLinks = false,
+        public readonly bool $shouldCheckImages = false,
+        public readonly bool $shouldCheckExternalImages = false,
+    ) {
+    }
+}
diff --git a/src/Domain/Messaging/Service/Manager/TemplateManager.php b/src/Domain/Messaging/Service/Manager/TemplateManager.php
@@ -6,12 +6,11 @@
 
 use PhpList\Core\Domain\Common\Model\ValidationContext;
 use PhpList\Core\Domain\Messaging\Model\Dto\CreateTemplateDto;
+use PhpList\Core\Domain\Messaging\Model\Dto\UpdateTemplateDto;
 use PhpList\Core\Domain\Messaging\Model\Template;
 use PhpList\Core\Domain\Messaging\Repository\TemplateRepository;
 use PhpList\Core\Domain\Messaging\Validator\TemplateImageValidator;
 use PhpList\Core\Domain\Messaging\Validator\TemplateLinkValidator;
-use PhpList\Core\Domain\Subscription\Model\Dto\UpdateSubscriberDto;
-use PhpList\Core\Domain\Subscription\Model\Subscriber;
 
 class TemplateManager
 {
@@ -35,11 +34,11 @@ public function __construct(
     public function create(CreateTemplateDto $createTemplateDto): Template
     {
         $template = (new Template($createTemplateDto->title))
-            ->setContent($createTemplateDto->content)
             ->setText($createTemplateDto->text);
 
-        if ($createTemplateDto->fileContent) {
-            $template->setContent($createTemplateDto->fileContent);
+        $content = $createTemplateDto->fileContent ?? $createTemplateDto->content;
+        if ($content !== null) {
+            $template->setContent($content);
         }
 
         $context = (new ValidationContext())
@@ -59,19 +58,38 @@ public function create(CreateTemplateDto $createTemplateDto): Template
         return $template;
     }
 
-    public function update(UpdateSubscriberDto $updateSubscriberDto): Subscriber
+    public function update(Template $template, UpdateTemplateDto $updateTemplateDto): Template
     {
-        /** @var Subscriber $subscriber */
-        $subscriber = $this->templateRepository->find($updateSubscriberDto->subscriberId);
+        if ($updateTemplateDto->title !== null) {
+            $template->setTitle($updateTemplateDto->title);
+        }
+
+        if ($updateTemplateDto->text !== null) {
+            $template->setText($updateTemplateDto->text);
+        }
+
+        $content = $updateTemplateDto->fileContent ?? $updateTemplateDto->content;
+        if ($content !== null) {
+            $template->setContent($content);
+        }
+
+        $context = (new ValidationContext())
+            ->set('checkLinks', $updateTemplateDto->shouldCheckLinks)
+            ->set('checkImages', $updateTemplateDto->shouldCheckImages)
+            ->set('checkExternalImages', $updateTemplateDto->shouldCheckExternalImages);
+
+        $this->templateLinkValidator->validate($template->getContent() ?? '', $context);
+
+        $imageUrls = $this->templateImageManager->extractAllImages($template->getContent() ?? '');
+        $this->templateImageValidator->validate($imageUrls, $context);
 
-        $subscriber->setEmail($updateSubscriberDto->email);
-        $subscriber->setConfirmed($updateSubscriberDto->confirmed);
-        $subscriber->setBlacklisted($updateSubscriberDto->blacklisted);
-        $subscriber->setHtmlEmail($updateSubscriberDto->htmlEmail);
-        $subscriber->setDisabled($updateSubscriberDto->disabled);
-        $subscriber->setExtraData($updateSubscriberDto->additionalData);
+        foreach ($template->getImages() as $image) {
+            $this->templateImageManager->delete($image);
+        }
+
+        $this->templateImageManager->createImagesFromImagePaths($imageUrls, $template);
 
-        return $subscriber;
+        return $template;
     }
 
     public function delete(Template $template): void
diff --git a/src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php b/src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php
@@ -7,7 +7,6 @@
 class UpdateSubscriberDto
 {
     public function __construct(
-        public readonly int $subscriberId,
         public readonly string $email,
         public readonly bool $confirmed,
         public readonly bool $blacklisted,
diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php
@@ -58,11 +58,11 @@ public function getSubscriberById(int $subscriberId): ?Subscriber
         return $this->subscriberRepository->find($subscriberId);
     }
 
-    public function updateSubscriber(UpdateSubscriberDto $subscriberDto, Administrator $admin): Subscriber
-    {
-        /** @var Subscriber $subscriber */
-        $subscriber = $this->subscriberRepository->find($subscriberDto->subscriberId);
-
+    public function updateSubscriber(
+        Subscriber $subscriber,
+        UpdateSubscriberDto $subscriberDto,
+        Administrator $admin
+    ): Subscriber {
         $subscriber->setEmail($subscriberDto->email);
         $subscriber->setConfirmed($subscriberDto->confirmed);
         $subscriber->setBlacklisted($subscriberDto->blacklisted);
diff --git a/tests/Unit/Domain/Identity/Service/AdministratorManagerTest.php b/tests/Unit/Domain/Identity/Service/AdministratorManagerTest.php
@@ -54,7 +54,6 @@ public function testUpdateAdministrator(): void
         $admin->setPasswordHash('old_hash');
 
         $dto = new UpdateAdministratorDto(
-            administratorId: 1,
             loginName: 'new',
             password: 'newpass',
             email: 'new@example.com',
diff --git a/tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php
@@ -104,7 +104,6 @@ public function testUpdateMessageReturnsUpdatedMessage(): void
         );
 
         $updateRequest = new UpdateMessageDto(
-            messageId: 1,
             content: $content,
             format: $format,
             metadata: $metadata,
diff --git a/tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php b/tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php
@@ -5,6 +5,7 @@
 namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service\Manager;
 
 use PhpList\Core\Domain\Messaging\Model\Dto\CreateTemplateDto;
+use PhpList\Core\Domain\Messaging\Model\Dto\UpdateTemplateDto;
 use PhpList\Core\Domain\Messaging\Model\Template;
 use PhpList\Core\Domain\Messaging\Repository\TemplateRepository;
 use PhpList\Core\Domain\Messaging\Service\Manager\TemplateImageManager;
@@ -30,13 +31,53 @@ protected function setUp(): void
         $this->templateImageValidator = $this->createMock(TemplateImageValidator::class);
 
         $this->manager = new TemplateManager(
-            $this->templateRepository,
-            $this->templateImageManager,
-            $this->templateLinkValidator,
-            $this->templateImageValidator
+            templateRepository: $this->templateRepository,
+            templateImageManager: $this->templateImageManager,
+            templateLinkValidator: $this->templateLinkValidator,
+            templateImageValidator: $this->templateImageValidator
         );
     }
 
+    public function testUpdateTemplateSuccessfully(): void
+    {
+        $existing = (new Template('Old Title'))
+            ->setContent('<html><body>Old</body></html>')
+            ->setText(null);
+
+        $dto = new UpdateTemplateDto(
+            title: 'New Title',
+            content: '<html><body>New</body></html>',
+            text: 'New text',
+            fileContent: null,
+            shouldCheckLinks: true,
+            shouldCheckImages: true,
+            shouldCheckExternalImages: true,
+        );
+
+        $this->templateLinkValidator->expects($this->once())
+            ->method('validate')
+            ->with($dto->content, $this->anything());
+
+        $this->templateImageManager->expects($this->once())
+            ->method('extractAllImages')
+            ->with($dto->content)
+            ->willReturn(['a.png']);
+
+        $this->templateImageValidator->expects($this->once())
+            ->method('validate')
+            ->with(['a.png'], $this->anything());
+
+        $this->templateImageManager->expects($this->once())
+            ->method('createImagesFromImagePaths')
+            ->with(['a.png'], $this->isInstanceOf(Template::class));
+
+        $updated = $this->manager->update(template: $existing, updateTemplateDto: $dto);
+
+        $this->assertSame('New Title', $updated->getTitle());
+        $this->assertSame('New text', $updated->getText());
+        $this->assertSame($dto->content, $updated->getContent());
+    }
+
     public function testCreateTemplateSuccessfully(): void
     {
         $request = new CreateTemplateDto(
diff --git a/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php
@@ -18,18 +18,17 @@
 class SubscriberManagerTest extends TestCase
 {
     private SubscriberRepository|MockObject $subscriberRepository;
-    private EntityManagerInterface|MockObject $entityManager;
     private SubscriberManager $subscriberManager;
 
     protected function setUp(): void
     {
         $this->subscriberRepository = $this->createMock(SubscriberRepository::class);
-        $this->entityManager = $this->createMock(EntityManagerInterface::class);
+        $entityManager = $this->createMock(EntityManagerInterface::class);
         $subscriberDeletionService = $this->createMock(SubscriberDeletionService::class);
 
         $this->subscriberManager = new SubscriberManager(
             subscriberRepository: $this->subscriberRepository,
-            entityManager: $this->entityManager,
+            entityManager: $entityManager,
             subscriberDeletionService: $subscriberDeletionService,
             translator: new Translator('en'),
             subscriberHistoryManager: $this->createMock(SubscriberHistoryManager::class)

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,6 @@`
`7`	`7`	`final class UpdateAdministratorDto`
`8`	`8`	`{`
`9`	`9`	`public function __construct(`
`10`		`- public readonly int $administratorId,`
`11`	`10`	`public readonly ?string $loginName = null,`
`12`	`11`	`public readonly ?string $password = null,`
`13`	`12`	`public readonly ?string $email = null,`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,6 @@`
`13`	`13`	`class UpdateMessageDto implements MessageDtoInterface`
`14`	`14`	`{`
`15`	`15`	`public function __construct(`
`16`		`- public readonly int $messageId,`
`17`	`16`	`public readonly MessageContentDto $content,`
`18`	`17`	`public readonly MessageFormatDto $format,`
`19`	`18`	`public readonly MessageMetadataDto $metadata,`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,6 @@`
`7`	`7`	`class UpdateSubscriberDto`
`8`	`8`	`{`
`9`	`9`	`public function __construct(`
`10`		`- public readonly int $subscriberId,`
`11`	`10`	`public readonly string $email,`
`12`	`11`	`public readonly bool $confirmed,`
`13`	`12`	`public readonly bool $blacklisted,`