Repository navigation
Conversation
|
Hello @boo-code! This is your first pull request on ps_apiresources repository of the PrestaShop project. Thank you, and welcome to this Open Source community! |
bca041e to
33905ef
Compare
| ), | ||
| ], | ||
| normalizationContext: ['skip_null_values' => false], | ||
| exceptionToStatus: [ |
There was a problem hiding this comment.
AddStateHandler throws the base StateException itself (invalid field values, wrapped PrestaShopException), and DeleteStateHandler throws DeleteStateException when an address uses the state (NY or FL in the demo data), so both answer 500 today. Could you map StateException to 422 after StateNotFoundException (it also covers StateConstraintException), and add a test deleting a state used by an address?
| #[ApiProperty(identifier: true)] | ||
| public int $stateId; | ||
|
|
||
| #[Assert\NotBlank(groups: ['Create'])] |
There was a problem hiding this comment.
Only presence is checked, so values the back-office StateType refuses reach the legacy model: isoCode: "ABCDEF" or name: "<b>" make AddStateHandler throw StateException, and countryId: 0 or zoneId: 0 throw CountryConstraintException / ZoneException from the command, all answering 500. Could you add the form constraints in the Default group, as Country does (Length(max: 64) and TypedRegex(TYPE_GENERIC_NAME) on name, Length(max: 7) and TypedRegex(TYPE_STATE_ISO_CODE) on isoCode, Assert\Positive on countryId and zoneId)?
| { | ||
| $state = $this->createItem('/states', $this->getCreatePayload(), ['state_write']); | ||
|
|
||
| $this->assertArrayHasKey('stateId', $state); |
There was a problem hiding this comment.
Could you assert the whole POST response here, as ZoneEndpointTest::testAddZone does? An expectedState(int $stateId, array $overrides = []) helper reused by the add, get and update steps would also remove the duplicated expected arrays.
| $return = $this->deleteItem('/states/' . $stateId, ['state_write']); | ||
| $this->assertNull($return); | ||
|
|
||
| // Fetching a state that does not exist returns a 404 (StateNotFoundException mapping). |
There was a problem hiding this comment.
Since daffd28, ApiTestCase::requestApi() clears the legacy Cache and the ObjectModel static cache before each request, so after a rebase the deleted state should answer 404 here (ZoneEndpointTest checks the deleted id too, not an unknown one). Could you GET '/states/' . $stateId and move the 999999 check to its own test?
| $this->getItem('/states/999999', ['state_read'], Response::HTTP_NOT_FOUND); | ||
| } | ||
|
|
||
| public function testInvalidState(): void |
There was a problem hiding this comment.
Only the blank name is covered; CountryEndpointTest on dev is a good model for the rest. Could you add the 404 on GET, PATCH and DELETE of an unknown id, /states/0 on each operation (422 through StateId on GET and PATCH, 404 through NoStateId on DELETE), one invalid case per field on POST and PATCH, and the delete of a state used by an address?
Expose the State CQRS (Add/Edit/Delete + GetStateForEditing) as an Admin API resource at /states, completing the geographic data model alongside the existing Country and Zone resources. Adds StateEndpointTest covering create/get/update/delete, protected-endpoint authentication and validation. Refs #39630.
…e back office, cover the error cases
33905ef to
8cd6edc
Compare
|
All done:
The new cases on the previous resource: 3 tests fail, all with 500. After the change: 30/30 on a 9.2.1 core. |
GET/POST/PATCH/DELETE/statesAdmin API endpoints, exposing the existingStateCQRS (Add/Edit/Deletecommands +GetStateForEditing) as an ApiPlatform resource. This completes the geographic data model exposed by the API, alongside the existingCountryandZoneresources.StateEndpointTest(composer run-module-tests). It covers create/get/update/delete, protected-endpoint authentication, unknown and0ids, the field constraints on create and update, and the delete of a state used by an address (422). Endpoints require a client with thestate_read/state_writescopes.Note
testDeleteStateasserts the204response and a404on a non-existent id rather than re-fetching the just-deleted state.State::delete()removes the row with a direct query without going throughparent::delete(), so theObjectModelcache is not cleared within the same PHP process — this is only observable in the single-process test runner (each real API request is a fresh process). This mirrors the existingZoneEndpointTestdelete convention.Verified locally against a 9.2.1 install —
StateEndpointTestpasses (30 tests, 236 assertions);php-cs-fixerand PHPStan are clean on the added files.