From 47d2ed5661f4779a72717729c09057a3d59d3b76 Mon Sep 17 00:00:00 2001 From: killecaptron <203002577+killecaptron@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:29:59 +0200 Subject: [PATCH 1/2] Do not treat a scalar value as an IRI when it is not for a resource class SkippableItemNormalizer resolves IRI strings itself, to work around an API Platform issue with abstract resource classes which have a discriminator map (see #1370). That check only looked at the shape of the data, not at what was being denormalized, so it also caught plain values nested inside a resource - a backed enum for example, whose scalar value is a string, but certainly not an IRI. Such a value was silently swallowed instead of being handed to the normalizer which is actually responsible for it. Only resolve IRIs if the target type really is an API resource class. Co-Authored-By: Claude Opus 5 --- .../APIPlatform/SkippableItemNormalizer.php | 10 +- .../SkippableItemNormalizerTest.php | 111 ++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 tests/Serializer/APIPlatform/SkippableItemNormalizerTest.php diff --git a/src/Serializer/APIPlatform/SkippableItemNormalizer.php b/src/Serializer/APIPlatform/SkippableItemNormalizer.php index 618736152..169520e14 100644 --- a/src/Serializer/APIPlatform/SkippableItemNormalizer.php +++ b/src/Serializer/APIPlatform/SkippableItemNormalizer.php @@ -26,6 +26,7 @@ use ApiPlatform\Metadata\Exception\InvalidArgumentException; use ApiPlatform\Metadata\Exception\ItemNotFoundException; use ApiPlatform\Metadata\IriConverterInterface; +use ApiPlatform\Metadata\ResourceClassResolverInterface; use ApiPlatform\Serializer\ItemNormalizer; use Symfony\Component\DependencyInjection\Attribute\AsDecorator; use Symfony\Component\Serializer\Exception\NotNormalizableValueException; @@ -53,6 +54,7 @@ class SkippableItemNormalizer implements NormalizerInterface, DenormalizerInterf public function __construct( private readonly ItemNormalizer $inner, private readonly IriConverterInterface $iriConverter, + private readonly ResourceClassResolverInterface $resourceClassResolver, ) { } @@ -63,7 +65,13 @@ public function denormalize(mixed $data, string $type, ?string $format = null, a // check (line 271). For abstract resource classes with a discriminator map (e.g. Attachment), this // fails because the array has no _type key. Fix by resolving IRI strings directly. // See: https://github.com/Part-DB/Part-DB-server/issues/1370 - if (is_string($data) || (is_array($data) && isset($data['@id']) && is_string($data['@id']))) { + // + // $type must actually be an API resource for this to make sense: this normalizer also runs for plain + // value objects (e.g. backed enums) nested inside a resource, and a string value there is the enum's + // scalar value, not an IRI - treating it as one silently swallows the value instead of letting the + // regular (enum) normalizer handle it. + if ($this->resourceClassResolver->isResourceClass($type) + && (is_string($data) || (is_array($data) && isset($data['@id']) && is_string($data['@id'])))) { if (is_array($data)) { $iri = $data['@id']; } else { diff --git a/tests/Serializer/APIPlatform/SkippableItemNormalizerTest.php b/tests/Serializer/APIPlatform/SkippableItemNormalizerTest.php new file mode 100644 index 000000000..83e3e71ab --- /dev/null +++ b/tests/Serializer/APIPlatform/SkippableItemNormalizerTest.php @@ -0,0 +1,111 @@ +. + */ +namespace App\Tests\Serializer\APIPlatform; + +use ApiPlatform\Metadata\IriConverterInterface; +use ApiPlatform\Metadata\ResourceClassResolverInterface; +use ApiPlatform\Serializer\ItemNormalizer; +use App\Entity\Attachments\Attachment; +use App\Serializer\APIPlatform\SkippableItemNormalizer; +use PHPUnit\Framework\TestCase; + +final class SkippableItemNormalizerTest extends TestCase +{ + private ItemNormalizer&\PHPUnit\Framework\MockObject\MockObject $inner; + private IriConverterInterface&\PHPUnit\Framework\MockObject\MockObject $iriConverter; + private ResourceClassResolverInterface&\PHPUnit\Framework\MockObject\MockObject $resourceClassResolver; + private SkippableItemNormalizer $normalizer; + + protected function setUp(): void + { + $this->inner = $this->createMock(ItemNormalizer::class); + $this->iriConverter = $this->createMock(IriConverterInterface::class); + $this->resourceClassResolver = $this->createMock(ResourceClassResolverInterface::class); + $this->normalizer = new SkippableItemNormalizer($this->inner, $this->iriConverter, $this->resourceClassResolver); + } + + public function testStringIsResolvedAsIriForResourceClass(): void + { + //Regression guard for https://github.com/Part-DB/Part-DB-server/issues/1370: an IRI string for a resource + //class (like Attachment, which has a discriminator map) must still be resolved via the IriConverter. + $this->resourceClassResolver->method('isResourceClass')->with(Attachment::class)->willReturn(true); + + $attachment = $this->createMock(Attachment::class); + $this->iriConverter->expects($this->once()) + ->method('getResourceFromIri') + ->with('/api/attachments/1') + ->willReturn($attachment); + + $result = $this->normalizer->denormalize('/api/attachments/1', Attachment::class); + + $this->assertSame($attachment, $result); + } + + public function testStringIsNotResolvedAsIriForNonResourceClass(): void + { + //A backed enum (or any other plain value object) is not an API resource, so a string value denormalized + //into it is the enum's scalar value, not an IRI - it must be passed through to the inner normalizer + //(which delegates to Symfony's BackedEnumNormalizer) instead of being swallowed by a failed IRI lookup. + $this->resourceClassResolver->method('isResourceClass')->willReturn(false); + + $this->iriConverter->expects($this->never())->method('getResourceFromIri'); + $this->inner->expects($this->once()) + ->method('denormalize') + ->with('warning', 'SomeEnum', null, []) + ->willReturn('warning-denormalized'); + + $result = $this->normalizer->denormalize('warning', 'SomeEnum'); + + $this->assertSame('warning-denormalized', $result); + } + + public function testArrayWithIriIsResolvedForResourceClass(): void + { + $this->resourceClassResolver->method('isResourceClass')->with(Attachment::class)->willReturn(true); + + $attachment = $this->createMock(Attachment::class); + $this->iriConverter->expects($this->once()) + ->method('getResourceFromIri') + ->with('/api/attachments/1') + ->willReturn($attachment); + + $result = $this->normalizer->denormalize(['@id' => '/api/attachments/1'], Attachment::class); + + $this->assertSame($attachment, $result); + } + + public function testArrayWithoutIriIsPassedThrough(): void + { + $this->resourceClassResolver->method('isResourceClass')->willReturn(true); + + $this->iriConverter->expects($this->never())->method('getResourceFromIri'); + $this->inner->expects($this->once()) + ->method('denormalize') + ->with(['name' => 'Test'], Attachment::class, null, []) + ->willReturn('denormalized'); + + $result = $this->normalizer->denormalize(['name' => 'Test'], Attachment::class); + + $this->assertSame('denormalized', $result); + } +} From c06c358dc546cff6679d1fa737fa7044786ec189 Mon Sep 17 00:00:00 2001 From: killecaptron <203002577+killecaptron@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:30:00 +0200 Subject: [PATCH 2/2] Add an optional color to the custom part states Custom part states are shown as a badge on the part page and in the part tables, but every state looked the same, so a state like "defective" could not be told apart from "in testing" at a glance. A state can now optionally be given one of the eight semantic Bootstrap colors. The choice is a closed list, so no free-form CSS class or color can end up in the templates, and the mapping from a color to its badge class exists exactly once. States without a color keep exactly the appearance they had before. The custom state is also available as an (optional) column in the project BOM table, where the color is what makes the column useful in the first place. Rendering the badge lives in the part table helper and in the entity itself, so the part table, the BOM table and the part page can not drift apart. Co-Authored-By: Claude Opus 5 --- migrations/Version20260903120000.php | 46 ++++++++++ .../Helpers/PartDataTableHelper.php | 17 ++++ src/DataTables/PartsDataTable.php | 13 +-- src/DataTables/ProjectBomEntriesDataTable.php | 8 ++ src/Entity/Parts/PartCustomState.php | 31 +++++++ src/Entity/Parts/PartCustomStateColor.php | 53 +++++++++++ .../AdminPages/PartCustomStateAdminForm.php | 23 +++++ .../admin/part_custom_state_admin.html.twig | 4 + templates/parts/info/_sidebar.html.twig | 2 +- .../Endpoints/PartCustomStateEndpointTest.php | 18 ++++ .../Entity/Parts/PartCustomStateColorTest.php | 88 +++++++++++++++++++ translations/messages.en.xlf | 60 +++++++++++++ 12 files changed, 352 insertions(+), 11 deletions(-) create mode 100644 migrations/Version20260903120000.php create mode 100644 src/Entity/Parts/PartCustomStateColor.php create mode 100644 tests/Entity/Parts/PartCustomStateColorTest.php diff --git a/migrations/Version20260903120000.php b/migrations/Version20260903120000.php new file mode 100644 index 000000000..d9c730301 --- /dev/null +++ b/migrations/Version20260903120000.php @@ -0,0 +1,46 @@ +addSql('ALTER TABLE part_custom_states ADD color VARCHAR(20) DEFAULT NULL'); + } + + public function mySQLDown(Schema $schema): void + { + $this->addSql('ALTER TABLE part_custom_states DROP COLUMN color'); + } + + public function sqLiteUp(Schema $schema): void + { + $this->addSql('ALTER TABLE part_custom_states ADD COLUMN color VARCHAR(20) DEFAULT NULL'); + } + + public function sqLiteDown(Schema $schema): void + { + $this->addSql('ALTER TABLE part_custom_states DROP COLUMN color'); + } + + public function postgreSQLUp(Schema $schema): void + { + $this->addSql('ALTER TABLE part_custom_states ADD color VARCHAR(20) DEFAULT NULL'); + } + + public function postgreSQLDown(Schema $schema): void + { + $this->addSql('ALTER TABLE part_custom_states DROP COLUMN color'); + } +} diff --git a/src/DataTables/Helpers/PartDataTableHelper.php b/src/DataTables/Helpers/PartDataTableHelper.php index 065e1201c..80a962e06 100644 --- a/src/DataTables/Helpers/PartDataTableHelper.php +++ b/src/DataTables/Helpers/PartDataTableHelper.php @@ -27,6 +27,7 @@ use App\Entity\ProjectSystem\Project; use App\Entity\Attachments\Attachment; use App\Entity\Parts\Part; +use App\Entity\Parts\PartCustomState; use App\Services\Attachments\AttachmentURLGenerator; use App\Services\Attachments\PartPreviewGenerator; use App\Services\EntityURLGenerator; @@ -170,6 +171,22 @@ public function renderEdaStatus(Part $context): string return sprintf('%s', $editUrl, $statusIcon); } + /** + * Renders the custom state of a part as the colored badge it is configured with. + * Returns an empty string if the part has no custom state. + */ + public function renderPartCustomState(?PartCustomState $state): string + { + if ($state === null) { + return ''; + } + + return sprintf('%s', + htmlspecialchars($state->getBadgeClass()), + htmlspecialchars($state->getName()) + ); + } + public function renderAmount(Part $context): string { $amount = $context->getAmountSum(); diff --git a/src/DataTables/PartsDataTable.php b/src/DataTables/PartsDataTable.php index c15468de5..a43a1939e 100644 --- a/src/DataTables/PartsDataTable.php +++ b/src/DataTables/PartsDataTable.php @@ -198,18 +198,11 @@ public function configure(DataTable $dataTable, array $options): void return $tmp; } ]) - ->add('partCustomState', TextColumn::class, [ + ->add('partCustomState', HTMLColumn::class, [ 'label' => $this->translator->trans('part.table.partCustomState'), 'orderField' => 'NATSORT(_partCustomState.name)', - 'data' => function(Part $context): string { - $partCustomState = $context->getPartCustomState(); - - if ($partCustomState === null) { - return ''; - } - - return $partCustomState->getName(); - } + 'data' => fn(Part $context): string + => $this->partDataTableHelper->renderPartCustomState($context->getPartCustomState()), ]) ->add('addedDate', LocaleDateTimeColumn::class, [ 'label' => $this->translator->trans('part.table.addedDate'), diff --git a/src/DataTables/ProjectBomEntriesDataTable.php b/src/DataTables/ProjectBomEntriesDataTable.php index 0f7af87c9..3245d097e 100644 --- a/src/DataTables/ProjectBomEntriesDataTable.php +++ b/src/DataTables/ProjectBomEntriesDataTable.php @@ -177,6 +177,14 @@ public function configure(DataTable $dataTable, array $options): void }, ]) + ->add('partCustomState', HTMLColumn::class, [ + 'label' => $this->translator->trans('part.table.partCustomState'), + 'orderField' => 'NATSORT(partCustomState.name)', + 'visible' => false, + 'data' => fn (ProjectBOMEntry $context): string + => $this->partDataTableHelper->renderPartCustomState($context->getPart()?->getPartCustomState()), + ]) + ->add('mountnames', HTMLColumn::class, [ 'label' => 'project.bom.mountnames', 'data' => function (ProjectBOMEntry $context) { diff --git a/src/Entity/Parts/PartCustomState.php b/src/Entity/Parts/PartCustomState.php index 29a96c007..396f7100d 100644 --- a/src/Entity/Parts/PartCustomState.php +++ b/src/Entity/Parts/PartCustomState.php @@ -50,6 +50,7 @@ use App\State\Mcp\ListStructuralElementsProcessor; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; +use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Serializer\Attribute\Groups; use Symfony\Component\Validator\Constraints as Assert; @@ -108,6 +109,14 @@ class PartCustomState extends AbstractPartsContainingDBElement #[Groups(['part_custom_state:read', 'part_custom_state:write', 'full', 'import'])] protected string $comment = ''; + /** + * @var PartCustomStateColor|null The semantic color this state is rendered as a badge with. + * Null keeps the default, uncolored appearance Part-DB used before this field existed. + */ + #[ORM\Column(type: Types::STRING, length: 20, nullable: true, enumType: PartCustomStateColor::class)] + #[Groups(['part_custom_state:read', 'part_custom_state:write', 'full', 'import'])] + protected ?PartCustomStateColor $color = null; + #[ORM\OneToMany(targetEntity: self::class, mappedBy: 'parent', cascade: ['persist'])] #[ORM\OrderBy(['name' => 'ASC'])] protected Collection $children; @@ -152,4 +161,26 @@ public function __construct() $this->attachments = new ArrayCollection(); $this->parameters = new ArrayCollection(); } + + public function getColor(): ?PartCustomStateColor + { + return $this->color; + } + + public function setColor(?PartCustomStateColor $color): self + { + $this->color = $color; + + return $this; + } + + /** + * Returns the CSS class this state is rendered as a badge with, everywhere it is shown. + * Without a configured color this is the color Part-DB used before the color existed, so an unconfigured + * state keeps looking exactly the way it did. + */ + public function getBadgeClass(): string + { + return $this->color?->toBadgeClass() ?? 'bg-primary'; + } } diff --git a/src/Entity/Parts/PartCustomStateColor.php b/src/Entity/Parts/PartCustomStateColor.php new file mode 100644 index 000000000..d5093aed1 --- /dev/null +++ b/src/Entity/Parts/PartCustomStateColor.php @@ -0,0 +1,53 @@ +. + */ + +declare(strict_types=1); + +namespace App\Entity\Parts; + +/** + * The semantic Bootstrap color a PartCustomState can be rendered with. + * This is a closed whitelist: no free-form CSS classes or colors can be stored. + */ +enum PartCustomStateColor: string +{ + case PRIMARY = 'primary'; + case SECONDARY = 'secondary'; + case INFO = 'info'; + case SUCCESS = 'success'; + case WARNING = 'warning'; + case DANGER = 'danger'; + case LIGHT = 'light'; + case DARK = 'dark'; + + public function toTranslationKey(): string + { + return 'part_custom_state.color.' . $this->value; + } + + /** + * Maps this color to the fixed Bootstrap badge class it is rendered with. + * This is the only place that translates a stored color into a CSS class. + */ + public function toBadgeClass(): string + { + return 'text-bg-' . $this->value; + } +} diff --git a/src/Form/AdminPages/PartCustomStateAdminForm.php b/src/Form/AdminPages/PartCustomStateAdminForm.php index b8bb2815e..cba043e9c 100644 --- a/src/Form/AdminPages/PartCustomStateAdminForm.php +++ b/src/Form/AdminPages/PartCustomStateAdminForm.php @@ -22,6 +22,29 @@ namespace App\Form\AdminPages; +use App\Entity\Base\AbstractNamedDBElement; +use App\Entity\Parts\PartCustomState; +use App\Entity\Parts\PartCustomStateColor; +use Symfony\Component\Form\Extension\Core\Type\EnumType; +use Symfony\Component\Form\FormBuilderInterface; + class PartCustomStateAdminForm extends BaseEntityAdminForm { + protected function additionalFormElements(FormBuilderInterface $builder, array $options, AbstractNamedDBElement $entity): void + { + if (!$entity instanceof PartCustomState) { + return; + } + + $is_new = null === $entity->getID(); + + $builder->add('color', EnumType::class, [ + 'class' => PartCustomStateColor::class, + 'choice_label' => fn (PartCustomStateColor $color) => $color->toTranslationKey(), + 'required' => false, + 'label' => 'part_custom_state.color.label', + 'help' => 'part_custom_state.color.help', + 'disabled' => !$this->security->isGranted($is_new ? 'create' : 'edit', $entity), + ]); + } } diff --git a/templates/admin/part_custom_state_admin.html.twig b/templates/admin/part_custom_state_admin.html.twig index 9d8576468..dac799e00 100644 --- a/templates/admin/part_custom_state_admin.html.twig +++ b/templates/admin/part_custom_state_admin.html.twig @@ -12,3 +12,7 @@ {% trans %}part_custom_state.new{% endtrans %} {% endblock %} +{% block additional_controls %} + {{ form_row(form.color) }} +{% endblock %} + diff --git a/templates/parts/info/_sidebar.html.twig b/templates/parts/info/_sidebar.html.twig index 120602418..06d2a3b3e 100644 --- a/templates/parts/info/_sidebar.html.twig +++ b/templates/parts/info/_sidebar.html.twig @@ -47,7 +47,7 @@ {% if part.partCustomState is not null %}
- {{ part.partCustomState.name }} + {{ part.partCustomState.name }} {% if part.partCustomState is not null and part.partCustomState.masterPictureAttachment and attachment_manager.fileExisting(part.partCustomState.masterPictureAttachment) %}
diff --git a/tests/API/Endpoints/PartCustomStateEndpointTest.php b/tests/API/Endpoints/PartCustomStateEndpointTest.php index 8d1253f30..def1d991a 100644 --- a/tests/API/Endpoints/PartCustomStateEndpointTest.php +++ b/tests/API/Endpoints/PartCustomStateEndpointTest.php @@ -66,4 +66,22 @@ public function testDeleteItem(): void { $this->_testDeleteItem(4); } + + public function testColorIsWritableAndReadable(): void + { + $this->_testPatchItem(5, [ + 'color' => 'warning', + ]); + self::assertJsonContains([ + 'color' => 'warning', + ]); + + //Unsetting the color must be possible again (back to the default, uncolored appearance). + //Like every other null-valued field in this API, an unset color is omitted from the response entirely + //rather than being serialized as an explicit "color": null (see e.g. InfoProviderEndpointTest). + $response = $this->_testPatchItem(5, [ + 'color' => null, + ]); + self::assertArrayNotHasKey('color', json_decode($response->getContent(), true)); + } } diff --git a/tests/Entity/Parts/PartCustomStateColorTest.php b/tests/Entity/Parts/PartCustomStateColorTest.php new file mode 100644 index 000000000..708e1fe4c --- /dev/null +++ b/tests/Entity/Parts/PartCustomStateColorTest.php @@ -0,0 +1,88 @@ +. + */ +namespace App\Tests\Entity\Parts; + +use App\Entity\Parts\PartCustomState; +use App\Entity\Parts\PartCustomStateColor; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\TestCase; + +final class PartCustomStateColorTest extends TestCase +{ + public static function colorProvider(): array + { + return [ + [PartCustomStateColor::PRIMARY, 'text-bg-primary'], + [PartCustomStateColor::SECONDARY, 'text-bg-secondary'], + [PartCustomStateColor::INFO, 'text-bg-info'], + [PartCustomStateColor::SUCCESS, 'text-bg-success'], + [PartCustomStateColor::WARNING, 'text-bg-warning'], + [PartCustomStateColor::DANGER, 'text-bg-danger'], + [PartCustomStateColor::LIGHT, 'text-bg-light'], + [PartCustomStateColor::DARK, 'text-bg-dark'], + ]; + } + + #[DataProvider('colorProvider')] + public function testToBadgeClassMapsEveryColorToAFixedClass(PartCustomStateColor $color, string $expectedClass): void + { + $this->assertSame($expectedClass, $color->toBadgeClass()); + } + + public function testUnknownColorValueIsRejected(): void + { + $this->assertNull(PartCustomStateColor::tryFrom('not-a-real-color')); + } + + public function testPartCustomStateDefaultsToNoColor(): void + { + $state = new PartCustomState(); + + $this->assertNull($state->getColor()); + } + + public function testPartCustomStateColorCanBeSetAndRetrieved(): void + { + $state = new PartCustomState(); + $state->setColor(PartCustomStateColor::WARNING); + + $this->assertSame(PartCustomStateColor::WARNING, $state->getColor()); + + $state->setColor(null); + $this->assertNull($state->getColor()); + } + + public function testBadgeClassFallsBackToThePreviousAppearance(): void + { + //A state without a configured color has to keep looking exactly the way it did before colors existed + $this->assertSame('bg-primary', (new PartCustomState())->getBadgeClass()); + } + + #[DataProvider('colorProvider')] + public function testBadgeClassUsesTheConfiguredColor(PartCustomStateColor $color, string $expectedClass): void + { + $state = (new PartCustomState())->setColor($color); + + $this->assertSame($expectedClass, $state->getBadgeClass()); + } +} diff --git a/translations/messages.en.xlf b/translations/messages.en.xlf index f7a9d4e9f..e738a576a 100644 --- a/translations/messages.en.xlf +++ b/translations/messages.en.xlf @@ -15110,5 +15110,65 @@ Buerklin-API Authentication server: Edit BOM entry #%id% of project + + + part_custom_state.color.label + Color + + + + + part_custom_state.color.help + Optional semantic color this custom state is shown with wherever it appears as a badge (e.g. on the part detail page and in part tables). Without a color, the previous default appearance is used. + + + + + part_custom_state.color.primary + Primary + + + + + part_custom_state.color.secondary + Secondary + + + + + part_custom_state.color.info + Info + + + + + part_custom_state.color.success + Success + + + + + part_custom_state.color.warning + Warning + + + + + part_custom_state.color.danger + Danger + + + + + part_custom_state.color.light + Light + + + + + part_custom_state.color.dark + Dark + +