Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR applies constructor property promotion, fixes list relationship accessors, adds boolean casting for googleTrack, introduces MessageManager::copyAsDraftMessage with tests, updates MessageStatus transitions, adds PHPDoc generics for Message list relations, and inserts an explicit EntityManager->flush() during email sending. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php (1)
33-37: These test updates only satisfy constructor arity.The production delta is the new conditional finalization branch, but both constructors pass throwaway third-arg mocks, so the suite never proves that branch runs for link-creation cases and is skipped for no-op paths. If this dependency stays, keep that mock on the fixture and add those expectations.
Also applies to: 217-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php` around lines 33 - 37, The tests currently only satisfy constructor arity by passing a throwaway third-arg mock (EntityManagerInterface) into LinkTrackService but do not assert the new conditional finalization branch; keep the EntityManagerInterface mock as a fixture property and in the tests that exercise link-creation set expectations on its methods (e.g., expect persist() and flush() to be called once on the EntityManager mock when LinkTrackService triggers finalization), and in the tests that represent no-op paths assert those methods are never called; apply the same expectation changes to the other affected test cases (the ones around the 217-221 region) so the suite verifies both the branch executes for creations and is skipped for no-ops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domain/Analytics/Service/LinkTrackService.php`:
- Around line 16-20: The LinkTrackService currently injects
EntityManagerInterface and calls flush() (via the $newLinksPersisted flag);
remove EntityManagerInterface from the constructor signature, delete the
$newLinksPersisted flag and the conditional flush block (the flush() call), and
leave only repository persist() calls in methods where new LinkTrack entities
are scheduled; ensure constructor only accepts LinkTrackRepository and
ParameterProvider and do not perform any transaction finalization in
LinkTrackService.
In `@src/Domain/Messaging/Service/MessageDataLoader.php`:
- Around line 148-153: The test mock is outdated: update
MessageDataLoaderTest.php so the ListMessage mock defines getList() and returns
a SubscriberList mock (or stub) whose getId() returns the expected int instead
of the old getListId(); also add a defensive null-check in
MessageDataLoader::populateTargetLists to verify $list = $listMessage->getList()
and $id = $list?->getId() are not null before assigning
$messageData['targetlist'][$id] = 1 to avoid potential null dereferences.
---
Nitpick comments:
In `@tests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php`:
- Around line 33-37: The tests currently only satisfy constructor arity by
passing a throwaway third-arg mock (EntityManagerInterface) into
LinkTrackService but do not assert the new conditional finalization branch; keep
the EntityManagerInterface mock as a fixture property and in the tests that
exercise link-creation set expectations on its methods (e.g., expect persist()
and flush() to be called once on the EntityManager mock when LinkTrackService
triggers finalization), and in the tests that represent no-op paths assert those
methods are never called; apply the same expectation changes to the other
affected test cases (the ones around the 217-221 region) so the suite verifies
both the branch executes for creations and is skipped for no-ops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38b7cafc-c8e3-4baa-9609-4d9c74b242da
📒 Files selected for processing (4)
src/Domain/Analytics/Service/LinkTrackService.phpsrc/Domain/Messaging/Service/MessageDataLoader.phpsrc/Domain/Messaging/Service/MessagePrecacheService.phptests/Unit/Domain/Analytics/Service/LinkTrackServiceTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php (1)
29-65: Solid test for the happy path.Good structure - uses a real
Messageobject and verifies the clone is a distinct instance withDraftstatus and preserved format settings.One thing to consider: adding assertions that modifications to the cloned message's
$schedule,$content, or$optionsdon't affect the original would help catch shallow-clone regressions. This ties into the implementation concern flagged in the main code.Optional: Add shallow-clone isolation assertion
// After $result = $manager->copyAsDraftMessage($message); // Verify embedded objects are independent copies (if implementation is fixed) $this->assertNotSame($message->getSchedule(), $result->getSchedule()); $this->assertNotSame($message->getContent(), $result->getContent()); $this->assertNotSame($message->getOptions(), $result->getOptions());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php` around lines 29 - 65, The test should also assert that the cloned message is a deep copy for embedded objects to catch shallow-clone regressions: after calling MessageManager::copyAsDraftMessage($message) (method copyAsDraftMessage on class MessageManager), add assertions that $result->getSchedule(), $result->getContent(), and $result->getOptions() are not the same instances as the original $message->getSchedule(), $message->getContent(), and $message->getOptions() respectively; if those assertions fail the implementation must be updated to deep-clone those properties when creating the draft (inspect the cloning logic in MessageManager::copyAsDraftMessage and ensure it creates new MessageSchedule, MessageContent, and MessageOptions instances rather than copying references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domain/Messaging/Service/Manager/MessageManager.php`:
- Around line 32-44: The current copyAsDraftMessage in MessageManager uses clone
which performs a shallow copy and leaves embedded objects ($schedule, $content,
$options) and identity fields ($id, $uuid) shared with the original; update the
code to avoid cloning or implement a __clone() on Message that: deep-clones
embedded value objects ($schedule, $content, $options and any DateTime inside
them), re-generates a new $uuid, and clears $id (or ensures a new entity is
constructed so $id is null) before persisting; specifically modify
MessageManager::copyAsDraftMessage to either construct a fresh Message instance
copying scalar/immutable fields and constructing new MessageMetadata(status:
Draft) and Message\MessageFormat(...) or add Message::__clone to deep-copy the
embedded properties and reset identifiers so MessageRepository->persist will
INSERT rather than conflict on unique uuid or treat as detached entity.
---
Nitpick comments:
In `@tests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php`:
- Around line 29-65: The test should also assert that the cloned message is a
deep copy for embedded objects to catch shallow-clone regressions: after calling
MessageManager::copyAsDraftMessage($message) (method copyAsDraftMessage on class
MessageManager), add assertions that $result->getSchedule(),
$result->getContent(), and $result->getOptions() are not the same instances as
the original $message->getSchedule(), $message->getContent(), and
$message->getOptions() respectively; if those assertions fail the implementation
must be updated to deep-clone those properties when creating the draft (inspect
the cloning logic in MessageManager::copyAsDraftMessage and ensure it creates
new MessageSchedule, MessageContent, and MessageOptions instances rather than
copying references).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91c2b3ae-63c5-4539-babd-c4cf12116d49
📒 Files selected for processing (2)
src/Domain/Messaging/Service/Manager/MessageManager.phptests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.php
TatevikGr
force-pushed
the
dev
branch
2 times, most recently
from
0416fe4 to
d7fa96a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Domain/Identity/Service/PermissionChecker.php`:
- Line 74: The returned array from PermissionChecker:: (the expression using
$resource->getListMessages()->map(fn(ListMessage $lm) =>
$lm->getList())->toArray()) can include null because ListMessage::getList() is
nullable; update this to filter out null list relations before returning so
checkRelatedResources() and the owns(...) checks never receive nulls — e.g.,
only include non-null results of getList() (or call ->filter(...) on
getListMessages() or apply array_filter after mapping) so all elements passed
into owns(...) are valid list objects.
In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`:
- Around line 208-210: The flush() call currently occurs before the try/catch in
CampaignProcessorMessageHandler (inside the message handling method), so
exceptions from $this->entityManager->flush() bypass the catch handlers; move
the $this->entityManager->flush() invocation into the existing try block (so it
executes inside the try that contains the recovery/catch logic) to ensure any
flush exceptions are caught and the existing catch handlers and user-message
recovery path run as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 290a7202-6a54-45ab-a259-5c9831081651
📒 Files selected for processing (7)
src/Domain/Analytics/Service/LinkTrackService.phpsrc/Domain/Identity/Service/PermissionChecker.phpsrc/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.phpsrc/Domain/Messaging/Model/Message.phpsrc/Domain/Messaging/Service/Manager/MessageManager.phptests/Unit/Domain/Messaging/Service/Manager/MessageManagerTest.phptests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
✅ Files skipped from review due to trivial changes (3)
- tests/Unit/Domain/Messaging/Service/MessageDataLoaderTest.php
- src/Domain/Messaging/Model/Message.php
- src/Domain/Analytics/Service/LinkTrackService.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Domain/Messaging/Service/Manager/MessageManager.php
| // todo: check which one is correct | ||
| // return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray(); | ||
| return $resource->getListMessages()->map(fn($lm) => $lm->getSubscriberList())->toArray(); | ||
| return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray(); |
There was a problem hiding this comment.
Filter nullable list relations before ownership checks.
At Line 74, ListMessage::getList() is nullable, so this array may contain null. That can break owns(...) checks in checkRelatedResources() or cause false negatives.
Suggested patch
- return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray();
+ return $resource->getListMessages()
+ ->map(fn (ListMessage $lm) => $lm->getList())
+ ->filter(fn (?SubscriberList $list) => $list !== null)
+ ->toArray();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return $resource->getListMessages()->map(fn(ListMessage $lm) => $lm->getList())->toArray(); | |
| return $resource->getListMessages() | |
| ->map(fn (ListMessage $lm) => $lm->getList()) | |
| ->filter(fn (?SubscriberList $list) => $list !== null) | |
| ->toArray(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Domain/Identity/Service/PermissionChecker.php` at line 74, The returned
array from PermissionChecker:: (the expression using
$resource->getListMessages()->map(fn(ListMessage $lm) =>
$lm->getList())->toArray()) can include null because ListMessage::getList() is
nullable; update this to filter out null list relations before returning so
checkRelatedResources() and the owns(...) checks never receive nulls — e.g.,
only include non-null results of getList() (or call ->filter(...) on
getListMessages() or apply array_filter after mapping) so all elements passed
into owns(...) are valid list objects.
Comment on lines
+208
to
210
| $this->entityManager->flush(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Put the new flush inside the try block.
At Line 208, flush() can throw before control reaches the catch handlers, so the user-message recovery path won’t run.
Suggested patch
- $this->entityManager->flush();
-
- try {
+ try {
+ $this->entityManager->flush();
$result = $this->campaignEmailBuilder->buildCampaignEmail(
messageId: $campaign->getId(),
data: $processed,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $this->entityManager->flush(); | |
| try { | |
| try { | |
| $this->entityManager->flush(); | |
| $result = $this->campaignEmailBuilder->buildCampaignEmail( | |
| messageId: $campaign->getId(), | |
| data: $processed, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`
around lines 208 - 210, The flush() call currently occurs before the try/catch
in CampaignProcessorMessageHandler (inside the message handling method), so
exceptions from $this->entityManager->flush() bypass the catch handlers; move
the $this->entityManager->flush() invocation into the existing try block (so it
executes inside the try that contains the recovery/catch logic) to ensure any
flush exceptions are caught and the existing catch handlers and user-message
recovery path run as intended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Tests
Documentation
Thanks for contributing to phpList!