Skip to content

Expect the position field in AttributeGroup endpoint responses - #283

Open
boo-code wants to merge 2 commits into
PrestaShop:devfrom
boo-code:fix/attribute-group-position-in-response
Open

boo-code wants to merge 2 commits into
PrestaShop:devfrom
boo-code:fix/attribute-group-position-in-response

Conversation

@boo-code

@boo-code boo-code commented Jun 26, 2026 •

Copy link
Copy Markdown
Questions Answers
Branch? dev
Description? Update the AttributeGroup endpoint test to expect the position field now returned by the responses.
Type? bug fix
Category? CO
BC breaks? no
Fixed issue Companion to PrestaShop/PrestaShop#41844 (fixes PrestaShop/PrestaShop#39729)
How to test? composer create-test-db then run AttributeGroupEndpointTest.

Context

PrestaShop/PrestaShop#39729: the Admin API POST /attributes/group response was missing the position key. The core fix PrestaShop/PrestaShop#41844 adds position to EditableAttributeGroup, so the GET / POST / PATCH AttributeGroup responses now correctly include position.

This updates AttributeGroupEndpointTest to expect that key in the create, get and partial-update assertions (a newly created group takes the next position — position = itemsCount).

Verification

Run end-to-end against an install carrying the core fix: AttributeGroupEndpointTest is 15/15 green. Without the core fix it goes red on the new position expectation (proving the two changes belong together).

The expectation is gated on self::isVersionAtLeast('9.3.0'), the milestone of PrestaShop/PrestaShop#41844. The develop leg already reports 9.3.0, so it runs the assertions and stays red until PrestaShop/PrestaShop#41844 merges; the other legs skip them.

@ps-jarvis

Copy link
Copy Markdown

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!

@boo-code

Copy link
Copy Markdown
Author

Updated in 9f0cccb so this no longer has to be merged in lockstep with the core side.

The single-item expectations now adapt to the core they run against, instead of pinning position unconditionally:

private function expectedPosition(int $position): array
{
    return method_exists(EditableAttributeGroup::class, 'getPosition') ? ['position' => $position] : [];
}

getPosition() only exists once PrestaShop/PrestaShop#41844 is in, so on a core without it the expected payload keeps its previous shape, and on a core with it the new key is asserted. The list-endpoint assertions are untouched, since those read the position from the grid query and already worked.

Why it matters: as it stood, this PR and PrestaShop/PrestaShop#41844 could not go green at the same time - #41844 fails API Module tests because dev here still expects the old shape, and this PR would fail against cores that do not expose the key yet. With the probe, each can merge on its own.

One blocker I cannot clear myself: the workflow runs on this PR sit in action_required (Integration tests, PHP tests, Build release, queued today at 12:22 for 9f0cccb), so the change is unverified by CI. #295 in this repo runs without that gate, so it seems specific to this PR. Could someone approve the runs?

@boo-code

Copy link
Copy Markdown
Author

Recording why this shows no CI, since it is not a state of the branch. All three workflow runs on 9f0cccbb are action_required and never started: PHP tests (30358646852), Integration tests (30358647846) and Build release (30358652088), all queued 2026-07-28T12:22Z. The commit therefore carries zero check runs, and the PR's merge state is BLOCKED with mergeable=MERGEABLE - no conflicts, just nothing to satisfy the required checks. This is not something a re-push clears: pushing again only queues another action_required run, so it needs the workflow approval on this repository.

Relevant because PrestaShop/PrestaShop#41844 keeps a red API Module tests until this merges: that job runs AttributeGroupEndpointTest from dev, which still expects the pre-position payload. The expectations here are gated on method_exists(EditableAttributeGroup::class, 'getPosition'), so this branch is green against a core with the change and against one without it, and can merge on its own.

*/
private function expectedPosition(int $position): array
{
return method_exists(EditableAttributeGroup::class, 'getPosition') ? ['position' => $position] : [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you gate these expectations on the core version with self::isVersionAtLeast(), like the other core-dependent assertions (CountryEndpointTest, CustomerEndpointTest)? The method_exists() probe follows the class shape rather than the API contract and asserts nothing new on any current core, and pinning this branch in PrestaShop/PrestaShop#41844 already lets the core CI go green before either side merges.

@ps-jarvis ps-jarvis moved this from Ready for review to Waiting for author in PR Dashboard Oct 29, 2026
Audrius and others added 2 commits October 29, 2026 18:58
The GET/POST/PATCH AttributeGroup responses now include the group position
(PrestaShop/PrestaShop#39729, fixed core-side in PrestaShop/PrestaShop#41844).
Update the endpoint test expectations accordingly.
@boo-code
boo-code force-pushed the fix/attribute-group-position-in-response branch from 9f0cccb to 786779b Compare October 29, 2026 18:47
@boo-code

Copy link
Copy Markdown
Author

Done: gated on self::isVersionAtLeast('9.3.0'), the milestone of PrestaShop/PrestaShop#41844. The develop leg already reports 9.3.0, so it runs these assertions and stays red until PrestaShop/PrestaShop#41844 merges; the other legs skip them. On a 9.2.1 core: 15 tests green, and forcing the expectation on fails testAddAttributeGroup, plus testAttributeGroupPositions and testBulkRemoveAttributeGroups: those have no @depends, they fail because the skipped testRemoveAttributeGroup leaves the created group behind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Waiting for author

Development

Successfully merging this pull request may close these issues.

Admin API : Endpoint POST /attributes/group : The response is not the expected : Missing key position

3 participants