Skip to content

Add carrier endpoints#147

Open
luigimassa wants to merge 23 commits into
PrestaShop:devfrom
bwlab:add-carrier-endpoints
Open

Add carrier endpoints#147
luigimassa wants to merge 23 commits into
PrestaShop:devfrom
bwlab:add-carrier-endpoints

Conversation

@luigimassa
Copy link
Copy Markdown

Questions Answers
Description? Adds a new read-only REST API endpoint GET /carriers/{carrierId}/ranges that exposes carrier shipping ranges via the GetCarrierRanges CQRS query. The response contains the list of zones, each with its price ranges (from/to/price as decimal strings). Note: This endpoint is currently non-functional due to a known limitation in CarrierRangeRepository::assertShopConstraint() (core), which only accepts ShopConstraint::allShops() while the API framework always passes a shop-specific constraint. Until the core is fixed, the endpoint returns HTTP 422 with message "Shop constraint isn't supported yet." The resource definition and mapping are ready and will work automatically once the core limitation is resolved.
Type? new feature
BC breaks? no
Deprecations? no
Fixed ticket? N/A
Sponsor company bwlab (Luigi Massa)
How to test? 1. Install the module on a PrestaShop 9.x instance. 2. Create an API client with the carrier_read scope. 3. Obtain an OAuth2 Bearer token. 4. GET /admin-api/carriers/{id}/ranges (e.g. /carriers/2/ranges) — currently returns HTTP 422 due to core limitation in CarrierRangeRepository (see TODO.md). 5. GET /admin-api/carriers/999/ranges — should return HTTP 404.

@ps-jarvis
Copy link
Copy Markdown

Hello @luigimassa!

This is your first pull request on ps_apiresources repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Mar 9, 2026
Expose ToggleCarrierStatusCommand and ToggleCarrierIsFreeCommand via
PUT /carriers/{carrierId}/toggle-status and
PUT /carriers/{carrierId}/toggle-is-free endpoints with carrier_write scope.
Add integration tests for both toggle endpoints.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
luigimassa and others added 7 commits March 15, 2026 21:21
Add BulkCarriers API resource with PUT /carriers/bulk-set-status endpoint
that maps to BulkToggleCarrierStatusCommand, following the same pattern
used for taxes and zones bulk status toggle.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without this constraint, passing carrierId=0 causes a CarrierConstraintException
thrown during command construction (before the command bus), which bypasses
the exceptionToStatus mapping and returns 500 instead of a proper error.
The Assert\All([Assert\Positive()]) constraint catches invalid IDs during
Symfony validation phase and returns a 400 Bad Request with details.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd instantiation

CQRSCommand/CQRSUpdate automatically sets input to the CQRS command class,
causing CQRSApiNormalizer to instantiate BulkToggleCarrierStatusCommand
directly during the DeserializeListener phase. This bypasses the DTO
and passes raw string values from JSON to the command constructor, where
(int) cast on non-numeric strings produces 0, triggering CarrierConstraintException.

Setting input: BulkCarriers::class forces the deserialization to use the
DTO class, letting the CommandProcessor handle command instantiation
with proper type handling.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rmalizer flow

Removing input: BulkCarriers::class lets CQRSCommand auto-set input to
BulkToggleCarrierStatusCommand, so CQRSApiNormalizer validates BulkCarriers
constraints (Assert\Positive on carrierIds) before applying CQRSCommandMapping
([enabled] -> [expectedStatus]) and instantiating the command.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Non-numeric values (e.g. strings) in carrierIds array were bypassing
Assert\Positive and causing CarrierConstraintException during command
instantiation, resulting in HTTP 500 instead of 422.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ping

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add DELETE /carriers/{carrierId} endpoint
CarrierNotFoundException::class => Response::HTTP_NOT_FOUND,
],
)]
class CarrierList
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a test for this endpoint.

luigimassa and others added 4 commits March 30, 2026 07:43
Replace individual property assertions with a single assertEquals
on the entire carrier array, following the project test conventions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use property names instead of getter method names in QUERY_MAPPING,
and only map properties where names differ between QueryResult and API.
Remove TODO.md as it should not be committed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jolelievre jolelievre added the Need AI review Trigger: Request an AI pre-review from Claude label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

Claude Code — ps_apiresources

The conventions, architecture, and Do/Don't list for this repository
live in CONTEXT.md. Read it before suggesting or
generating any code.

When the user asks to add, expose, or wire up a new Admin API endpoint,
invoke the ps-api-endpoint
skill — it generates the resource class and integration test in line
with CONTEXT.md.

@github-actions
Copy link
Copy Markdown

🤖 Claude AI Pre-Review — Automated analysis. Does not replace human review.

📋 Summary of changes

This PR exposes the Carrier domain via the Admin API: GET /carriers/{carrierId}, GET /carriers (paginated list), GET /carriers/{carrierId}/ranges (sub-resource), DELETE /carriers/{carrierId}, two toggle operations (PUT .../toggle-status, .../toggle-is-free), and PUT /carriers/bulk-set-status. It also adds GET /addresses/required-fields backed by GetRequiredFieldsForAddress. The CarrierRanges endpoint is intentionally non-functional — it always returns 422 due to a known limitation in CarrierRangeRepository::assertShopConstraint() in Core.

⏱️ Estimated review time

30–40 minutes — multiple resource classes, field-mapping concerns, test coverage gaps, and a deliberately broken endpoint to evaluate.

🎯 Scope

  • Exposed operations: GET (single + list + sub-resource) / PUT (toggle × 2 + bulk) / DELETE
  • CQRS entity: GetCarrierForEditing, GetCarrierRanges, ToggleCarrierStatusCommand, ToggleCarrierIsFreeCommand, DeleteCarrierCommand, BulkToggleCarrierStatusCommand; + GetRequiredFieldsForAddress
  • Integration test: partial (missing delete, list, ranges, bulk)

@github-actions
Copy link
Copy Markdown

🤖 Claude AI Pre-Review — Automated analysis. Does not replace human review.

📋 Summary of changes

This PR exposes the Carrier domain via the Admin API: GET /carriers/{carrierId}, GET /carriers (paginated list), GET /carriers/{carrierId}/ranges (sub-resource), DELETE /carriers/{carrierId}, two toggle operations (PUT .../toggle-status, .../toggle-is-free), and PUT /carriers/bulk-set-status. It also adds GET /addresses/required-fields backed by GetRequiredFieldsForAddress. The CarrierRanges endpoint is intentionally non-functional — it always returns 422 due to a known limitation in CarrierRangeRepository::assertShopConstraint() in Core.

⏱️ Estimated review time

30–40 minutes — multiple resource classes, field-mapping concerns, test coverage gaps, and a deliberately broken endpoint to evaluate.

🎯 Scope

  • Exposed operations: GET (single + list + sub-resource) / PUT (toggle × 2 + bulk) / DELETE
  • CQRS entity: GetCarrierForEditing, GetCarrierRanges, ToggleCarrierStatusCommand, ToggleCarrierIsFreeCommand, DeleteCarrierCommand, BulkToggleCarrierStatusCommand; + GetRequiredFieldsForAddress
  • Integration test: partial (missing delete, list, ranges, bulk)
🧱 API Platform / CQRS architecture compliance

🔴 BLOCKER — `$active` must be `$enabled` (Carrier.php, CarrierList.php)

CONTEXT.md rule: "Status uses enabled, not active / status". Both Carrier and CarrierList expose public bool $active. Rename to $enabled in both DTOs, add '[active]' => '[enabled]' to the ApiResourceMapping in CarrierList, and update all test assertions.

🔴 BLOCKER (CI-enforced) — `float` instead of `DecimalNumber`

Carrier.php: public float $maxWeight;

CONTEXT.md: "Decimals — use DecimalNumber, never float. This is checked by the CI." Must be public \PrestaShop\Decimal\DecimalNumber $maxWeight. Update the test assertion accordingly.

🔴 BLOCKER — `QUERY_MAPPING` direction inverted in `CarrierRanges.php`

The mapping currently reads:

'[carrierId]' => '[getCarrierId][getValue]' and '[zones]' => '[getZones]'

CONTEXT.md: "QUERY_MAPPING maps query result field -> API field". The API field names are used as keys and CQRS result accessors as values — the classic inversion pitfall. Correct form:

'[getCarrierId][getValue]' => '[carrierId]' and '[getZones]' => '[zones]'

The [_context][shopConstraint] entry is a context-injection directive and is correct as-is.

🔴 MAJOR CONCERN — Shipping a deliberately non-functional endpoint

CarrierRanges always returns HTTP 422 because CarrierRangeRepository::assertShopConstraint() only accepts ShopConstraint::allShops(). Merging a broken endpoint into main is inadvisable: it pollutes the API contract, misleads consumers, and the CarrierConstraintException -> 422 mapping silences what is effectively a runtime error. Recommend holding this until the Core is fixed.

🟠 Multi-shop — `$associatedShopIds` should be `$shopIds`

CONTEXT.md: "Entities with shop association expose a public array $shopIds property on the DTO." Carrier.php uses $associatedShopIds. Rename to $shopIds, update the QUERY_MAPPING entry, and update the test assertion.

🟠 Property naming — `$idTaxRuleGroup` should be `$taxRuleGroupId`

CONTEXT.md convention: "domain name + Id". The legacy DB prefix id_ should not appear in the API DTO. Rename to $taxRuleGroupId and update the test assertion.

🟡 `BulkCarriers` — `$carrierIds` absent from `CQRSCommandMapping`

Only '[enabled]' => '[expectedStatus]' is mapped. $carrierIds is not listed. If BulkToggleCarrierStatusCommand's constructor param is named exactly $carrierIds, the framework may wire it automatically; otherwise IDs are silently dropped. Add '[carrierIds]' => '[carrierIds]' explicitly for safety.

🟡 `AddressRequiredFields` — no identifier; consider `CQRSGetCollection`

No identifier: true property and no {id} segment in the URI. CQRSGet is designed for single-entity lookup by ID; a parameterless query returning an array of strings fits CQRSGetCollection better. Verify this works correctly at runtime.

🟢 URI conventions

All URIs are plural, lowercase, kebab-case. bulk-set-status uses the bulk- prefix; carrierIds uses the required plural domain + Ids form. /carriers/{carrierId}/ranges correctly nests under the parent.

🟢 Scopes

carrier_read / carrier_write follow {entity_snake_case}_{action}. Every operation declares at least one scope.

🟢 Exception mapping

CarrierNotFoundException -> 404 and CarrierConstraintException -> 422 are correct. No HTTP_BAD_REQUEST (400) misuse.

🟡 `TODO.md` in Italian

The project is English-language. Content belongs in a GitHub issue; if the file is kept, translate to English.

💡 Improvement suggestions
  1. CarrierList.delay field alignment — Verify the carrier grid query builder selects a column named exactly delay. If it returns a language-joined alias, an ApiResourceMapping entry is needed.

  2. BulkCarriers.$enabled validation$enabled has no Assert\NotNull. If a client omits the field, the property will be uninitialised. Add NotNull to make the contract explicit.

  3. Carrier.php QUERY_MAPPING completeness — Only localizedDelay and the shop constraint are explicitly mapped. Verify isFree, hasAdditionalHandlingFee, ordersCount, and shopIds (once renamed) in GetCarrierForEditing's result DTO use the exact camelCase names as the DTO properties.

  4. testGetAddressRequiredFields weak assertion — The if (!empty(...)) guard means the test silently passes when requiredFields is empty. Assert at least one known required field unconditionally.

✅ Pre-review checklist

URI & routing

  • URI is plural, lowercase, kebab-case
  • Identifier uses domain name + Id suffix — $idTaxRuleGroup should be $taxRuleGroupId
  • Sub-resources follow parent path (/carriers/{carrierId}/ranges)
  • Bulk operation URI uses bulk- prefix and plural Ids parameter

Operations & scopes

  • Correct operation attribute per HTTP method
  • Scope format: carrier_read / carrier_write, singular form

API Resource properties

  • All properties strictly typed — public float $maxWeight violates CI rule (must be DecimalNumber)
  • Naming conventions — $active must be $enabled (Carrier.php + CarrierList.php); $idTaxRuleGroup should be $taxRuleGroupId
  • identifier: true on ID property (present in all four resource classes)
  • LocalizedValue on $delay in Carrier.php; list endpoint correctly uses plain ?string

CQRS mapping

  • QUERY_MAPPING direction — inverted for carrierId/zones in CarrierRanges.php
  • CQRSCommandMapping direction — enabled -> expectedStatus correct
  • CQRSQuery on create/partial-update — n/a; toggle ops correctly use output: false
  • No SerializedName — mappings only

Forbidden practices (CI-enforced)

  • No custom normalizers or processors
  • No Value Objects — float is forbidden; must be DecimalNumber

Exception handling & validation

  • ConstraintException -> 422, NotFoundException -> 404
  • No HTTP_BAD_REQUEST (400) misuse

Multi-shop

  • shopIds present and mapped — entity has shop association but exposes $associatedShopIds instead of $shopIds

Listing field alignment (CarrierList)

  • DTO properties match grid fields — delay, logo, active column names need verification against the carrier grid SQL SELECT
  • ApiResourceMapping covers id_carrier -> carrierId and is_free -> isFree
  • filtersMapping covers carrierId and isFree differences
  • No orphan DTO property — delay and logo need grid query builder verification

Integration test

  • Asserts all fields — testGetCarrier is thorough but no tests for list, ranges, or bulk
  • testInvalid* with assertValidationErrors — absent
  • getProtectedEndpoints() complete — missing DELETE /carriers/{carrierId}, GET /carriers, GET /carriers/{carrierId}/ranges, PUT /carriers/bulk-set-status
  • DatabaseDump::restoreTables() covers all affected tables
  • declare(strict_types=1) present

@github-actions github-actions Bot added AI reviewed Status: Claude AI has already pre-reviewed this PR and removed Need AI review Trigger: Request an AI pre-review from Claude labels Apr 23, 2026
@nicosomb
Copy link
Copy Markdown
Contributor

nicosomb commented May 7, 2026

@luigimassa CI is red, could you have a look please?

@ps-jarvis ps-jarvis moved this from Ready for review to Waiting for author in PR Dashboard May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Admin API Contributions AI reviewed Status: Claude AI has already pre-reviewed this PR Waiting for author

Projects

Status: Waiting for author

Development

Successfully merging this pull request may close these issues.

6 participants