Refactor: id in update requests + template::update method

[TatevikGr]

Summary by CodeRabbit

• 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.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Thanks for contributing to phpList!

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'checks'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

Removed ID fields from several Update DTO constructors, added a new UpdateTemplateDto, refactored TemplateManager and SubscriberManager method signatures and flows to accept entities and DTOs directly, adjusted template image/validation handling, and updated unit tests to match the new DTO and manager APIs.

Changes

Cohort / File(s)	Change Summary
DTO ID Field Removals src/Domain/Identity/Model/Dto/UpdateAdministratorDto.php, src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php, src/Domain/Messaging/Model/Dto/UpdateMessageDto.php	Removed constructor-promoted administratorId, subscriberId, and messageId parameters respectively, shrinking public constructor contracts and eliminating ID initialization via those DTO constructors.
New Template DTO src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php	Added new final DTO with promoted readonly properties: ?string $title, ?string $content, ?string $text, ?string $fileContent, and boolean flags shouldCheckLinks, shouldCheckImages, shouldCheckExternalImages.
TemplateManager Refactor src/Domain/Messaging/Service/Manager/TemplateManager.php	Reworked public API and logic: update(Template, UpdateTemplateDto): Template replaces prior signature. Unified content assignment (fileContent vs content), conditional setters for title/text/content, consolidated validation using DTO flags, and explicit deletion/recreation of template images. Adjusted imports.
SubscriberManager Signature & Logic src/Domain/Subscription/Service/Manager/SubscriberManager.php	updateSubscriber(Subscriber $subscriber, UpdateSubscriberDto $dto, Administrator $admin): Subscriber now accepts an explicit Subscriber and no longer does ID lookup. Method updates additional fields, computes Doctrine change set, creates a ChangeSetDto, and logs history before returning the updated entity.
Tests Updated tests/Unit/Domain/Identity/Service/AdministratorManagerTest.php, tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php, tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php, tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php	Tests updated to omit ID params from DTO constructors (Administrator, Message), added UpdateTemplateDto import and a new testUpdateTemplateSuccessfully test, and changed SubscriberManagerTest to use a locally-scoped EntityManager mock instead of a class property.
Config .coderabbit.yaml	Disabled checks.docstring_coverage.enabled (set to false) in configuration.

Sequence Diagram(s)

sequenceDiagram
    actor Caller
    participant TemplateManager
    participant Template
    participant UpdateTemplateDto
    participant Validator
    participant ImageService

    Caller->>TemplateManager: update(Template, UpdateTemplateDto)
    rect rgb(230,245,255)
        TemplateManager->>UpdateTemplateDto: read fileContent / content
        TemplateManager->>Template: setContent() if provided
        TemplateManager->>Template: setTitle()/setText() if not null
    end
    rect rgb(245,230,255)
        TemplateManager->>Validator: run validations (links/images) using DTO flags
        Validator-->>TemplateManager: validation result
    end
    rect rgb(255,245,230)
        TemplateManager->>ImageService: delete existing images
        TemplateManager->>ImageService: recreate images from current URLs
    end
    TemplateManager->>Caller: return updated Template

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Areas to focus on:
  • TemplateManager::update — content selection (fileContent vs content), validation context flags, and image delete/recreate ordering.
  • SubscriberManager::updateSubscriber — new parameter ordering, Doctrine change-set extraction, ChangeSetDto construction, and history logging.
  • All call sites and tests that previously passed ID parameters to UpdateAdministratorDto, UpdateSubscriberDto, and UpdateMessageDto — ensure no remaining usages expect IDs.
  • New TemplateManagerTest — confirm mocks and assertions reflect the new image/validation flow.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main changes: removing IDs from DTOs (UpdateAdministratorDto, UpdateSubscriberDto, UpdateMessageDto) and refactoring the TemplateManager::update method signature.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/id-in-update-request

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3a141c and 451b1b6.

📒 Files selected for processing (1)

• .coderabbit.yaml (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• .coderabbit.yaml

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: phpList on PHP 8.1 [Build, Test]
• GitHub Check: phpList on PHP 8.1 [Build, Test]

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

107-123: Domain layer calling EntityManager::persist is a layering leak

In createFromImport, the Domain service directly depends on Doctrine’s EntityManagerInterface and calls $this->entityManager->persist($subscriber);. Per the Domain guidelines, Domain code should not perform persistence operations via Doctrine APIs for regular entities; that work should be expressed via abstractions (e.g., SubscriberRepository) or pushed to the application/infrastructure layer.

Even though this is pre‑existing and not introduced by this PR, it would be good to:

• Move the persist responsibility into SubscriberRepository (or another infra‑level service), and
• Let SubscriberManager only express intent (e.g., subscriberRepository->save($subscriber)).

Not something that has to be fixed in this PR, but worth scheduling as a follow‑up refactor to align with the layering rules.
As per coding guidelines

🧹 Nitpick comments (3)

src/Domain/Messaging/Service/Manager/TemplateManager.php (1)

61-93: Update flow is solid; consider clarifying null/clearing semantics & persistence expectations

The new update(Template $template, UpdateTemplateDto $updateTemplateDto) implementation looks clean:

• Optional fields (title, text, content/fileContent) are only applied when non‑null.
• Validation context reuses the same flags as create.
• Images are re‑extracted/validated from the current content and existing images are deleted before recreating.

Two things you may want to double‑check/clarify:

1. Null vs. clearing fields – With the current DTO, null means “don’t touch this field”, so there’s no way to explicitly clear text or content. If you ever need “set to null” semantics, you’ll need either explicit “hasX” flags or a different DTO shape.

2. Persistence responsibility – create() calls $this->templateRepository->persist($template), while update() just mutates the aggregate. If the intent is that the caller/UnitOfWork handles persistence for updates, it’s worth documenting that to avoid confusion, or mirroring the create behavior if updates are also meant to be persisted here.

Other than that, the flow and responsibilities in this manager stay nicely within the domain layer.

tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php (1)

7-9: Update test covers the main path; consider a couple of extra edge cases

The new testUpdateTemplateSuccessfully is a good fit for the refactored API: it checks that

• link validation runs with the updated content,
• images are extracted and validated, and
• the template’s title/text/content are updated as expected.

Two optional ideas if you want to tighten coverage further:

• Add a case where only some fields are provided in UpdateTemplateDto (e.g., just text) to lock in the “partial update” semantics.
• If Template::getImages() can ever be non‑empty, a test that asserts the delete path is used (e.g., expecting delete() calls on TemplateImageManager) would make the image‑replacement behavior explicit.

The existing test is already solid for the main flow though.

Also applies to: 41-79

src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php (1)

7-22: DTO shape looks appropriate for partial updates

The DTO is straightforward and lines up with the create DTO: nullable strings for fields that may or may not be updated, plus explicit boolean flags for validation behavior. The suppression for the boolean‑argument warning makes sense here.

If you find people tripping over the “null means no change” contract, a short class‑level docblock noting that behavior could help, but structurally this looks good.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c07ff3 and 4215c40.

📒 Files selected for processing (8)

• src/Domain/Identity/Model/Dto/UpdateAdministratorDto.php (0 hunks)
• src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php (1 hunks)
• src/Domain/Messaging/Service/Manager/TemplateManager.php (3 hunks)
• src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php (0 hunks)
• src/Domain/Subscription/Service/Manager/SubscriberManager.php (1 hunks)
• tests/Unit/Domain/Identity/Service/AdministratorManagerTest.php (0 hunks)
• tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php (2 hunks)
• tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php (1 hunks)

💤 Files with no reviewable changes (3)

• tests/Unit/Domain/Identity/Service/AdministratorManagerTest.php
• src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php
• src/Domain/Identity/Model/Dto/UpdateAdministratorDto.php

🧰 Additional context used

📓 Path-based instructions (1)

src/Domain/**

⚙️ CodeRabbit configuration file

src/Domain/**: You are reviewing PHP domain-layer code. Enforce domain purity, with a relaxed policy for DynamicListAttr:

• ❌ Do not allow persistence or transaction side effects here for normal domain models.

• Flag ANY usage of Doctrine persistence APIs on regular domain entities, especially:

  • $entityManager->flush(...), $this->entityManager->flush(...)
  • $em->persist(...), $em->remove(...)
  • $em->beginTransaction(), $em->commit(), $em->rollback()

• ✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
  as long as it does not modify state or perform writes.

• ✅ Relaxed rule for DynamicListAttr-related code:

  • DynamicListAttr is a special case dealing with dynamic tables/attrs.
  • It is acceptable for DynamicListAttr repositories/services to:
    • Create/update/drop DynamicListAttr tables/columns.
    • Use Doctrine persistence APIs (persist, remove, flush, etc.)
      as part of managing DynamicListAttr data and schema.
  • Do not flag persistence or schema-creation calls that are clearly scoped
    to DynamicListAttr tables or their management.
  • Still prefer keeping this logic well-encapsulated (e.g. in dedicated services/repos),
    not scattered across unrelated domain objects.

• ⚠️ For non-DynamicListAttr code:

  • If code is invoking actual table-creation, DDL execution, or schema synchronization,
    then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).
  • Repositories in Domain should be abstractions without side effects; they should express intent,
    not perform flush/transactional logic.

Files:

• src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php
• src/Domain/Subscription/Service/Manager/SubscriberManager.php
• src/Domain/Messaging/Service/Manager/TemplateManager.php

🧬 Code graph analysis (5)

src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php (1)

src/Domain/Messaging/Service/Manager/TemplateManager.php (1)

• __construct (22-32)

tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php (2)

src/Domain/Subscription/Service/SubscriberDeletionService.php (1)

• SubscriberDeletionService (18-77)

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

• SubscriberManager (19-143)

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

src/Domain/Subscription/Model/Dto/UpdateSubscriberDto.php (1)

• UpdateSubscriberDto (7-18)

src/Domain/Messaging/Service/Manager/TemplateManager.php (1)

src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php (1)

• UpdateTemplateDto (7-22)

tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php (2)

src/Domain/Messaging/Model/Dto/UpdateTemplateDto.php (1)

• UpdateTemplateDto (7-22)

src/Domain/Messaging/Service/Manager/TemplateManager.php (1)

• update (61-93)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: phpList on PHP 8.1 [Build, Test]

🔇 Additional comments (4)

src/Domain/Messaging/Service/Manager/TemplateManager.php (1)

34-42: Content selection in create looks consistent and safe

Using $createTemplateDto->fileContent ?? $createTemplateDto->content and only setting content when non‑null keeps the behavior clear and matches how the update flow prefers file content. Validators running on $template->getContent() ?? '' also align with that. No issues from my side here.

tests/Unit/Domain/Messaging/Service/Manager/TemplateManagerTest.php (1)

33-38: Named arguments for manager construction read nicely

Using named args for TemplateManager setup keeps the dependencies self‑documenting and makes future refactors less error‑prone. Looks good.

tests/Unit/Domain/Subscription/Service/Manager/SubscriberManagerTest.php (1)

23-35: Local EntityManager mock in setUp looks good

Using a local $entityManager mock here is perfectly fine and keeps the test fixture minimal; there’s no need to store it as a property since nothing else in the test uses it. Construction of SubscriberManager stays clear and explicit.

src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)

61-81: updateSubscriber is correctly designed for its domain layer contract

The reviewed method avoids any direct persistence calls and correctly defers flush responsibility to the caller. The assumption that $subscriber is a managed Doctrine entity is sound—the computeChangeSet() and getEntityChangeSet() pattern requires this, and it's a reasonable expectation given the method signature and design.

No domain purity concerns here. No call sites exist in the codebase yet, so outer-layer verification isn't possible at this stage—but the method itself is correctly structured: it mutates, it logs changes via the history manager, and it stops there.