Refactor/subscriber attribute routes #162
Conversation
|
Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 13.64% 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/subscriber attribute routes' accurately captures the main change—updating API routes for subscriber attributes from a flat structure to a RESTful nested structure. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
refactor/subscriber-attribute-routes
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
⏰ 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 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 latest [Build, Test]
- GitHub Check: phpList Base Dist on PHP 8.1, with dist oldest [Build, Test]
🔇 Additional comments (1)
.coderabbit.yaml (1)
18-20: Verify the newchecks.docstring_coverageconfiguration structure.The new
checks.docstring_coverage.enabledsection being added doesn't align with the provided CodeRabbit configuration schema, which defines docstring coverage checks underreviews.pre_merge_checks.docstring_coverageinstead. This introduces a parallel config path that may:
- Conflict with or confuse the existing pre-merge checks structure
- Go unrecognized by the config validator if the schema hasn't been updated
Could you clarify whether this should be placed at the root
checkslevel (as a new feature) or if it should be consolidated underreviews.pre_merge_checks.docstring_coverage? Also, please confirm the configuration schema has been updated to accommodate the newcheckssection at the root level.
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Subscription/Controller/SubscriberAttributeValueController.php (1)
282-348: Fix entity type mappings ingetAttributeDefinitionto match other controller methodsThe
subscriberIdroute parameter is incorrectly mapped toSubscriberAttributeDefinitioninstead ofSubscriber. Three other methods in the same controller (createOrUpdate,delete,getPaginated) correctly mapsubscriberIdto?Subscriber. ThegetAttributeDefinitionmethod should follow the same pattern:public function getAttributeDefinition( Request $request, - #[MapEntity(mapping: ['subscriberId' => 'id'])] ?SubscriberAttributeDefinition $subscriber, - #[MapEntity(mapping: ['definitionId' => 'id'])] ?SubscriberAttributeDefinition $definition, + #[MapEntity(mapping: ['subscriberId' => 'id'])] ?Subscriber $subscriber = null, + #[MapEntity(mapping: ['definitionId' => 'id'])] ?SubscriberAttributeDefinition $definition = null, ): JsonResponse {
🧹 Nitpick comments (2)
src/Subscription/Controller/SubscriberAttributeValueController.php (2)
133-201: Delete endpoint docs don’t match actual HTTP status
delete()returnsResponse::HTTP_NO_CONTENT(204), but the OAresponsesstill document a 200 success. This can confuse API consumers and generated clients.Consider updating the OA annotation to match the implementation, e.g.:
- responses: [ - new OA\Response( - response: 200, - description: 'Success' - ), + responses: [ + new OA\Response( + response: 204, + description: 'No Content' + ),
203-280: Guard against null$subscriberingetPaginated
getPaginated()calls$subscriber->getId()without checking for null. IfMapEntityever yieldsnullfor an unknownsubscriberId, this will throw instead of returning a 404 like the other endpoints.To make behavior consistent, consider:
public function getPaginated( Request $request, #[MapEntity(mapping: ['subscriberId' => 'id'])] ?Subscriber $subscriber = null, ): JsonResponse { $this->requireAuthentication($request); - $filter = (new SubscriberAttributeValueFilter())->setSubscriberId($subscriber->getId()); + if (!$subscriber) { + throw $this->createNotFoundException('Subscriber not found.'); + } + + $filter = (new SubscriberAttributeValueFilter())->setSubscriberId($subscriber->getId());
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)src/Subscription/Controller/SubscriberAttributeValueController.php(5 hunks)tests/Integration/Subscription/Controller/SubscriberAttributeValueControllerTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Subscription/Controller/SubscriberAttributeValueController.php (2)
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/Subscription/OpenApi/SwaggerSchemasResponse.php (1)
OA(9-171)
⏰ 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 (3)
.github/PULL_REQUEST_TEMPLATE.md (1)
1-1: Template marker change looks goodUnquoting
@coderabbitai summaryso it stands alone in the PR body is fine and keeps the template simple.tests/Integration/Subscription/Controller/SubscriberAttributeValueControllerTest.php (1)
34-86: Tests correctly aligned with new nested attribute routesUpdated test URLs under
/api/v2/subscribers/{subscriberId}/attributesmatch the controller routes and keep existing assertions intact.src/Subscription/Controller/SubscriberAttributeValueController.php (1)
25-57: Route and OA path refactor to/subscribers/{subscriberId}/attributes/{definitionId}looks consistentThe new base route and the create/update endpoint (including OA
path) line up cleanly with the nested structure and the updated tests; no functional issues stand out here.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Identity/Serializer/AdministratorNormalizer.php (1)
45-46: Update OpenAPI schema to match implementation changes.The code changes are sound, but the OpenAPI schema documentation (lines 12-27) is now out of sync:
- Line 20 documents
'super_user'but code returns'super_admin'- The new
'privileges'field is completely missing from the schemaUpdate the schema to document both
'super_admin'(replacing'super_user') and add documentation for'privileges'as an object containing privilege flags (e.g.,subscribers,campaigns,statistics) with boolean values.src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
7-37: Fix OA enum values and type annotation to match validator schemaTwo concrete issues confirmed:
Line 32:
type: 'number'should betype: 'integer'for the?int $orderfield (matching the pattern used inSubscriberAttributeDefinitionRequest).Lines 24–31: The OA
enumlists enum objects, but the validator expects string values. TheAssert\Choice(choices: ['hidden', 'textline'])constraint (line 44) validates against strings, so the OpenAPI schema should document strings to stay in sync:new OA\Property( property: 'type', type: 'string', enum: [ - AttributeTypeEnum::TextLine, - AttributeTypeEnum::Hidden, + 'textline', + 'hidden', ], - example: 'hidden', + example: 'hidden', nullable: true ), - new OA\Property(property: 'order', type: 'number', example: 12), + new OA\Property(property: 'order', type: 'integer', example: 12),
🧹 Nitpick comments (4)
src/Messaging/Serializer/TemplateImageNormalizer.php (1)
7-31: Nice work adding OpenAPI documentation!The schema accurately describes the TemplateImage structure and will help API consumers. A couple of optional refinements to consider for perfect schema-to-code alignment:
- Line 15:
template_idcan be null (line 45 uses?->) but the schema doesn't includenullable: true- Lines 19-25:
datais markednullable: true, but line 48 showsbase64_encode($object->getData() ?? '')always returns a string (empty string at minimum), never nullIf you'd like to make the schema precisely match the runtime output:
- new OA\Property(property: 'template_id', type: 'integer', example: 1), + new OA\Property(property: 'template_id', type: 'integer', example: 1, nullable: true),new OA\Property( property: 'data', description: 'Base64-encoded image data', type: 'string', format: 'byte', example: 'iVBORw0KGgoAAAANSUhEUgAAA...', - nullable: true ),src/Subscription/Serializer/SubscriberAttributeValueNormalizer.php (1)
11-18: Add explicittype: 'object'declaration for consistency.The schema annotation looks solid and matches the normalizer output structure. However, for consistency with other normalizers in the codebase (like
AdminAttributeValueNormalizerandAdminAttributeDefinitionNormalizer), consider explicitly declaringtype: 'object'after the properties array.Apply this diff:
#[OA\Schema( schema: 'SubscriberAttributeValue', properties: [ new OA\Property(property: 'subscriber', ref: '#/components/schemas/Subscriber'), new OA\Property(property: 'definition', ref: '#/components/schemas/AttributeDefinition'), new OA\Property(property: 'value', type: 'string', example: 'United States'), ], + type: 'object' )]src/Messaging/Request/Message/MessageOptionsRequest.php (1)
11-21: Consider marking optional properties as nullable in the OpenAPI schema.The schema defines
to_field,reply_to, anduser_selectionas plain strings, but the actual properties at lines 29, 32, and 34 are nullable (string,?string,?stringrespectively, withto_fieldmissing the?but seemingly optional). Marking these asnullable: truein the OpenAPI schema would better align with the implementation and clarify for API consumers which fields can be omitted or null.Apply this pattern for optional fields:
- new OA\Property(property: 'to_field', type: 'string', example: '[email protected]'), - new OA\Property(property: 'reply_to', type: 'string', example: '[email protected]'), - new OA\Property(property: 'user_selection', type: 'string', example: 'all-active-users'), + new OA\Property(property: 'to_field', type: 'string', example: '[email protected]', nullable: true), + new OA\Property(property: 'reply_to', type: 'string', example: '[email protected]', nullable: true), + new OA\Property(property: 'user_selection', type: 'string', example: 'all-active-users', nullable: true),src/Subscription/Serializer/SubscriberOnlyNormalizer.php (1)
7-30: SubscriberOnly schema is consistent; optional email format tweakThe
SubscriberOnlyOA schema matches thenormalize()output field‑for‑field and type‑for‑type, which is great. If you want to be extra explicit in the docs, you could tagformat: 'email', but that’s purely optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/Identity/OpenApi/SwaggerSchemasRequest.php(0 hunks)src/Identity/OpenApi/SwaggerSchemasResponse.php(0 hunks)src/Identity/Request/AdminAttributeDefinitionRequest.php(2 hunks)src/Identity/Request/CreateAdministratorRequest.php(1 hunks)src/Identity/Request/UpdateAdministratorRequest.php(1 hunks)src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php(1 hunks)src/Identity/Serializer/AdminAttributeValueNormalizer.php(1 hunks)src/Identity/Serializer/AdministratorNormalizer.php(1 hunks)src/Messaging/OpenApi/SwaggerSchemasRequest.php(0 hunks)src/Messaging/OpenApi/SwaggerSchemasResponse.php(0 hunks)src/Messaging/Request/Message/MessageContentRequest.php(1 hunks)src/Messaging/Request/Message/MessageFormatRequest.php(1 hunks)src/Messaging/Request/Message/MessageMetadataRequest.php(1 hunks)src/Messaging/Request/Message/MessageOptionsRequest.php(1 hunks)src/Messaging/Request/Message/MessageScheduleRequest.php(1 hunks)src/Messaging/Serializer/BounceRegexNormalizer.php(1 hunks)src/Messaging/Serializer/ListMessageNormalizer.php(1 hunks)src/Messaging/Serializer/MessageNormalizer.php(1 hunks)src/Messaging/Serializer/TemplateImageNormalizer.php(1 hunks)src/Messaging/Serializer/TemplateNormalizer.php(1 hunks)src/PhpListRestBundle.php(2 hunks)src/Statistics/OpenApi/SwaggerSchemasRequest.php(0 hunks)src/Statistics/OpenApi/SwaggerSchemasResponse.php(0 hunks)src/Statistics/Serializer/CampaignStatisticsNormalizer.php(1 hunks)src/Statistics/Serializer/TopDomainsNormalizer.php(1 hunks)src/Statistics/Serializer/TopLocalPartsNormalizer.php(1 hunks)src/Statistics/Serializer/ViewOpensStatisticsNormalizer.php(1 hunks)src/Subscription/OpenApi/SwaggerSchemasResponse.php(0 hunks)src/Subscription/Serializer/AttributeDefinitionNormalizer.php(1 hunks)src/Subscription/Serializer/SubscribePageNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberAttributeValueNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberHistoryNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberListNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberOnlyNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriptionNormalizer.php(1 hunks)
💤 Files with no reviewable changes (7)
- src/Statistics/OpenApi/SwaggerSchemasRequest.php
- src/Identity/OpenApi/SwaggerSchemasResponse.php
- src/Subscription/OpenApi/SwaggerSchemasResponse.php
- src/Identity/OpenApi/SwaggerSchemasRequest.php
- src/Statistics/OpenApi/SwaggerSchemasResponse.php
- src/Messaging/OpenApi/SwaggerSchemasRequest.php
- src/Messaging/OpenApi/SwaggerSchemasResponse.php
✅ Files skipped from review due to trivial changes (2)
- src/Subscription/Serializer/SubscribePageNormalizer.php
- src/Statistics/Serializer/TopDomainsNormalizer.php
🧰 Additional context used
🧬 Code graph analysis (26)
src/Statistics/Serializer/ViewOpensStatisticsNormalizer.php (1)
src/Statistics/Serializer/CampaignStatisticsNormalizer.php (1)
OA(11-80)
src/Subscription/Serializer/SubscriberOnlyNormalizer.php (4)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)src/Messaging/Serializer/BounceRegexNormalizer.php (1)
OA(11-57)src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
src/Messaging/Request/Message/MessageScheduleRequest.php (2)
src/Messaging/Request/Message/MessageContentRequest.php (1)
OA(11-45)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Messaging/Request/Message/MessageContentRequest.php (4)
src/Messaging/Request/Message/MessageFormatRequest.php (1)
OA(11-53)src/Messaging/Request/Message/MessageMetadataRequest.php (1)
OA(12-33)src/Messaging/Request/Message/MessageOptionsRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageScheduleRequest.php (1)
OA(11-58)
src/Messaging/Serializer/ListMessageNormalizer.php (2)
src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
src/Messaging/Serializer/TemplateNormalizer.php (2)
src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)src/Messaging/Serializer/TemplateImageNormalizer.php (1)
OA(11-61)
src/Statistics/Serializer/CampaignStatisticsNormalizer.php (3)
src/Statistics/Serializer/TopDomainsNormalizer.php (1)
OA(10-61)src/Statistics/Serializer/TopLocalPartsNormalizer.php (1)
OA(10-63)src/Statistics/Serializer/ViewOpensStatisticsNormalizer.php (1)
OA(11-73)
src/Identity/Serializer/AdminAttributeValueNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)
src/Identity/Request/CreateAdministratorRequest.php (2)
src/Identity/Request/UpdateAdministratorRequest.php (1)
OA(15-100)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)
src/Subscription/Serializer/SubscriberHistoryNormalizer.php (3)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)src/Messaging/Serializer/ListMessageNormalizer.php (1)
OA(12-65)
src/Subscription/Serializer/SubscriberListNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Messaging/Serializer/ListMessageNormalizer.php (1)
OA(12-65)
src/Messaging/Serializer/MessageNormalizer.php (6)
src/Messaging/Request/Message/MessageContentRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageFormatRequest.php (1)
OA(11-53)src/Messaging/Request/Message/MessageMetadataRequest.php (1)
OA(12-33)src/Messaging/Request/Message/MessageOptionsRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageScheduleRequest.php (1)
OA(11-58)src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
src/Subscription/Serializer/SubscriptionNormalizer.php (3)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Messaging/Serializer/ListMessageNormalizer.php (1)
OA(12-65)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Identity/Serializer/AdministratorNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)
src/Messaging/Serializer/BounceRegexNormalizer.php (4)
src/Messaging/Serializer/ListMessageNormalizer.php (1)
OA(12-65)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)src/Messaging/Serializer/TemplateImageNormalizer.php (1)
OA(11-61)src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
src/Messaging/Serializer/TemplateImageNormalizer.php (1)
src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
src/Messaging/Request/Message/MessageMetadataRequest.php (5)
src/Messaging/Request/Message/MessageContentRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageFormatRequest.php (1)
OA(11-53)src/Messaging/Request/Message/MessageOptionsRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageScheduleRequest.php (1)
OA(11-58)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Statistics/Serializer/TopLocalPartsNormalizer.php (3)
src/Statistics/Serializer/CampaignStatisticsNormalizer.php (1)
OA(11-80)src/Statistics/Serializer/TopDomainsNormalizer.php (1)
OA(10-61)src/Statistics/Serializer/ViewOpensStatisticsNormalizer.php (1)
OA(11-73)
src/Subscription/Serializer/SubscriberAttributeValueNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)
src/Identity/Request/UpdateAdministratorRequest.php (2)
src/Identity/Request/CreateAdministratorRequest.php (1)
OA(15-102)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (2)
src/Identity/Request/AdminAttributeDefinitionRequest.php (1)
OA(17-80)src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)
src/Messaging/Request/Message/MessageOptionsRequest.php (5)
src/Messaging/Request/Message/MessageContentRequest.php (1)
OA(11-45)src/Messaging/Request/Message/MessageFormatRequest.php (1)
OA(11-53)src/Messaging/Request/Message/MessageMetadataRequest.php (1)
OA(12-33)src/Messaging/Request/Message/MessageScheduleRequest.php (1)
OA(11-58)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)
src/Messaging/Request/Message/MessageFormatRequest.php (2)
src/Messaging/Request/Message/MessageContentRequest.php (1)
OA(11-45)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Subscription/Serializer/SubscriberNormalizer.php (3)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-58)src/Messaging/Serializer/MessageNormalizer.php (1)
OA(11-148)
src/Identity/Request/AdminAttributeDefinitionRequest.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-52)src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
OA(11-79)
⏰ 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 (20)
src/Subscription/Serializer/SubscriberHistoryNormalizer.php (1)
7-7: LGTM!The OpenAPI attributes import is standard and necessary for the schema annotations added below.
src/Messaging/Serializer/TemplateNormalizer.php (1)
7-29: LGTM! Clean OpenAPI schema addition.The Template schema accurately describes the normalized output, including proper nullable flags and the correct reference to the TemplateImage schema. This pairs nicely with the TemplateImageNormalizer annotations.
src/Subscription/Serializer/SubscriberAttributeValueNormalizer.php (1)
7-7: Import looks good!The OpenAPI attributes import follows the pattern used in other normalizers throughout the codebase.
src/Subscription/Serializer/AttributeDefinitionNormalizer.php (2)
7-7: LGTM! Import looks good.The OpenAPI attributes import is necessary for the schema annotations below.
30-38: LGTM! DynamicListAttrOption schema is accurate.The schema properties correctly match the options normalization logic (lines 52-58).
src/Identity/Serializer/AdministratorNormalizer.php (1)
47-47: Nice addition with proper formatting.The ATOM format provides ISO 8601 compliance, and the null-safe operator handles cases where creation dates might be missing. Schema documentation correctly reflects this field.
src/Identity/Request/UpdateAdministratorRequest.php (1)
7-7: LGTM!The OpenAPI attributes import is correctly added and necessary for the schema annotations below.
src/Messaging/Request/Message/MessageMetadataRequest.php (1)
7-18: LGTM! OpenAPI documentation added.The OpenAPI schema annotation appropriately documents the request structure. No runtime logic changes.
src/Statistics/Serializer/CampaignStatisticsNormalizer.php (1)
7-26: LGTM! OpenAPI schema added for campaign statistics.The schema accurately documents all normalized fields from the
normalizeCampaignmethod. No functional changes.src/Messaging/Request/Message/MessageContentRequest.php (1)
7-21: LGTM! Request schema documented.OpenAPI metadata correctly reflects the required fields and validation constraints. No runtime changes.
src/Statistics/Serializer/ViewOpensStatisticsNormalizer.php (1)
7-22: LGTM! Schema added for view opens statistics.The OpenAPI annotation accurately documents the normalized structure. No behavioral changes.
src/Messaging/Serializer/BounceRegexNormalizer.php (1)
7-25: LGTM! BounceRegex schema documented.The OpenAPI annotation comprehensively documents all normalized properties with appropriate nullability. No runtime changes.
src/Messaging/Serializer/MessageNormalizer.php (1)
7-85: LGTM! Comprehensive Message schema added.The OpenAPI schema thoroughly documents the complex nested structure of normalized messages. Well-organized and consistent with the normalization logic.
src/Subscription/Serializer/SubscriberListNormalizer.php (1)
7-29: LGTM! SubscriberList schema documented.The OpenAPI annotation appropriately documents the subscriber list structure with all properties and examples. No functional changes.
src/PhpListRestBundle.php (1)
32-81: LGTM!The DetailedDomainStats schema is well-structured with clear property definitions for domain statistics. The nested object structure properly represents the confirmed/unconfirmed/blacklisted metrics.
src/Statistics/Serializer/TopLocalPartsNormalizer.php (1)
10-29: LGTM!The LocalPartsStats schema accurately describes the normalized output structure. The property definitions match what the
normalize()method returns.src/Messaging/Request/Message/MessageFormatRequest.php (1)
11-30: LGTM!The MessageFormatRequest schema is well-defined with proper enum constraints that match the validation annotations. The schema accurately represents the request structure.
src/Subscription/Serializer/SubscriberNormalizer.php (1)
12-36: LGTM!The Subscriber schema is comprehensive and accurately reflects the normalized output structure. The use of schema references for nested objects (SubscriberList) is appropriate.
src/Subscription/Serializer/SubscriptionNormalizer.php (1)
7-24: Subscription schema matches normalized payloadThe
SubscriptionOA schema lines up cleanly with thenormalize()output (subscriber,subscriber_list,subscription_datewith ISO 8601 formatting). Nothing to change here.src/Identity/Request/CreateAdministratorRequest.php (1)
7-7: Nice addition of OpenAPI documentation!The schema is comprehensive and well-structured, clearly documenting all request fields with appropriate types, constraints, and examples. This will help API consumers understand the expected request format.
Also applies to: 15-59
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Messaging/Serializer/ListMessageNormalizer.php (1)
12-32: Nice work addressing the previous review feedback!The
type: 'object'parameter is now included on line 31, which resolves the earlier concern.One small nitpick: The date-time examples use the 'Z' timezone format (
2022-12-01T10:00:00Z), but the actualnormalize()method on lines 54-55 uses'Y-m-d\TH:i:sP', which produces the+00:00timezone format instead (e.g.,2022-12-01T10:00:00+00:00). Both are valid ISO 8601, but for consistency with the code's actual output, consider updating the examples to match.Optional adjustment if you want the examples to match actual output:
new OA\Property( property: 'created_at', type: 'string', format: 'date-time', - example: '2022-12-01T10:00:00Z' + example: '2022-12-01T10:00:00+00:00' ), new OA\Property( property: 'updated_at', type: 'string', format: 'date-time', - example: '2022-12-01T10:00:00Z' + example: '2022-12-01T10:00:00+00:00' ),src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
11-22: Previous concern resolved; consider markingtypeas nullable.The previous review comment about the missing
table_nameproperty has been addressed—table_namewas removed from the normalized output, and the schema now accurately matches whatnormalize()returns.For consistency with the subscriber-facing
AttributeDefinitionNormalizer(which markstypeasnullable: true), consider marking thetypeproperty as nullable here as well:- new OA\Property(property: 'type', type: 'string', example: 'hidden'), + new OA\Property(property: 'type', type: 'string', example: 'hidden', nullable: true),This ensures the schema correctly reflects cases where
getType()might return null.Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Identity/Request/UpdateAdministratorRequest.php(3 hunks)src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php(1 hunks)src/Identity/Serializer/AdminAttributeValueNormalizer.php(1 hunks)src/Identity/Serializer/AdministratorNormalizer.php(2 hunks)src/Messaging/Request/Message/MessageScheduleRequest.php(1 hunks)src/Messaging/Serializer/ListMessageNormalizer.php(1 hunks)src/Subscription/Serializer/AttributeDefinitionNormalizer.php(1 hunks)src/Subscription/Serializer/SubscriberHistoryNormalizer.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Messaging/Request/Message/MessageScheduleRequest.php
- src/Subscription/Serializer/AttributeDefinitionNormalizer.php
🧰 Additional context used
🧬 Code graph analysis (6)
src/Identity/Serializer/AdminAttributeValueNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-51)src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-59)
src/Subscription/Serializer/SubscriberHistoryNormalizer.php (2)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
OA(11-51)src/Messaging/Serializer/ListMessageNormalizer.php (1)
OA(12-66)
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (2)
src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)src/Subscription/Serializer/AttributeDefinitionNormalizer.php (1)
OA(11-78)
src/Identity/Serializer/AdministratorNormalizer.php (3)
src/Identity/Request/UpdateAdministratorRequest.php (1)
OA(15-100)src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
OA(11-51)src/Identity/Request/CreateAdministratorRequest.php (1)
OA(15-102)
src/Identity/Request/UpdateAdministratorRequest.php (2)
src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-59)src/Identity/Request/CreateAdministratorRequest.php (1)
OA(15-102)
src/Messaging/Serializer/ListMessageNormalizer.php (2)
src/Identity/Serializer/AdministratorNormalizer.php (1)
OA(13-59)src/Messaging/Serializer/TemplateNormalizer.php (1)
OA(12-64)
🔇 Additional comments (7)
src/Identity/Serializer/AdminAttributeValueNormalizer.php (1)
7-7: LGTM! Past issue resolved.The OpenAPI schema annotation looks good. The previous concern about an incorrect schema reference on line 15 has been addressed—it now correctly points to
#/components/schemas/AdminAttributeDefinition. The schema properties accurately reflect the structure returned by thenormalize()method, and both schema references (AdministratorandAdminAttributeDefinition) are valid.Also applies to: 11-19
src/Identity/Serializer/AdminAttributeDefinitionNormalizer.php (1)
7-7: Import looks good.Standard OpenAPI attributes import, properly used in the schema annotation below.
src/Subscription/Serializer/SubscriberHistoryNormalizer.php (2)
7-27: OA schema annotation matches the normalizer output nicelyThe
SubscriberHistoryschema looks aligned with the normalized payload: field names, types, and thecreated_atdate-time format all match whatnormalize()returns and are consistent with the other normalizers’ OA metadata. No changes needed here.
43-43:summarykey now consistent with the documented schemaThe normalization array key has been corrected to
summary, matching the OA\Schema definition and avoiding the previous mismatch that would confuse clients. All good here.src/Identity/Serializer/AdministratorNormalizer.php (1)
13-29: LGTM! Past review issues resolved.The OpenAPI schema now correctly matches the normalized output:
- Line 19's
super_userproperty aligns with line 46's output key- Line 20's
privilegesproperty aligns with line 47's outputThe previous inconsistencies have been addressed.
Also applies to: 46-47
src/Identity/Request/UpdateAdministratorRequest.php (2)
15-58: OpenAPI schema looks solid.The schema accurately reflects the request structure with proper types and examples. The
super_userproperty (line 40) correctly maps to the$superUserclass property (line 75) via the configuredCamelCaseToSnakeCaseNameConverter.
96-96: No issues found—code is correct.UpdateAdministratorRequest line 96 correctly passes
superAdmin: $this->superUserto the UpdateAdministratorDto constructor from the externalphplist/corepackage, which expects this parameter name. The parameter naming differs from CreateAdministratorDto'sisSuperUserby design of the Core package—both Request classes use the correct parameter names for their respective DTOs and will execute without errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Unit/Identity/Serializer/AdministratorNormalizerTest.php (1)
18-47: Expectation update tosuper_usermatches renamed normalization outputThe normalized payload now using
'super_user' => truecorrectly reflects theisSuperUser()source and the broader rename away from “super_admin”. Just make sure any API docs and consumers are updated to expectsuper_userinstead ofsuper_admin.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/Unit/Identity/Request/UpdateAdministratorRequestTest.php(1 hunks)tests/Unit/Identity/Serializer/AdminAttributeDefinitionNormalizerTest.php(0 hunks)tests/Unit/Identity/Serializer/AdministratorNormalizerTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/Unit/Identity/Serializer/AdminAttributeDefinitionNormalizerTest.php
⏰ 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 (1)
tests/Unit/Identity/Request/UpdateAdministratorRequestTest.php (1)
15-40: Rename in test correctly exercises superUser → superAdmin mappingUsing
$request->superUser = true;while still asserting$dto->superAdminkeeps the test aligned with the new request API and verifies the mapping into the DTO. Assertions for the null case in the second test still look consistent with that contract.
Summary by CodeRabbit
Refactor
Documentation
Bug Fixes
Tests
Chores
Thanks for contributing to phpList!