Conversation
📝 WalkthroughWalkthroughAdds a new CampaignActionController exposing four campaign/message operations (copy as draft, update status, send, resend) with authentication, request validation, MessageManager/CampaignService usage, Doctrine persistence, message-bus dispatch for send/resend, and OpenAPI attributes. Removes send/resend handling and the MessageBus dependency from the existing CampaignController. Adds integration tests for the new controller and removes corresponding tests from the old controller. Updates composer.json to change phplist/core constraint from Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as CampaignActionController
participant Auth as Authentication
participant Manager as MessageManager
participant EM as EntityManager
participant Service as CampaignService
Client->>Controller: POST /api/v2/campaigns/{messageId}/copy
Controller->>Auth: requireAuthentication()
Auth-->>Controller: authenticated
Controller->>Manager: copyAsDraftMessage(message, user)
Manager-->>Controller: new draft message
Controller->>EM: flush()
Controller->>Service: getMessage(newId)
Service-->>Controller: message payload
Controller-->>Client: 201 Created + JSON
sequenceDiagram
autonumber
actor Client
participant Controller as CampaignActionController
participant Auth as Authentication
participant Validator as RequestValidator
participant Manager as MessageManager
participant EM as EntityManager
participant Service as CampaignService
Client->>Controller: PATCH /api/v2/campaigns/{messageId}/status {status}
Controller->>Auth: requireAuthentication()
Auth-->>Controller: authenticated
Controller->>Validator: validate MessageMetadataRequest
Validator-->>Controller: valid
Controller->>Manager: updateStatus(message, MessageStatus)
Manager-->>Controller: updated message
Controller->>EM: flush()
Controller->>Service: getMessage(messageId)
Service-->>Controller: message payload
Controller-->>Client: 200 OK + JSON
sequenceDiagram
autonumber
actor Client
participant Controller as CampaignActionController
participant Auth as Authentication
participant Validator as RequestValidator
participant Bus as MessageBus
participant Service as CampaignService
Client->>Controller: POST /api/v2/campaigns/{messageId}/send or /resend {list_ids?}
Controller->>Auth: requireAuthentication()
Auth-->>Controller: authenticated
Controller->>Validator: (resend) validate ResendMessageToListsRequest
Validator-->>Controller: valid
Controller->>Bus: dispatch SyncCampaignProcessorMessage(messageId, listIds?)
Bus-->>Controller: dispatched
Controller->>Service: getMessage(messageId)
Service-->>Controller: message payload
Controller-->>Client: 200 OK + JSON
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: 4
🧹 Nitpick comments (1)
tests/Integration/Messaging/Controller/CampaignActionControllerTest.php (1)
47-76: Consider adding validation error tests for resend endpointThe
ResendMessageToListsRequesthas validation constraints (list_ids required, non-empty array, positive integers, list must exist). Tests currently only cover the happy path and 403/404 scenarios.For more robust coverage, consider adding tests for:
- Missing
list_ids→ expects 400 or 422- Empty
list_idsarray → expects 422- Non-existent list ID → expects 422
This isn't blocking for a beta endpoint, just a nice-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Messaging/Controller/CampaignActionControllerTest.php` around lines 47 - 76, Add three tests to cover validation failures for the resend endpoint: create testMissingListIdsReturnsUnprocessable, testEmptyListIdsReturnsUnprocessable, and testNonExistentListIdReturnsUnprocessable; each should load the minimal fixtures so campaign id 2 exists (use MessageFixture and SubscriberListFixture where needed), call authenticatedJsonRequest('POST', '/api/v2/campaigns/2/resend', ..., json_encode(...)) with respectively (a) no body or missing list_ids, (b) {'list_ids'=>[]}, and (c) {'list_ids'=>[99999]} and then assertHttpUnprocessable() and that getDecodedJsonResponseContent() contains a validation error for the 'list_ids' field; reference ResendMessageToListsRequest and the existing test methods (testResendMessageToListsWithValidSessionReturnsOkay) to mirror setup and assertions.
🤖 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/Messaging/Controller/CampaignActionController.php`:
- Line 181: The response currently returns the raw $message via
$this->json($message, Response::HTTP_OK) which is inconsistent with copyMessage
and sendMessage; change this method to serialize the message using
$this->campaignService->getMessage($message) and return that result (e.g.,
$this->json($this->campaignService->getMessage($message), Response::HTTP_OK)) so
all three actions use the same response structure; update any variable names
accordingly and ensure you reference CampaignActionController and
campaignService->getMessage when making the change.
- Around line 168-178: The PATCH handler in CampaignActionController is reading
status from $request->request (form data) which is empty for JSON payloads;
change it to parse JSON from $request->getContent() (e.g., decode to array and
read 'status') and validate presence; also guard the MessageStatus conversion by
catching the ValueError thrown by MessageStatus::from (or use try-from pattern)
and return a 400/422 JSON error response on invalid values instead of letting it
bubble to a 500; update the call site where $message =
$this->messageManager->updateStatus(...) is invoked to use the validated enum
value.
- Around line 207-218: Add a 404 OpenAPI response to the responses arrays for
both sendMessage and copyMessage in CampaignActionController.php: update the
responses list inside the OA\Response annotations for the sendMessage and
copyMessage endpoints to include a new OA\Response(response: 404, description:
'Not Found', content: new OA\JsonContent(ref:
'#/components/schemas/NotFoundResponse')) so the documentation reflects the
controller's 404 return when a campaign or resource is not found.
- Around line 240-244: The OpenAPI requestBody annotation in
CampaignActionController is pointing to the wrong schema
(AdminAttributeDefinitionRequest); update the requestBody content ref to the
correct schema (ResendMessageToListsRequest) so the endpoint documents the
expected list_ids payload correctly—locate the requestBody block inside
CampaignActionController (the OA\RequestBody for this resend/list-send action)
and replace the ref with '#/components/schemas/ResendMessageToListsRequest'.
---
Nitpick comments:
In `@tests/Integration/Messaging/Controller/CampaignActionControllerTest.php`:
- Around line 47-76: Add three tests to cover validation failures for the resend
endpoint: create testMissingListIdsReturnsUnprocessable,
testEmptyListIdsReturnsUnprocessable, and
testNonExistentListIdReturnsUnprocessable; each should load the minimal fixtures
so campaign id 2 exists (use MessageFixture and SubscriberListFixture where
needed), call authenticatedJsonRequest('POST', '/api/v2/campaigns/2/resend',
..., json_encode(...)) with respectively (a) no body or missing list_ids, (b)
{'list_ids'=>[]}, and (c) {'list_ids'=>[99999]} and then
assertHttpUnprocessable() and that getDecodedJsonResponseContent() contains a
validation error for the 'list_ids' field; reference ResendMessageToListsRequest
and the existing test methods
(testResendMessageToListsWithValidSessionReturnsOkay) to mirror setup and
assertions.
🪄 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: b3d5f8af-378e-4bb9-974e-aecec35384c1
📒 Files selected for processing (4)
src/Messaging/Controller/CampaignActionController.phpsrc/Messaging/Controller/CampaignController.phptests/Integration/Messaging/Controller/CampaignActionControllerTest.phptests/Integration/Messaging/Controller/CampaignControllerTest.php
💤 Files with no reviewable changes (2)
- src/Messaging/Controller/CampaignController.php
- tests/Integration/Messaging/Controller/CampaignControllerTest.php
Comment on lines
+207
to
+218
| responses: [ | ||
| new OA\Response( | ||
| response: 200, | ||
| description: 'Success', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | ||
| ), | ||
| new OA\Response( | ||
| response: 403, | ||
| description: 'Failure', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Missing 404 response in OpenAPI documentation
The sendMessage endpoint can return 404 when the campaign is not found (line 226), but this response isn't documented. Same applies to copyMessage (lines 67-78).
📝 Add 404 response to OpenAPI
responses: [
new OA\Response(
response: 200,
description: 'Success',
content: new OA\JsonContent(ref: '#/components/schemas/Message')
),
new OA\Response(
response: 403,
description: 'Failure',
content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse')
- )
+ ),
+ new OA\Response(
+ response: 404,
+ description: 'Campaign not found',
+ content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse')
+ )
]📝 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
| responses: [ | |
| new OA\Response( | |
| response: 200, | |
| description: 'Success', | |
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | |
| ), | |
| new OA\Response( | |
| response: 403, | |
| description: 'Failure', | |
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | |
| ) | |
| ] | |
| responses: [ | |
| new OA\Response( | |
| response: 200, | |
| description: 'Success', | |
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | |
| ), | |
| new OA\Response( | |
| response: 403, | |
| description: 'Failure', | |
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | |
| ), | |
| new OA\Response( | |
| response: 404, | |
| description: 'Campaign not found', | |
| content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') | |
| ) | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Messaging/Controller/CampaignActionController.php` around lines 207 -
218, Add a 404 OpenAPI response to the responses arrays for both sendMessage and
copyMessage in CampaignActionController.php: update the responses list inside
the OA\Response annotations for the sendMessage and copyMessage endpoints to
include a new OA\Response(response: 404, description: 'Not Found', content: new
OA\JsonContent(ref: '#/components/schemas/NotFoundResponse')) so the
documentation reflects the controller's 404 return when a campaign or resource
is not found.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Messaging/Controller/CampaignActionController.php (3)
167-167:⚠️ Potential issue | 🟡 MinorInconsistent response serialization.
This method returns
$messagedirectly whilecopyMessage,sendMessage, andresendMessageToListsall use$this->campaignService->getMessage($message). This could result in different response structures.🔧 Proposed fix
- return $this->json($message, Response::HTTP_OK); + return $this->json($this->campaignService->getMessage($message), Response::HTTP_OK);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Messaging/Controller/CampaignActionController.php` at line 167, The controller is returning the raw $message causing inconsistent serialization; change the response to use the canonical serializer by calling $this->campaignService->getMessage($message) and return that in the JSON response so it matches copyMessage, sendMessage, and resendMessageToLists; update the return statement in CampaignActionController to return $this->json($this->campaignService->getMessage($message), Response::HTTP_OK).
68-79:⚠️ Potential issue | 🟡 MinorAdd 404 response to OpenAPI documentation.
The endpoint throws a not-found exception (line 87) when the campaign doesn't exist, but the OpenAPI responses don't document this case.
📝 Proposed fix
new OA\Response( response: 403, description: 'Failure', content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') - ) + ), + new OA\Response( + response: 404, + description: 'Campaign not found', + content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') + ) ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Messaging/Controller/CampaignActionController.php` around lines 68 - 79, The OpenAPI responses block in CampaignActionController is missing the 404 case thrown when a campaign isn't found; update the responses array (the OA\Response entries in the annotations for the relevant controller method in CampaignActionController.php) to include a new OA\Response with response: 404, a description like 'Not Found', and a content pointing to the appropriate error schema (e.g. ref: '#/components/schemas/NotFoundResponse' or the existing error schema used for not-found errors) so the not-found exception at the campaign lookup is documented.
161-164:⚠️ Potential issue | 🟠 MajorUnhandled
ValueErrorfromMessageStatus::from()will cause 500.The
MessageMetadataRequestonly validates thatstatusis not blank. If an invalid enum value is provided (e.g.,"invalid_status"),MessageStatus::from()throws aValueErrorthat bubbles up as a 500 error instead of a proper 400/422.Consider catching the exception here, or adding an
@Assert\Choiceconstraint toMessageMetadataRequestto validate against valid enum values.🐛 Proposed fix
/** `@var` MessageMetadataRequest $messageMetadataRequest */ $messageMetadataRequest = $this->validator->validate($request, MessageMetadataRequest::class); + try { + $messageStatus = MessageStatus::from($messageMetadataRequest->status); + } catch (\ValueError) { + return $this->json( + ['error' => 'Invalid "status" value.'], + Response::HTTP_UNPROCESSABLE_ENTITY + ); + } + $message = $this->messageManager->updateStatus( $message, - MessageStatus::from($messageMetadataRequest->status), + $messageStatus, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Messaging/Controller/CampaignActionController.php` around lines 161 - 164, CampaignActionController currently calls MessageStatus::from($messageMetadataRequest->status) without handling invalid enum values, which lets a ValueError bubble up and produce a 500; either validate the DTO or catch the exception: add an `@Assert`\Choice constraint on MessageMetadataRequest->status listing MessageStatus::cases() (ensuring invalid values are rejected before controller) or wrap the call in CampaignActionController around MessageStatus::from(...) in a try/catch(ValueError $e) and convert it to a client error (throw a BadRequestHttpException or return a 400/422 with a clear message) before calling $this->messageManager->updateStatus(...).
🤖 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/Messaging/Controller/CampaignActionController.php`:
- Around line 253-264: The OpenAPI responses for the endpoint in
CampaignActionController:: (the responses array shown) are missing the 404
not-found response; update the responses block to include a 404 OA\Response with
description 'Not Found' and content referencing the same schema used by
sendMessage's spec (e.g., OA\JsonContent(ref:
'#/components/schemas/NotFoundResponse') or whichever NotFound schema
sendMessage uses) so the documented responses mirror sendMessage and cover the
case where the campaign is not found (the code path that throws the not-found
error).
---
Duplicate comments:
In `@src/Messaging/Controller/CampaignActionController.php`:
- Line 167: The controller is returning the raw $message causing inconsistent
serialization; change the response to use the canonical serializer by calling
$this->campaignService->getMessage($message) and return that in the JSON
response so it matches copyMessage, sendMessage, and resendMessageToLists;
update the return statement in CampaignActionController to return
$this->json($this->campaignService->getMessage($message), Response::HTTP_OK).
- Around line 68-79: The OpenAPI responses block in CampaignActionController is
missing the 404 case thrown when a campaign isn't found; update the responses
array (the OA\Response entries in the annotations for the relevant controller
method in CampaignActionController.php) to include a new OA\Response with
response: 404, a description like 'Not Found', and a content pointing to the
appropriate error schema (e.g. ref: '#/components/schemas/NotFoundResponse' or
the existing error schema used for not-found errors) so the not-found exception
at the campaign lookup is documented.
- Around line 161-164: CampaignActionController currently calls
MessageStatus::from($messageMetadataRequest->status) without handling invalid
enum values, which lets a ValueError bubble up and produce a 500; either
validate the DTO or catch the exception: add an `@Assert`\Choice constraint on
MessageMetadataRequest->status listing MessageStatus::cases() (ensuring invalid
values are rejected before controller) or wrap the call in
CampaignActionController around MessageStatus::from(...) in a
try/catch(ValueError $e) and convert it to a client error (throw a
BadRequestHttpException or return a 400/422 with a clear message) before calling
$this->messageManager->updateStatus(...).
🪄 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: 17abf6bf-89e7-4d4b-b281-740c5c1b9856
📒 Files selected for processing (5)
src/Messaging/Controller/CampaignActionController.phpsrc/Messaging/Controller/CampaignController.phpsrc/Messaging/Request/Message/MessageMetadataRequest.phptests/Integration/Messaging/Controller/CampaignActionControllerTest.phptests/Integration/Messaging/Controller/CampaignControllerTest.php
💤 Files with no reviewable changes (2)
- tests/Integration/Messaging/Controller/CampaignControllerTest.php
- src/Messaging/Controller/CampaignController.php
✅ Files skipped from review due to trivial changes (1)
- src/Messaging/Request/Message/MessageMetadataRequest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Integration/Messaging/Controller/CampaignActionControllerTest.php
Comment on lines
+253
to
+264
| responses: [ | ||
| new OA\Response( | ||
| response: 200, | ||
| description: 'Success', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | ||
| ), | ||
| new OA\Response( | ||
| response: 403, | ||
| description: 'Failure', | ||
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Missing 404 response in OpenAPI documentation.
The endpoint throws not-found (line 272) when the campaign doesn't exist, but this isn't documented in the responses. Should mirror sendMessage's OpenAPI spec for consistency.
📝 Proposed fix
new OA\Response(
response: 403,
description: 'Failure',
content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse')
- )
+ ),
+ new OA\Response(
+ response: 404,
+ description: 'Campaign not found',
+ content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse')
+ )
]📝 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
| responses: [ | |
| new OA\Response( | |
| response: 200, | |
| description: 'Success', | |
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | |
| ), | |
| new OA\Response( | |
| response: 403, | |
| description: 'Failure', | |
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | |
| ) | |
| ] | |
| responses: [ | |
| new OA\Response( | |
| response: 200, | |
| description: 'Success', | |
| content: new OA\JsonContent(ref: '#/components/schemas/Message') | |
| ), | |
| new OA\Response( | |
| response: 403, | |
| description: 'Failure', | |
| content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') | |
| ), | |
| new OA\Response( | |
| response: 404, | |
| description: 'Campaign not found', | |
| content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') | |
| ) | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Messaging/Controller/CampaignActionController.php` around lines 253 -
264, The OpenAPI responses for the endpoint in CampaignActionController:: (the
responses array shown) are missing the 404 not-found response; update the
responses block to include a 404 OA\Response with description 'Not Found' and
content referencing the same schema used by sendMessage's spec (e.g.,
OA\JsonContent(ref: '#/components/schemas/NotFoundResponse') or whichever
NotFound schema sendMessage uses) so the documented responses mirror sendMessage
and cover the case where the campaign is not found (the code path that throws
the not-found error).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Messaging/Controller/CampaignActionController.php (2)
255-266:⚠️ Potential issue | 🟡 MinorMissing 404 response in OpenAPI for resend endpoint
The controller throws a not-found exception at line 274 when the campaign doesn't exist, but the OpenAPI spec only documents 200 and 403 responses. This should mirror
sendMessage's spec for consistency.📝 Proposed fix
responses: [ new OA\Response( response: 200, description: 'Success', content: new OA\JsonContent(ref: '#/components/schemas/Message') ), new OA\Response( response: 403, description: 'Failure', content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') - ) + ), + new OA\Response( + response: 404, + description: 'Failure', + content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') + ) ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Messaging/Controller/CampaignActionController.php` around lines 255 - 266, The OpenAPI responses for the resend endpoint in CampaignActionController are missing a 404 entry (the controller throws a not-found when the campaign doesn't exist); update the responses array in the resend action to include a 404 OA\Response (e.g., response: 404, description: 'Not Found' or 'Campaign not found', content: new OA\JsonContent(ref: '#/components/schemas/NotFoundResponse')) so it matches the sendMessage spec and documents the not-found case.
70-81:⚠️ Potential issue | 🟡 MinorMissing 404 response in OpenAPI for copy endpoint
Similar to the resend endpoint, this method throws not-found at line 89 but the OpenAPI spec doesn't document the 404 response.
📝 Proposed fix
responses: [ new OA\Response( response: 201, description: 'Success', content: new OA\JsonContent(ref: '#/components/schemas/Message') ), new OA\Response( response: 403, description: 'Failure', content: new OA\JsonContent(ref: '#/components/schemas/UnauthorizedResponse') - ) + ), + new OA\Response( + response: 404, + description: 'Failure', + content: new OA\JsonContent(ref: '#/components/schemas/NotFoundErrorResponse') + ) ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Messaging/Controller/CampaignActionController.php` around lines 70 - 81, The OpenAPI annotation for the copy endpoint in CampaignActionController (the method that throws a not-found at the location around line 89) is missing a 404 response; update the responses array for that method to include a new OA\Response with response: 404, an appropriate description like 'Not Found', and content pointing to the project's NotFound response schema (e.g. new OA\JsonContent(ref: '#/components/schemas/NotFoundResponse')) so it matches the resend endpoint's documented responses.
🧹 Nitpick comments (2)
tests/Integration/Messaging/Controller/CampaignActionControllerTest.php (2)
68-76: Same fixture loading inconsistency for resend not-found testSimilar to the send not-found test -
AdministratorFixturemight be needed for authenticated requests.📝 Suggested improvement
public function testResendMessageToListsWithInvalidIdReturnsNotFound(): void { - $this->loadFixtures([SubscriberListFixture::class]); + $this->loadFixtures([AdministratorFixture::class, SubscriberListFixture::class]); $this->authenticatedJsonRequest('POST', '/api/v2/campaigns/999/resend', [], [], [], json_encode([ 'list_ids' => [1], ]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Messaging/Controller/CampaignActionControllerTest.php` around lines 68 - 76, The test method testResendMessageToListsWithInvalidIdReturnsNotFound loads only SubscriberListFixture but performs an authenticated request; add the AdministratorFixture to the fixtures loaded so authentication context exists. Update the loadFixtures call in testResendMessageToListsWithInvalidIdReturnsNotFound to include AdministratorFixture alongside SubscriberListFixture so the authenticatedJsonRequest has a valid administrator user for the request.
41-45: Missing fixture loads for not-found testThis test calls
authenticatedJsonRequestbut doesn't load any fixtures. For consistency and to ensure the admin session exists, consider loadingAdministratorFixture. The test might still pass ifauthenticatedJsonRequesthandles this internally, but explicit fixture loading makes the test's dependencies clearer.📝 Suggested improvement
public function testSendMessageWithInvalidIdReturnsNotFound(): void { + $this->loadFixtures([AdministratorFixture::class]); + $this->authenticatedJsonRequest('POST', '/api/v2/campaigns/999/send'); $this->assertHttpNotFound(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Integration/Messaging/Controller/CampaignActionControllerTest.php` around lines 41 - 45, The test method testSendMessageWithInvalidIdReturnsNotFound is missing explicit fixture setup; load the AdministratorFixture before calling authenticatedJsonRequest so the admin session is guaranteed to exist. Update the test to call the fixture loader (e.g., loadFixture or the test helper that loads fixtures) to include AdministratorFixture prior to invoking authenticatedJsonRequest('POST', '/api/v2/campaigns/999/send') and then assertHttpNotFound(); this makes the dependency on an admin account explicit and stable.
🤖 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/Messaging/Controller/CampaignActionController.php`:
- Around line 163-169: Add an Assert\Choice constraint to the
MessageMetadataRequest::$status validation so invalid enum backing values are
rejected before MessageStatus::from() is called: update the
MessageMetadataRequest class to mirror the approach used in MessageFormatRequest
by adding #[Assert\Choice([...])] with the valid backing strings from the
MessageStatus enum (use the exact enum backing values, e.g. 'draft','sent',
etc.), ensuring the controller never receives bad values that would cause
MessageStatus::from() to throw.
---
Duplicate comments:
In `@src/Messaging/Controller/CampaignActionController.php`:
- Around line 255-266: The OpenAPI responses for the resend endpoint in
CampaignActionController are missing a 404 entry (the controller throws a
not-found when the campaign doesn't exist); update the responses array in the
resend action to include a 404 OA\Response (e.g., response: 404, description:
'Not Found' or 'Campaign not found', content: new OA\JsonContent(ref:
'#/components/schemas/NotFoundResponse')) so it matches the sendMessage spec and
documents the not-found case.
- Around line 70-81: The OpenAPI annotation for the copy endpoint in
CampaignActionController (the method that throws a not-found at the location
around line 89) is missing a 404 response; update the responses array for that
method to include a new OA\Response with response: 404, an appropriate
description like 'Not Found', and content pointing to the project's NotFound
response schema (e.g. new OA\JsonContent(ref:
'#/components/schemas/NotFoundResponse')) so it matches the resend endpoint's
documented responses.
---
Nitpick comments:
In `@tests/Integration/Messaging/Controller/CampaignActionControllerTest.php`:
- Around line 68-76: The test method
testResendMessageToListsWithInvalidIdReturnsNotFound loads only
SubscriberListFixture but performs an authenticated request; add the
AdministratorFixture to the fixtures loaded so authentication context exists.
Update the loadFixtures call in
testResendMessageToListsWithInvalidIdReturnsNotFound to include
AdministratorFixture alongside SubscriberListFixture so the
authenticatedJsonRequest has a valid administrator user for the request.
- Around line 41-45: The test method testSendMessageWithInvalidIdReturnsNotFound
is missing explicit fixture setup; load the AdministratorFixture before calling
authenticatedJsonRequest so the admin session is guaranteed to exist. Update the
test to call the fixture loader (e.g., loadFixture or the test helper that loads
fixtures) to include AdministratorFixture prior to invoking
authenticatedJsonRequest('POST', '/api/v2/campaigns/999/send') and then
assertHttpNotFound(); this makes the dependency on an admin account explicit and
stable.
🪄 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: 62257dc0-4a64-40e6-bb2b-78397fd388ee
📒 Files selected for processing (5)
src/Messaging/Controller/CampaignActionController.phpsrc/Messaging/Controller/CampaignController.phpsrc/Messaging/Request/Message/MessageMetadataRequest.phptests/Integration/Messaging/Controller/CampaignActionControllerTest.phptests/Integration/Messaging/Controller/CampaignControllerTest.php
💤 Files with no reviewable changes (2)
- tests/Integration/Messaging/Controller/CampaignControllerTest.php
- src/Messaging/Controller/CampaignController.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Messaging/Request/Message/MessageMetadataRequest.php
Comment on lines
+163
to
+169
| $message = $this->messageManager->updateStatus( | ||
| $message, | ||
| MessageStatus::from($messageMetadataRequest->status), | ||
| ); | ||
| $this->entityManager->flush(); | ||
|
|
||
| return $this->json($this->messageNormalizer->normalize($message), Response::HTTP_OK); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check MessageMetadataRequest validation constraints for status
ast-grep --pattern $'class MessageMetadataRequest {
$$$
}'
# Also check for any validation attributes on the status property
rg -A 10 'class MessageMetadataRequest' --type phpRepository: phpList/rest-api
Length of output: 972
🏁 Script executed:
#!/bin/bash
# Get the complete MessageMetadataRequest class
cat src/Messaging/Request/Message/MessageMetadataRequest.php
# Also find and display the MessageStatus enum
fd -t f 'MessageStatus.php' --type phpRepository: phpList/rest-api
Length of output: 1178
🏁 Script executed:
#!/bin/bash
# Find MessageStatus enum file
fd 'MessageStatus' -x file {} \;
# Search for MessageStatus enum definition
rg 'enum MessageStatus' -A 20Repository: phpList/rest-api
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for any Choice validator on status field
rg 'Choice|Enum' src/Messaging/Request/Message/MessageMetadataRequest.php
# Search for exception handling around MessageStatus::from
rg -B 5 -A 5 'MessageStatus::from' src/
# Check if there's any validation elsewhere for enum values
rg '#\[Assert.*status' src/
# Look for any exception handling in the controller
rg -B 10 -A 10 'MessageStatus::from' src/Messaging/Controller/CampaignActionController.phpRepository: phpList/rest-api
Length of output: 2778
🏁 Script executed:
#!/bin/bash
# Check for global exception handlers that might catch ValueError
rg 'ValueError|catch.*Value' src/ --type php
# Check for enum value constraint definitions
rg 'Choice|Enum' src/Messaging/Request/ -A 2Repository: phpList/rest-api
Length of output: 566
🏁 Script executed:
#!/bin/bash
# Double-check MessageMetadataRequest doesn't have Choice constraint elsewhere
cat src/Messaging/Request/Message/MessageMetadataRequest.php | grep -i "choice\|enum"
# Verify the exact line range being reviewed
sed -n '163,169p' src/Messaging/Controller/CampaignActionController.phpRepository: phpList/rest-api
Length of output: 348
Add Assert\Choice validation to the status field in MessageMetadataRequest
The status field currently only has #[Assert\NotBlank] validation. Without an Assert\Choice constraint (like used in MessageFormatRequest), invalid enum backing values slip through validation and cause MessageStatus::from() to throw an unhandled ValueError, returning a 500 instead of a proper 400/422 response.
Add constraint: #[Assert\Choice(['draft', 'sent', ...])] with valid enum backing values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Messaging/Controller/CampaignActionController.php` around lines 163 -
169, Add an Assert\Choice constraint to the MessageMetadataRequest::$status
validation so invalid enum backing values are rejected before
MessageStatus::from() is called: update the MessageMetadataRequest class to
mirror the approach used in MessageFormatRequest by adding
#[Assert\Choice([...])] with the valid backing strings from the MessageStatus
enum (use the exact enum backing values, e.g. 'draft','sent', etc.), ensuring
the controller never receives bad values that would cause MessageStatus::from()
to throw.
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
Thanks for contributing to phpList!