Merge the Country endpoints into one domain PR

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Consolidates the three pending Country PRs into one domain PR, with the status and zone tests moved off raw SQL and off the fixture countries
Type?	new feature
BC breaks?	no
Deprecations?	no
Fixed ticket?	Related to PrestaShop/PrestaShop#39630
How to test?	See below
Sponsor company	PrestaEdit

What this PR does

Merges #217, #368 and #394 into one PR, following the mutualisation done on the Product domain in #410.

Endpoints added

Method	URI	CQRS	Scope
GET	/countries/{countryId}/required-fields	GetCountryRequiredFields	country_read
PUT	/countries/{countryId}/toggle-status	ToggleCountryStatusCommand	country_write
PUT	/countries/bulk-toggle-status	BulkToggleCountriesStatusCommand	country_write
PUT	/countries/bulk-update-zone	BulkUpdateCountriesZoneCommand	country_write
DELETE	/countries/bulk-delete	BulkDeleteCountriesCommand	country_write

#217 and #394 both modified tests/Integration/ApiPlatform/CountryEndpointTest.php and conflict on cherry-pick.

Tests

CountryStatusZoneEndpointTest is folded into CountryEndpointTest, and this is where the merge pays off: the status and zone tests were operating on fixture data.

// before
$countryId = (int) \Db::getInstance()->getValue('SELECT id_country FROM ...');
$after = (int) \Db::getInstance()->getValue('SELECT active FROM ...');

They looked their subject up in ps_country and asserted with SELECT active / SELECT id_zone — so they toggled and re-zoned whichever country the fixtures happened to install first, and left it that way for the rest of the suite. CountryEndpointTest, in the same domain, was already creating countries through POST /countries.

The status and zone tests now create the countries they operate on, and read the result back through GET /countries/{countryId}, which already exposes enabled and zoneId.

The required-fields assertion pins the complete key set (countryId, stateRequired, dniRequired) rather than checking three keys one by one.

The status and bulk operations require 9.2.0

ToggleCountryStatusCommand, BulkToggleCountriesStatusCommand, BulkUpdateCountryZoneCommand and BulkDeleteCountriesCommand were all introduced by the Countries grid migration (PrestaShop/PrestaShop#41495 and PrestaShop/PrestaShop#41931) and only ship from 9.2.0. On 9.0/9.1 ApiResourceScopesExtractor::skipCQRSNotFound() drops the operations from the routing entirely, so the endpoints answer 404 — not 401 — and they disappear from the scope registration too.

The four operations therefore declare extraProperties: ['minVersion' => '9.2.0'], their tests call markTestSkippedByMinVersion('9.2.0'), and getProtectedEndpoints() only yields their data sets from 9.2.0 on. This replaces the ad-hoc class_exists(BulkDeleteCountriesCommand::class) gate #394 used for the bulk delete alone.

How to test

GET    /countries/{id}/required-fields  -> 200 {countryId, stateRequired, dniRequired}
PUT    /countries/{id}/toggle-status    -> 200, then the GET shows enabled flipped
PUT    /countries/bulk-toggle-status    -> 200, then each GET shows the new status
PUT    /countries/bulk-update-zone      -> 200, then each GET shows the new zoneId
DELETE /countries/bulk-delete           -> 204, then each GET answers 404

The four status/bulk endpoints need a 9.2.0+ core; on 9.0/9.1 they are not routed at all.

Covered by CountryEndpointTest.

Supersedes

• Add Country required-fields endpoint #217
• Add Country toggle-status + bulk-toggle + bulk-update-zone endpoints #368
• Add BulkDeleteCountries endpoint #394

All three will be closed once the CI is green here.

[github-actions]

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

📋 Summary of changes

This PR adds five new Country-domain endpoints by merging three previously open PRs (#217, #368, #394): a read-only sub-resource GET /countries/{countryId}/required-fields, a per-country toggle PUT /countries/{countryId}/toggle-status, and three bulk operations (bulk-toggle-status, bulk-update-zone, bulk-delete). The four status/bulk endpoints are gated behind minVersion: '9.2.0' because the underlying CQRS commands (ToggleCountryStatusCommand, BulkToggleCountriesStatusCommand, BulkUpdateCountryZoneCommand, BulkDeleteCountriesCommand) only ship from 9.2.0. Tests are consolidated into CountryEndpointTest and migrated away from raw SQL fixture lookups.

⏱️ Estimated review time

30–40 minutes — five new resource classes with straightforward mappings, but the bulk/toggle operations raise questions about exception coverage, response bodies, and missing validation tests.

🎯 Scope

• Exposed operations: GET (required-fields sub-resource), PUT (toggle-status), PUT×2 (bulk-toggle-status, bulk-update-zone), DELETE (bulk-delete)
• CQRS entity: Country — GetCountryRequiredFields, ToggleCountryStatusCommand, BulkToggleCountriesStatusCommand, BulkUpdateCountryZoneCommand, BulkDeleteCountriesCommand
• Integration test: yes (in CountryEndpointTest)

🧱 API Platform / CQRS architecture compliance

CountryRequiredFields.php

• URI /countries/{countryId}/required-fields is correctly formed: plural base, kebab-case, sub-resource ✓
• CQRSGet + country_read scope ✓
• Identifier property $countryId with #[ApiProperty(identifier: true)] ✓
• No QUERY_MAPPING declared — this is correct only if GetCountryRequiredFields's result class returns fields named exactly countryId, stateRequired, and dniRequired. This should be verified against the Core query result class.
• CountryNotFoundException → 404 ✓

CountryStatus.php

• URI /countries/{countryId}/toggle-status ✓; CQRSUpdate (PUT) for a command-style operation ✓
• minVersion: '9.2.0', allowEmptyBody: true, read: false ✓
• No CQRSQuery: the response body will be {"countryId": X} — just the identifier echoed back. Per CONTEXT.md: "prefer returning the updated resource over answering 204 with output: false, so the caller does not need a follow-up GET." Adding CQRSQuery: GetCountryForEditing::class (reusing the existing GET query) would return the full country state. The test itself works around this by calling GET after the PUT.
• Rector keywords: toggle-status must be in ApiResourceUriTemplateRector::SKIPPED_KEYWORDS; if it is not, the Rector CI job will try to pluralize the segment. Please confirm this is already listed.

BulkCountriesStatus.php

• URI /countries/bulk-toggle-status, CQRSUpdate (PUT), country_write scope ✓
• CQRSCommandMapping: ['[enabled]' => '[expectedStatus]'] — direction API→command ✓
• Missing CountryException → HTTP_UNPROCESSABLE_ENTITY in exceptionToStatus. BulkCountriesZone maps CountryException → 422; BulkCountriesStatus does not. If BulkToggleCountriesStatusCommand can throw a CountryConstraintException (a subclass of CountryException), those violations would leak as 500s. This is worth verifying against the Core command and aligning with BulkCountriesZone.
• No CQRSQuery: response echoes the input DTO (enabled, countryIds) rather than updated state.
• $enabled has #[Assert\NotNull] (correct for a bool that must be explicitly set) ✓; $countryIds has #[Assert\NotBlank] ✓

BulkCountriesZone.php

• URI /countries/bulk-update-zone, CQRSUpdate (PUT), country_write scope ✓
• read: false ✓; CountryException → 422 and CountryNotFoundException → 404 ✓
• No CQRSCommandMapping — $countryIds and $newZoneId are passed as-is. Verify that BulkUpdateCountryZoneCommand accepts parameters named exactly countryIds and newZoneId; if the constructor uses a different name (e.g. zoneId), a mapping entry is required.
• #[Assert\GreaterThan(0)] on $newZoneId ✓

BulkDeleteCountries.php

• URI /countries/bulk-delete, CQRSDelete (DELETE), country_write scope ✓
• allowEmptyBody: false ✓; $countryIds property ✓
• Only maps CountryNotFoundException → 404. If deletion can fail with a constraint violation, CountryConstraintException → 422 should also be listed.

PHPStan neon files

• Correct pattern: #Class .*Country\\Command.* not found.# scoped to src/ApiPlatform/Resources/Country/* for 9.0.3 and 9.1.4 ✓

💡 Improvement suggestions

1. Return full country state from toggle-status — add CQRSQuery: GetCountryForEditing::class to CountryStatus. Right now the PUT returns {"countryId": X} and callers need a second GET to see the new enabled value. The test confirms this pattern is awkward (requestApi + a separate getCountry() assertion).

2. Align exceptionToStatus across bulk operations — BulkCountriesZone maps CountryException → 422 but BulkCountriesStatus and BulkDeleteCountries do not. Verify what each command can throw and add mappings as needed.

3. Confirm Rector skip-list — toggle-status, required-fields, bulk-toggle-status, and bulk-update-zone are action segments in URIs. Check that they are present in ApiResourceUriTemplateRector::SKIPPED_KEYWORDS so the Rector CI check doesn't try to pluralize them.

4. testToggleCountryStatus uses requestApi with no body assertion — the PUT returns HTTP 200 with {"countryId": X}, but the test ignores the response. If you keep the current minimal response, at least assert the returned countryId. If you add CQRSQuery, you can assert enabled directly.

5. Verify Core field names match DTO without extra mapping — GetCountryRequiredFields result, BulkUpdateCountryZoneCommand constructor params — both need to match DTO field names (stateRequired, dniRequired, countryIds, newZoneId) to avoid silent null fields or runtime denormalization errors.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case
• Identifier uses domain name + Id suffix (countryId)
• Sub-resources follow parent path (/countries/{countryId}/required-fields, /countries/{countryId}/toggle-status)
• Bulk operation URI uses bulk- prefix and plural Ids parameter (countryIds)

Operations & scopes

• Correct operation attribute per HTTP method (CQRSGet, CQRSUpdate, CQRSDelete)
• Scope format: country_read / country_write ✓

API Resource properties

• All properties strictly typed, scalars/arrays only
• Naming conventions respected (enabled, no is prefix, no localized prefix)
• #[ApiProperty(identifier: true)] on ID property (CountryRequiredFields, CountryStatus)
• #[LocalizedValue] / #[DefaultLanguage] — not applicable for these endpoints (no localized fields)

CQRS mapping

• CQRSCommandMapping direction: API field → command parameter (BulkCountriesStatus)
• No QUERY_MAPPING on CountryRequiredFields — needs verification that Core result fields match DTO names exactly
• CQRSQuery missing on CountryStatus CQRSUpdate — recommend adding to return full state rather than just identifier
• No SerializedName

Forbidden practices (CI-enforced)

• No custom normalizers or processors
• No Value Objects in properties

Exception handling & validation

• CountryNotFoundException → 404 mapped on all operations
• CountryException → 422 missing on BulkCountriesStatus (present on BulkCountriesZone, inconsistent)
• Potential missing CountryConstraintException → 422 on BulkDeleteCountries
• No HTTP_BAD_REQUEST (400) used for constraint violations

Multi-shop

• shopIds absent — countries are global entities, not shop-associated ✓

Listing field alignment — not applicable (no list endpoint in this PR)

Integration test

• Extends ApiTestCase, uses helper methods
• Missing testInvalid* methods for the new endpoints that accept a payload (BulkCountriesStatus, BulkCountriesZone, BulkDeleteCountries). CONTEXT.md requires a testInvalid… method with assertValidationErrors for every endpoint that has validation constraints.
• getProtectedEndpoints() lists new URIs (with isVersionAtLeast('9.2.0') guard for the 9.2+ operations)
• DatabaseDump::restoreTables() — existing setup covers ps_country; no new tables
• declare(strict_types=1) present in test file

[nicosomb]

A few items from the automated pre-review turned out fine after checking Core, no changes needed:

CountryRequiredFields with no QUERY_MAPPING: CountryRequiredFields's isStateRequired()/isDniRequired() match stateRequired/dniRequired exactly, and countryId comes from the URI variable merge rather than the query result. No mapping needed.

BulkCountriesZone with no CQRSCommandMapping: BulkUpdateCountryZoneCommand's constructor parameters (countryIds, newZoneId) match the DTO property names exactly.

The Rector skip-list for toggle-status/required-fields/etc: the Rector Dry Run check is green on this PR, which confirms these keywords are already handled.

[nicosomb]

Only CountryNotFoundException is mapped. BulkToggleCountriesStatusCommand builds a CountryId per id, whose assertion throws CountryConstraintException on an invalid one, unmapped here so it falls through to 500. BulkCountriesZone in this same PR has CountryException mapped to 422, worth aligning.

[nicosomb]

Same gap as BulkCountriesStatus: BulkDeleteCountriesCommand builds a CountryId per id the same way, and the resulting CountryConstraintException isn't mapped here either.

[nicosomb]

None of the four new endpoints here (bulk-delete, toggle-status, bulk-toggle-status, bulk-update-zone) have a testInvalid* counterpart. The existing create/edit tests do use assertValidationErrors, just not extended to these.