Conversation
New Features
Email forwarding functionality for campaigns/messages with recipient validation, sender details, and personal notes support
Campaign message processing enhancements
Refactor
Service configuration simplified and consolidated
Updated dependency management
Co-authored-by: Tatevik <tatevikg1@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 5.08% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Title check | ❓ Inconclusive | The title 'Dev' is too vague and generic to meaningfully describe the changeset, which includes substantial features like email forwarding, attachment downloads, and message open tracking. | Provide a more specific and descriptive title that captures the primary changes, such as 'Add email forwarding, attachment downloads, and message open tracking features'. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Merge Conflict Detection | ✅ Passed | ✅ No merge conflicts detected when merging into main |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
dev
No actionable comments were generated in the recent review. 🎉
🧹 Recent nitpick comments
tests/Integration/Statistics/Fixtures/UserMessageFixture.php (1)
18-42: Looks solid overall. 👍One minor thing to consider: this fixture silently returns when prerequisites are missing (Lines 32-35), which could mask fixture ordering issues in tests. Since the test in
MessageOpenTrackControllerTestcontrols the load order explicitly, this works fine in practice. But you might want to implementDependentFixtureInterfaceto declare the dependency onSubscriberFixtureandMessageFixture— that way Doctrine's fixture loader handles ordering automatically if these fixtures are ever used outside of manually-orderedloadFixtures()calls.Totally optional though, no pressure.
src/Statistics/Controller/MessageOpenTrackController.php (2)
21-31: Unnecessary base class dependencies for an unauthenticated endpoint.This controller extends
BaseControllerwhich injectsAuthenticationandRequestValidator, but neither is used here — the tracking pixel is intentionally unauthenticated and takes no request body. Consider either using a plain SymfonyAbstractControlleror a lighter base class to avoid the unused dependencies.Not a blocker, just dead weight in the constructor.
75-86: Include the UID in the error log context.When tracking fails, you log
message_idbut not the subscriber UID. Adding it would make debugging much easier.Suggested tweak
$this->logger->error( 'Failed to track user message view', [ 'exception' => $e, 'message_id' => $messageId, + 'uid' => $uid, ] );tests/Integration/Statistics/Controller/MessageOpenTrackControllerTest.php (2)
53-54: Duplicated GIF constant between test and controller.The base64 GIF string appears both here and in
MessageOpenTrackController::returnPixelResponse()(Line 94). If it ever changes, you'd need to update both places. Consider extracting it to a shared constant.No rush on this — just something to keep tidy.
30-55: Nice coverage of the happy path with full fixture chain and header assertions.One thing that could strengthen this: asserting that the
UserMessageview count actually incremented in the database after the request. Right now the test only validates the HTTP response, so a silently swallowed tracking failure would still pass. Totally fine if you want to keep this as a pure HTTP-layer test though.
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Identity/Controller/AdminAttributeValueController.php (1)
350-353:⚠️ Potential issue | 🔴 CriticalPre-existing bug:
$adminis typed asAdminAttributeDefinitioninstead ofAdministrator.The
$adminparameter on line 352 usesAdminAttributeDefinitionas its type, but it's mappingadminIdand should resolve to anAdministratorentity. This will either silently returnnull(triggering the 404) or map to the wrong table entirely.🐛 Proposed fix
public function getAttributeDefinition( Request $request, - #[MapEntity(mapping: ['adminId' => 'id'])] ?AdminAttributeDefinition $admin, + #[MapEntity(mapping: ['adminId' => 'id'])] ?Administrator $admin, #[MapEntity(mapping: ['definitionId' => 'id'])] ?AdminAttributeDefinition $definition, ): JsonResponse {
🤖 Fix all issues with AI agents
In `@composer.json`:
- Line 45: composer.json currently pins the dependency "phplist/core" to the
feature branch "dev-ref/campaign-processing"; replace that ref with a stable ref
(a tagged release like "vX.Y.Z" or at least "dev-main") in the "phplist/core"
version string, then run composer update phplist/core to refresh composer.lock
and run your tests; look for the "phplist/core" entry in composer.json and
change "dev-ref/campaign-processing" to the chosen stable tag or "dev-main",
update composer.lock, and verify the build passes.
In `@src/Messaging/Controller/EmailForwardController.php`:
- Line 28: The OpenAPI path is inconsistent with the Symfony route: update the
OpenAPI path declaration to match the route by changing the OpenAPI path from
/api/v2/campaigns/{messageId}/forward to /api/v2/email-forward/{messageId};
locate the route attribute #[Route('/email-forward', name: 'email_forward_')]
and the OpenAPI path entry referencing /api/v2/campaigns/{messageId}/forward and
modify that OpenAPI path string so routes and OpenAPI paths follow the
established /api/v2/{resource} pattern.
In `@src/Messaging/Validator/Constraint/MaxForwardCountValidator.php`:
- Around line 13-16: The constructor currently injects $maxForward as string
which is later compared to the int $uniqueCount in MaxForwardCountValidator,
causing fragile type juggling; change the constructor/property to accept an int
(e.g., update the parameter signature in __construct to "private readonly int
$maxForward" and adjust the Autowire attribute to supply the integer config
value) so comparisons with $uniqueCount are type-safe, or alternatively cast
$maxForward to int once (e.g., (int)$this->maxForward) before the comparison
with $uniqueCount.
In `@src/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidator.php`:
- Around line 32-38: The enforced limit ($sizeLimit = $this->maxSize * 2 in
MaxPersonalNoteSizeValidator) does not match the reported limit in the violation
message (you pass $this->maxSize to setParameter); align them by either removing
the unintended multiplier (change $sizeLimit to $this->maxSize) or, if the *2 is
intentional, pass the actual enforced value to the message (use
(string)$sizeLimit or update the constraint message to reference bytes vs
characters). Update the code in MaxPersonalNoteSizeValidator so the value used
to check ($sizeLimit) and the value passed to setParameter('{{ limit }}') are
the same and the message text reflects the correct unit.
🧹 Nitpick comments (7)
composer.json (1)
47-47:symfony/test-packis inrequireinstead ofrequire-dev.This is pre-existing, but worth noting: test dependencies shouldn't ship to production. Consider moving it to
require-devin a follow-up.config/services/services.yml (1)
2-4: Inconsistentpublicvisibility across services.The three new forwarding services explicitly set
public: false, butSubscriberHistoryServicedoesn't specify visibility. If it's intentionally public, that's fine — just flagging the inconsistency in case it should also bepublic: false.src/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidator.php (1)
13-16: Inconsistent type for injected config parameter.
MaxForwardCountValidatorinjects$maxForwardasstring(seeMaxForwardCountValidator.phpLine 14), while this validator injects$maxSizeas?int. This isn't a bug per se, but it creates an inconsistency in how config parameters are handled across the two validators. Consider aligning them — either both asint/?int(cleaner for numeric comparisons) or both asstring.config/services/normalizers.yml (1)
7-9: The_instanceoftag may be redundant withautoconfigure.Since
_defaultsalready hasautoconfigure: true(Line 4), Symfony will automatically tag services implementingNormalizerInterfacewithserializer.normalizer. The explicit_instanceofblock does the same thing, so it's belt-and-suspenders. Not harmful, just potentially unnecessary.src/Messaging/Serializer/ForwardingResultNormalizer.php (1)
36-70: ImplementgetSupportedTypes()for forward compatibility.While
getSupportedTypes()is optional in Symfony 6.4, it's recommended and will be required in 7.0. All normalizers in this codebase are missing it. Consider adding it here and across the other normalizers:Suggested addition
+ public function getSupportedTypes(?string $format): array + { + return [ + ForwardingResult::class => true, + ]; + } + /** * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) */ public function supportsNormalization($data, string $format = null): boolsrc/Messaging/Controller/EmailForwardController.php (1)
61-67: OpenAPI parameter type formessageIdshould beinteger.The route constraint requires digits (
\d+), so the OpenAPI schema type should beintegerrather thanstringto accurately reflect the expected input.new OA\Parameter( name: 'messageId', description: 'message ID', in: 'path', required: true, - schema: new OA\Schema(type: 'string') + schema: new OA\Schema(type: 'integer') )src/Messaging/Request/ForwardMessageRequest.php (1)
56-59: Minor: redundant constraints on$recipients.
#[Assert\NotBlank]on an array already checks it's not empty, which overlaps with#[Assert\Count(min: 1)]. And#[Assert\NotNull]is partially covered byNotBlanktoo. Not harmful, but it means the same input can generate multiple violation messages for the same root cause.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Messaging/Request/ForwardMessageRequest.php`:
- Around line 13-50: The OpenAPI schema for ForwardMessageRequest is
inconsistent with the validation attributes: update the OA\Schema so that 'uid',
'from_name', and 'from_email' are not marked nullable and add them to the
required array (so required => ['recipients','uid','from_name','from_email']),
and ensure the OA\Property definitions for 'from_email' retain the email format
and 'uid'/'from_name' remain string types to match the #[Assert\NotNull] and
#[Assert\NotBlank] constraints on the ForwardMessageRequest properties.
In `@tests/Integration/Messaging/Controller/EmailForwardControllerTest.php`:
- Line 55: The assertion in EmailForwardControllerTest uses
assertStringContainsString with the arguments reversed — currently
assertStringContainsString($data['message'], 'Cannot forward: user has not
received this message'); — so swap the arguments so the needle is the expected
literal ('Cannot forward: user has not received this message') and the haystack
is the actual response ($data['message']) to correctly assert the expected
substring is present in the response.
🧹 Nitpick comments (5)
src/Common/EventListener/ExceptionListener.php (1)
21-23: The@SuppressWarningsfor cyclomatic complexity is fine for now, but this chain is getting long.Every branch does the exact same thing — build a
JsonResponsewith the exception message and a status code, then set it on the event. If more exception types keep landing here, consider refactoring to a map-based approach. No rush though.💡 Possible simplification sketch
+ private const EXCEPTION_STATUS_MAP = [ + AccessDeniedHttpException::class => 403, + SubscriptionCreationException::class => null, // uses getStatusCode() + AdminAttributeCreationException::class => null, + AttributeDefinitionCreationException::class => null, + ValidatorException::class => 400, + AccessDeniedException::class => 403, + MessageNotReceivedException::class => 422, + ];A loop over the map (with a special case for
HttpExceptionInterfaceand classes that carry their own status code) would replace the entire if/elseif chain and make future additions one-liners.Also applies to: 28-76
tests/Integration/Messaging/Controller/EmailForwardControllerTest.php (1)
58-69: Test name says "invalid email" but payload also omits other required fields.The payload at Line 62-64 only includes
recipientswith an invalid email, butuid,from_name, andfrom_emailare also required. The 422 you get might be for the missing required fields rather than the invalid email. Consider providing all other required fields so the test isolates the specific validation path it claims to exercise.Proposed fix
$payload = json_encode([ 'recipients' => ['not-an-email'], + 'uid' => 'fwd-test', + 'from_name' => 'Test', + 'from_email' => 'test@example.com', ]);tests/Unit/Messaging/Validator/Constraint/MaxPersonalNoteSizeValidatorTest.php (3)
15-28:assertTrue(true)is redundant here.The
$this->never()mock expectation already counts as an assertion in PHPUnit 10, so line 27 is just noise. Same applies to lines 43 and 63 in the other tests. No big deal, just a minor cleanup opportunity.✨ Remove the redundant assertions
- $this->assertTrue(true); }(Same for
testSkipsWhenMaxSizeIsNullOrNegativeline 43 andtestNoViolationWhenWithinOrAtLimitline 63.)
30-44: Consider adding amaxSize = 0case. The validator skips when$this->maxSize <= 0, so zero is a distinct boundary worth covering. Not critical though — the null and negative cases already exercise that branch well.
66-88: Good violation path coverage. The mock chain cleanly verifies the message, parameter substitution, and thataddViolation()is called. One thing to keep an eye on: the expected message string on line 78 is hardcoded — ifMaxPersonalNoteSize::$messageever changes, this test will break silently (well, loudly, but for a confusing reason). You could reference$constraint->messagedirectly instead of duplicating the string, though it's a minor point.✨ Use the constraint's message property directly
$context->expects($this->once()) ->method('buildViolation') - ->with('Your personal note must be at most {{ limit }} characters long.') + ->with($constraint->message) ->willReturn($builder);Note: you'd need to move the
$constraintinstantiation (line 84) before the mock setup, or inline it.
tests/Integration/Messaging/Controller/EmailForwardControllerTest.php
Outdated
Show resolved
Hide resolved
New Features
Added an attachment download endpoint that streams files with correct headers (Content-Type, Content-Disposition, Content-Length).
Bug Fixes
Consolidated and standardized JSON error responses with proper HTTP statuses (404 for missing attachments/subscribers/files, 403 for access denied, others mapped appropriately).
Tests
Added integration tests and fixtures covering successful streaming downloads, header/body validation, and not-found/error scenarios.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Messaging/Controller/AttachmentController.php`:
- Around line 53-57: The OA\Response annotation for the 403 status in
AttachmentController.php is mislabeled as "Unauthorized"; update the OA\Response
with response: 403 (the one inside the responses array) to use a correct
description such as "Forbidden" or "Access denied" so the OpenAPI docs reflect
HTTP 403 semantics (look for the OA\Response(response: 403, description:
'Unauthorized') entry and change its description).
- Around line 78-88: The callback that reads from $downloadable->content
(variable $stream) never closes the stream, causing potential file-handle leaks;
modify the anonymous callback (callback: function () use ($downloadable) { ...
}) to ensure $stream->close() is called after the read loop — ideally wrap the
read loop in a try/finally (or ensure a finally-like cleanup) so
$stream->close() runs even if reading errors occur, e.g., after the while
(!$stream->eof()) loop call $stream->close() in the finally block.
🧹 Nitpick comments (2)
src/Common/EventListener/ExceptionListener.php (1)
60-66: Leaking internal exception messages in 500 responsesReturning
$exception->getMessage()verbatim for unhandled exceptions can expose sensitive internals (DB errors, file paths, etc.) to API consumers. Consider using a generic message like"Internal server error"for the 500 case, and logging the real message server-side instead.This is pre-existing behavior so no pressure to fix it in this PR, but something to keep in mind.
Possible tweak
if ($exception instanceof Exception) { $event->setResponse( new JsonResponse([ - 'message' => $exception->getMessage() + 'message' => 'Internal server error.' ], 500) ); }tests/Integration/Messaging/Controller/AttachmentControllerTest.php (1)
16-23: Consider cleanup of the created directory intearDown.
setUpcreates the directory if it doesn't exist (line 20-22), buttearDownonly removes the file, not the directory. This is fine if the directory is expected to persist, but worth noting for test isolation. No action needed if this path is shared across tests.
| responses: [ | ||
| new OA\Response(response: 200, description: 'File stream'), | ||
| new OA\Response(response: 403, description: 'Unauthorized'), | ||
| new OA\Response(response: 404, description: 'Not found'), | ||
| ] |
There was a problem hiding this comment.
OA response 403 says "Unauthorized" — that's typically 401.
HTTP 403 means "Forbidden", not "Unauthorized". If you keep it, the description should be "Forbidden" or "Access denied".
Suggested fix
- new OA\Response(response: 403, description: 'Unauthorized'),
+ new OA\Response(response: 403, description: 'Forbidden'),📝 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.
| responses: [ | |
| new OA\Response(response: 200, description: 'File stream'), | |
| new OA\Response(response: 403, description: 'Unauthorized'), | |
| new OA\Response(response: 404, description: 'Not found'), | |
| ] | |
| responses: [ | |
| new OA\Response(response: 200, description: 'File stream'), | |
| new OA\Response(response: 403, description: 'Forbidden'), | |
| new OA\Response(response: 404, description: 'Not found'), | |
| ] |
🤖 Prompt for AI Agents
In `@src/Messaging/Controller/AttachmentController.php` around lines 53 - 57, The
OA\Response annotation for the 403 status in AttachmentController.php is
mislabeled as "Unauthorized"; update the OA\Response with response: 403 (the one
inside the responses array) to use a correct description such as "Forbidden" or
"Access denied" so the OpenAPI docs reflect HTTP 403 semantics (look for the
OA\Response(response: 403, description: 'Unauthorized') entry and change its
description).
| callback: function () use ($downloadable) { | ||
| $stream = $downloadable->content; | ||
|
|
||
| if ($stream->isSeekable()) { | ||
| $stream->rewind(); | ||
| } | ||
|
|
||
| while (!$stream->eof()) { | ||
| echo $stream->read(8192); | ||
| flush(); | ||
| } |
There was a problem hiding this comment.
Stream resource is never closed after reading.
The callback reads the entire stream but never closes it, which can leak file handles — especially under load. Add $stream->close() after the loop.
Suggested fix
while (!$stream->eof()) {
echo $stream->read(8192);
flush();
}
+
+ $stream->close();📝 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.
| callback: function () use ($downloadable) { | |
| $stream = $downloadable->content; | |
| if ($stream->isSeekable()) { | |
| $stream->rewind(); | |
| } | |
| while (!$stream->eof()) { | |
| echo $stream->read(8192); | |
| flush(); | |
| } | |
| callback: function () use ($downloadable) { | |
| $stream = $downloadable->content; | |
| if ($stream->isSeekable()) { | |
| $stream->rewind(); | |
| } | |
| while (!$stream->eof()) { | |
| echo $stream->read(8192); | |
| flush(); | |
| } | |
| $stream->close(); |
🤖 Prompt for AI Agents
In `@src/Messaging/Controller/AttachmentController.php` around lines 78 - 88, The
callback that reads from $downloadable->content (variable $stream) never closes
the stream, causing potential file-handle leaks; modify the anonymous callback
(callback: function () use ($downloadable) { ... }) to ensure $stream->close()
is called after the read loop — ideally wrap the read loop in a try/finally (or
ensure a finally-like cleanup) so $stream->close() runs even if reading errors
occur, e.g., after the while (!$stream->eof()) loop call $stream->close() in the
finally block.
New Features
Added 1x1 pixel message-open tracking endpoint to record when messages are viewed.
Tests
Added integration tests validating pixel responses, headers, behavior with missing/unknown parameters, and included test fixtures linking messages and subscribers.
Chores
Adjusted CI/config finishing touches related to docstring handling.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores