Add forward email endpoint (#164) #165
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
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 9.76% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title 'Add forward email endpoint' directly matches the main objective: introducing a new email forwarding endpoint with controller, request handler, validators, and service components. |
✏️ 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/Messaging/Controller/EmailForwardControllerTest.php (2)
48-50: Prefer the idiomatic header accessor over string casting.
(string)$response->headersdumps all headers as a string. CheckingContent-Typedirectly is more precise and readable.Suggested change
- self::assertStringContainsString('application/json', (string)$response->headers); + self::assertSame('application/json', $response->headers->get('Content-Type'));
14-81: No happy-path test for a successful forward (202).All tests cover error/edge cases, but there's no test verifying a successful forward returns 202 with the expected response body. If setting up the "message received" state via fixtures is feasible, it'd be worth adding — even a minimal one — to ensure the full flow works end-to-end.
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.
composer.json
Outdated
| "require": { | ||
| "php": "^8.1", | ||
| "phplist/core": "dev-main", | ||
| "phplist/core": "dev-ref/campaign-processing", |
There was a problem hiding this comment.
phplist/core pinned to a feature branch — don't forget to update before merge.
Pointing at dev-ref/campaign-processing is fine during development, but this should be updated to a stable ref (tagged release or dev-main) before merging into main. Shipping a feature-branch dependency makes builds fragile if that branch is rebased or deleted.
🤖 Prompt for AI Agents
In `@composer.json` at 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.
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
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores