Refactor/admin attributes routes #163
Conversation
|
Warning Rate limit exceeded@TatevikGr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.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 'Refactor/admin attributes routes' clearly describes the main change: refactoring admin attributes API routes to use a new /attributes namespace structure. |
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Subscription/Controller/ListMembersController.php (1)
125-127: Fix the OpenAPI path and parameter type ingetSubscribersCountBoth issues are real:
Line 127: OA path
/api/v2/lists/{listId}/countshould include/subscribersto match the actual route at line 125:/api/v2/lists/{listId}/subscribers/countLine 145:
listIdparameter type is'string'but should be'integer'to match the first endpoint (line 52) and the numeric route requirementrequirements: ['listId' => '\d+']src/Identity/Controller/AdminAttributeValueController.php (1)
293-367: Fix wrong entity mapping/type and handle missing attribute ingetAttributeDefinitionHere there are two coupled issues:
- The
adminparameter is mapped as anAdminAttributeDefinitioninstead of anAdministrator, while usingadminIdfrom the path. This will hydrate the wrong entity type and can easily lead to incorrect IDs being passed intogetAdminAttributeor subtle failures.getAdminAttribute(...)can returnnull(see the explicit null-check indelete()), but here its result is passed straight into the normalizer without a guard. That can yield a 500 or a misleading 200 with empty data instead of the documented 404.Consider tightening this endpoint as follows.
- public function getAttributeDefinition( - Request $request, - #[MapEntity(mapping: ['adminId' => 'id'])] ?AdminAttributeDefinition $admin, - #[MapEntity(mapping: ['definitionId' => 'id'])] ?AdminAttributeDefinition $definition, - ): JsonResponse { + public function getAttributeDefinition( + Request $request, + #[MapEntity(mapping: ['adminId' => 'id'])] ?Administrator $admin = null, + #[MapEntity(mapping: ['definitionId' => 'id'])] ?AdminAttributeDefinition $definition = null, + ): JsonResponse { @@ - if (!$definition || !$admin) { + if (!$definition || !$admin) { throw $this->createNotFoundException('Administrator attribute not found.'); } - $attribute = $this->attributeManager->getAdminAttribute( - adminId: $admin->getId(), - attributeDefinitionId: $definition->getId() - ); - - return $this->json( - $this->normalizer->normalize($attribute), - Response::HTTP_OK - ); + $attribute = $this->attributeManager->getAdminAttribute( + adminId: $admin->getId(), + attributeDefinitionId: $definition->getId() + ); + + if ($attribute === null) { + throw $this->createNotFoundException('Administrator attribute not found.'); + } + + return $this->json( + $this->normalizer->normalize($attribute), + Response::HTTP_OK + );(Optional follow‑up: the method name
getAttributeDefinitionnow effectively returns an attribute value, not the definition; renaming it in a future cleanup would make the intent clearer.)
🧹 Nitpick comments (6)
src/Messaging/Controller/CampaignController.php (3)
155-165: Null‑guard before auth ingetMessagechanges semantics; confirm this is desiredThe explicit
if ($message === null) { createNotFoundException(...) }is fine and keeps the controller from passingnullinto the service, but putting this check beforerequireAuthentication()means unauthenticated callers now see 404 vs 403 for non‑existent IDs. That’s a behavior change and also makesgetMessage/updateMessagediffer fromsendMessage/deleteMessagein ordering.If you want to avoid exposing resource existence to unauthenticated clients or keep all campaign endpoints consistent, consider either:
- calling
requireAuthentication()before the null‑check, or- updating the other endpoints (and their docs) to follow the same pattern.
Otherwise, the guard itself is fine.
Please double‑check with your API behavior/security expectations and adjust the ordering if needed.
294-309: Same null‑guard / auth‑ordering considerations inupdateMessageThe null check with
createNotFoundException('Campaign not found.')is correct and prevents passingnullinto the service, but—just likegetMessage—it runs before$this->requireAuthentication($request). That means unauthenticated callers get 404 for unknown IDs instead of 403, and this differs fromsendMessage/deleteMessagewhere auth is enforced first.If that behavior change is intentional, you may want to:
- align
sendMessage(and its OA docs) to also surface 404, and/or- document this pattern as the standard for campaign routes.
If not intentional, move the null‑guard after
requireAuthentication().Please confirm the desired contract for 403 vs 404 across all campaign endpoints and adjust for consistency.
387-399: Consider documenting the 404 case forsendMessageas well
sendMessagealready throwscreateNotFoundException('Campaign not found.')when$message === null, so at runtime clients can get a 404. The OA responses, however, only advertise 200 and 403.For consistency with
getMessage/updateMessageand to make client handling easier, consider adding a404 NotFoundErrorResponseto this endpoint’s OpenAPI responses too.After adding it, regenerate your OpenAPI spec to ensure the new 404 response is visible in the docs.
src/Identity/Controller/AdministratorController.php (1)
266-303: Delete endpoint rename is correct; tiny 204/JSON nitThe delete endpoint also uses
adminIdconsistently across route, documentation, andMapEntity, so the refactor is solid. As a minor non‑blocking nit, if you ever touch this again you might consider returning an emptyResponsefor 204 instead ofjson(null, 204)to align strictly with HTTP semantics.src/Identity/Controller/AdminAttributeValueController.php (1)
135-205: Delete endpoint docs return 200, but implementation returns 204The route/OA path changes for delete (
/{adminId}/attributes/{definitionId}and corresponding OA path) are good and match the new tests. However, OA declares a200success response, while the controller returnsResponse::HTTP_NO_CONTENT(204) in Line 203. That mismatch may confuse API consumers and generated clients.- new OA\Response( - response: 200, - description: 'Success' - ), + new OA\Response( + response: 204, + description: 'Success (resource deleted, no content)' + ),tests/Integration/Identity/Controller/AdminAttributeValueControllerTest.php (1)
29-183: Test URLs match the new/administrators/{adminId}/attributesAPI surfaceAll integration tests now hit
/api/v2/administrators/{adminId}/attributes/{definitionId}(and/attributesfor listing), which lines up with the controller’s new routing and OA paths. The scenarios for valid CRUD, invalid JSON, bad definitionId, and bad adminId look good.If you want to exercise the new 404 behavior on the list endpoint specifically, you could add a small test that calls
GET /api/v2/administrators/999999/attributesand asserts 404, mirroring the create‑with‑invalid‑adminId case.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/Common/Validator/RequestValidator.php(0 hunks)src/Identity/Controller/AdminAttributeValueController.php(6 hunks)src/Identity/Controller/AdministratorController.php(7 hunks)src/Identity/Request/UpdateAdministratorRequest.php(0 hunks)src/Messaging/Controller/CampaignController.php(3 hunks)src/Messaging/Controller/TemplateController.php(2 hunks)src/Messaging/Request/CreateTemplateRequest.php(1 hunks)src/Messaging/Request/UpdateMessageRequest.php(0 hunks)src/Subscription/Controller/ListMembersController.php(2 hunks)src/Subscription/Controller/SubscriberController.php(1 hunks)src/Subscription/Request/UpdateSubscriberRequest.php(0 hunks)tests/Integration/Identity/Controller/AdminAttributeValueControllerTest.php(9 hunks)tests/Unit/Identity/Request/UpdateAdministratorRequestTest.php(0 hunks)tests/Unit/Messaging/Request/UpdateMessageRequestTest.php(0 hunks)tests/Unit/Subscription/Request/UpdateSubscriberRequestTest.php(0 hunks)
💤 Files with no reviewable changes (7)
- src/Common/Validator/RequestValidator.php
- src/Subscription/Request/UpdateSubscriberRequest.php
- tests/Unit/Subscription/Request/UpdateSubscriberRequestTest.php
- tests/Unit/Messaging/Request/UpdateMessageRequestTest.php
- src/Messaging/Request/UpdateMessageRequest.php
- src/Identity/Request/UpdateAdministratorRequest.php
- tests/Unit/Identity/Request/UpdateAdministratorRequestTest.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Messaging/Request/CreateTemplateRequest.php (4)
src/Messaging/Request/Message/MessageMetadataRequest.php (1)
OA(12-33)src/Messaging/Serializer/BounceRegexNormalizer.php (1)
OA(11-57)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)src/Subscription/Serializer/SubscribePageNormalizer.php (1)
OA(12-52)
src/Subscription/Controller/SubscriberController.php (1)
src/Subscription/Request/UpdateSubscriberRequest.php (1)
getDto(49-59)
src/Messaging/Controller/TemplateController.php (1)
src/Messaging/Request/CreateTemplateRequest.php (1)
OA(14-78)
src/Identity/Controller/AdministratorController.php (3)
src/Subscription/Controller/SubscriberController.php (8)
Route(32-512)Route(47-105)Route(107-181)Route(183-232)Route(234-331)Route(334-390)Route(392-460)Route(462-511)src/Messaging/Controller/CampaignController.php (2)
Route(29-413)Route(47-110)src/Subscription/Controller/SubscriberAttributeDefinitionController.php (6)
Route(24-364)Route(45-101)Route(103-169)Route(171-224)Route(226-291)Route(293-363)
src/Messaging/Controller/CampaignController.php (2)
src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)src/Messaging/Service/CampaignService.php (1)
getMessage(38-45)
src/Identity/Controller/AdminAttributeValueController.php (3)
src/Subscription/Controller/SubscriberController.php (8)
Route(32-512)Route(47-105)Route(107-181)Route(183-232)Route(234-331)Route(334-390)Route(392-460)Route(462-511)src/Messaging/Controller/TemplateController.php (5)
Route(28-285)Route(49-114)Route(116-168)Route(170-228)Route(230-284)src/Subscription/Controller/ListMembersController.php (3)
Route(22-187)Route(39-123)Route(125-186)
⏰ 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). (4)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (10)
src/Subscription/Controller/SubscriberController.php (1)
172-176: Named arguments toupdateSubscriberlook correct and improve clarityUsing named parameters here lines up well with the updated
SubscriberManager::updateSubscriber(subscriber, subscriberDto, admin)signature and makes the call site self-documenting. The wiring fromMapEntity→$subscriber→getDto()→updateSubscriber()is consistent, given thatUpdateSubscriberRequest::getDto()now builds the DTO without an ID.Only thing to keep an eye on: if the parameter names in
SubscriberManager::updateSubscriberever change, this call will need to be updated too, otherwise PHP will complain at runtime. Worth double-checking that the manager method currently uses exactly these parameter names.src/Messaging/Controller/TemplateController.php (2)
108-110: LGTM! Nice readability improvement.Switching to named arguments makes the call site more self-documenting. Clean refactor.
181-181: Good refactor – centralizes the schema definition.Using a reference to
CreateTemplateRequestkeeps the schema in one place and makes it easier to maintain.src/Messaging/Controller/CampaignController.php (2)
137-152: 404 response in OpenAPI forGET /campaigns/{messageId}looks goodThe new
404 NotFoundErrorResponsein the docs matches the controller’s not‑found behavior and makes the API surface clearer for clients. 👍Please regenerate your OpenAPI/Swagger spec and confirm the new 404 response appears as expected in the rendered docs.
271-292: 404 response in OpenAPI forPUT /campaigns/{messageId}is consistent and usefulAdding
404 NotFoundErrorResponsehere aligns the docs with the new not‑found guard inupdateMessageand withgetMessage, which should help client implementations handle missing campaigns cleanly.After updating, verify that your generated OpenAPI spec (and any client SDKs) pick up the new 404 response.
src/Identity/Controller/AdministratorController.php (2)
160-198: adminId rename is consistent across route, docs, and entity mapping
adminIdis used consistently in the route placeholder, OA path, OA\Parameter, andMapEntitymapping, so this refactor should be transparent behavior‑wise and avoids the oldadministratorId/adminIdmismatch.
209-252: Update endpoint param rename also looks cleanFor the update route,
adminIdis wired through the Symfony route, OpenAPI path/parameter, andMapEntitymapping consistently, so the change keeps the behavior the same while standardizing the name.src/Subscription/Controller/ListMembersController.php (1)
115-120: Named arguments forgetPaginatedListlook goodUsing named args here (
request,normalizer,className,filter) makes the call clearer and resilient to future parameter reordering inPaginatedDataProvider::getPaginatedList. No issues from my side.src/Identity/Controller/AdminAttributeValueController.php (2)
25-55: Route nesting for admin attributes looks consistentUsing the controller prefix
/administratorsplus/{adminId}/attributes/{definitionId}and matching OA path/api/v2/administrators/{adminId}/attributes/{definitionId}keeps this in line with other controllers (e.g. templates/lists) and with the updated tests. No functional issues spotted here.
206-288: List endpoint refactor and named arguments look goodThe move to
/{adminId}/attributesplus OA path/api/v2/administrators/{adminId}/attributesaligns with the rest of the admin-attributes surface, and the added 404 response matches theif (!$admin)guard. Using named arguments forgetPaginatedListwith anAdminAttributeValueFilterkeyed byadminIdis clear and consistent with other controllers (e.g. list members/templates).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Messaging/Request/CreateTemplateRequest.php (1)
65-76: Handlefile_get_contentsfailure explicitly.
file_get_contents()returnsstring|false, but line 71 passes the result directly to the DTO which expectsstring|null. If the file read fails,falsewill be passed instead ofnull.Apply this diff to handle the failure case:
return new CreateTemplateDto( title: $this->title, content: $this->content, text: $this->text, - fileContent: $this->file instanceof UploadedFile ? file_get_contents($this->file->getPathname()) : null, + fileContent: $this->file instanceof UploadedFile + ? (file_get_contents($this->file->getPathname()) ?: null) + : null, shouldCheckLinks: $this->checkLinks, shouldCheckImages: $this->checkImages, shouldCheckExternalImages: $this->checkExternalImages, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Messaging/Request/CreateTemplateRequest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Messaging/Request/CreateTemplateRequest.php (2)
src/Messaging/Request/Message/MessageMetadataRequest.php (1)
OA(12-33)src/Messaging/Serializer/BounceRegexNormalizer.php (1)
OA(11-57)
⏰ 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). (5)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: Checkout phpList rest-api and generate docs specification (OpenAPI latest-restapi.json)
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist latest [Build, Test]
| #[OA\Schema( | ||
| schema: 'CreateTemplateRequest', | ||
| required: ['title'], | ||
| properties: [ | ||
| new OA\Property(property: 'title', type: 'string', example: 'Newsletter Template'), | ||
| new OA\Property(property: 'content', type: 'string', example: '<html><body>[CONTENT]</body></html>'), | ||
| new OA\Property(property: 'text', type: 'string', example: '[CONTENT]'), | ||
| new OA\Property( | ||
| property: 'file', | ||
| description: 'Optional file upload for HTML content', | ||
| type: 'string', | ||
| format: 'binary' | ||
| ), | ||
| new OA\Property( | ||
| property: 'check_links', | ||
| description: 'Check that all links have full URLs', | ||
| type: 'boolean', | ||
| example: true | ||
| ), | ||
| new OA\Property( | ||
| property: 'check_images', | ||
| description: 'Check that all images have full URLs', | ||
| type: 'boolean', | ||
| example: false | ||
| ), | ||
| new OA\Property( | ||
| property: 'check_external_images', | ||
| description: 'Check that all external images exist', | ||
| type: 'boolean', | ||
| example: true | ||
| ), | ||
| ], | ||
| type: 'object' | ||
| )] |
There was a problem hiding this comment.
Clarify whether content is required or optional.
The content property (line 55) is declared as non-nullable string but isn't marked as required in the OpenAPI schema nor validated with NotBlank or NotNull. This creates ambiguity:
- If
contentis required, add it to therequiredarray and/or add validation constraints - If
contentis optional, declare it as?stringto match the OpenAPI schema
Apply this diff if content should be required:
#[OA\Schema(
schema: 'CreateTemplateRequest',
- required: ['title'],
+ required: ['title', 'content'],
properties: [Or this diff if content should be optional:
- #[ContainsPlaceholder]
- public string $content;
+ #[ContainsPlaceholder]
+ public ?string $content = null;And update line 69 in getDto() accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Messaging/Request/CreateTemplateRequest.php around lines 14-47, the OA
Schema declares content as a string but its required status is ambiguous; pick
one approach and apply it: if content should be required, add 'content' to the
schema's required array, add the appropriate Symfony validation annotation
(NotBlank or NotNull) to the content property, and ensure getDto() (line ~69)
returns a non-null string; if content should be optional, change the property
declaration to ?string, update any validation annotations to allow null, and
update getDto() (line ~69) to return ?string accordingly so the PHP type and
OpenAPI schema stay consistent.
Summary by CodeRabbit
API Changes
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.
Thanks for contributing to phpList!