diff --git a/CHANGELOG.md b/CHANGELOG.md index c43a87ba..0c4c8a01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +- Fixed issue where saving a screen changed all regions with same ID. + ## [2.1.2] - 2024-10-24 - [#213](https://github.com/os2display/display-api-service/pull/213) @@ -16,7 +18,7 @@ All notable changes to this project will be documented in this file. - Update psalm baseline - Add regions/playlists and groups to POST screen test - `composer update symfony/* --with-dependencies` - - + ## [2.1.1] - 2024-10-23 - [#217](https://github.com/os2display/display-api-service/pull/217) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 1931db62..d09ac5d0 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + @@ -525,11 +525,6 @@ - - - - - @@ -565,7 +560,6 @@ - @@ -670,21 +664,12 @@ - + - - - - - - - - - - - - - + + + + diff --git a/psalm.xml b/psalm.xml index 94ff6eaa..2d36c754 100644 --- a/psalm.xml +++ b/psalm.xml @@ -4,7 +4,7 @@ resolveFromConfigFile="true" findUnusedBaselineEntry="true" findUnusedCode="false" - errorBaseline="./psalm-baseline.xml" + errorBaseline="psalm-baseline.xml" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="https://getpsalm.org/schema/config" xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" diff --git a/src/State/AbstractProcessor.php b/src/State/AbstractProcessor.php index dc158ec9..460e39bf 100644 --- a/src/State/AbstractProcessor.php +++ b/src/State/AbstractProcessor.php @@ -53,11 +53,11 @@ public function toOutput(object $object): object * * This is needed to get an object handled by entity manager. * - * @param $object + * @param \App\Entity\Tenant\Playlist|\App\Entity\Tenant\Screen|\App\Entity\Tenant\ScreenGroup|\App\Entity\Tenant\Slide|\App\Entity\Tenant\Theme $object * * @return mixed|object|null */ - protected function loadPrevious($object, array $context) + protected function loadPrevious(\App\Entity\Tenant\Playlist|\App\Entity\Tenant\ScreenGroup|\App\Entity\Tenant\Screen|\App\Entity\Tenant\Slide|\App\Entity\Tenant\Theme $object, array $context) { if ($previous = $context['previous_data'] ?? null) { $repository = $this->entityManager->getRepository($object::class); diff --git a/src/State/ScreenProcessor.php b/src/State/ScreenProcessor.php index a36353b7..57f32e47 100644 --- a/src/State/ScreenProcessor.php +++ b/src/State/ScreenProcessor.php @@ -8,7 +8,6 @@ use ApiPlatform\Metadata\Operation; use ApiPlatform\State\ProcessorInterface; use App\Dto\ScreenInput; -use App\Entity\Tenant\Playlist; use App\Entity\Tenant\PlaylistScreenRegion; use App\Entity\Tenant\Screen; use App\Repository\PlaylistRepository; @@ -32,7 +31,7 @@ public function __construct( EntityManagerInterface $entityManager, ProcessorInterface $persistProcessor, ProcessorInterface $removeProcessor, - ScreenProvider $provider + ScreenProvider $provider, ) { parent::__construct($entityManager, $persistProcessor, $removeProcessor, $provider); } @@ -42,6 +41,10 @@ protected function fromInput(mixed $object, Operation $operation, array $uriVari // FIXME Do we really have to do (something like) this to load an existing object into the entity manager? $screen = $this->loadPrevious(new Screen(), $context); + if (!$screen instanceof Screen) { + throw new InvalidArgumentException('object must be of type Screen'); + } + assert($object instanceof ScreenInput); empty($object->title) ?: $screen->setTitle($object->title); empty($object->description) ?: $screen->setDescription($object->description); @@ -56,57 +59,67 @@ protected function fromInput(mixed $object, Operation $operation, array $uriVari $screen->setEnableColorSchemeChange($object->enableColorSchemeChange); } - // Adding relations for playlist/screen/region - if (isset($object->regions) && isset($screen)) { + // Adding relations for playlist/screen/region if object has region property. + if (isset($object->regions)) { + // Ensure regions object has valid structure + $this->validateRegionsAndPlaylists($object->regions); + + $existingPlaylistScreenRegions = $screen->getPlaylistScreenRegions(); + foreach ($object->regions as $regionAndPlaylists) { - // Relevant region - $region = $this->screenLayoutRegionsRepository->findOneBy(['id' => $regionAndPlaylists['regionId']]); + $regionId = $regionAndPlaylists['regionId']; + + $region = $this->screenLayoutRegionsRepository->findOneBy(['id' => $regionId]); if (is_null($region)) { - throw new InvalidArgumentException('Unknown region resource'); + throw new InvalidArgumentException(sprintf('Unknown region resource (id: %s)', $regionId)); } - // Collection to be saved. - $playlistScreenRegionCollection = new ArrayCollection(); + $existingPlaylistScreenRegionsInRegion = $existingPlaylistScreenRegions->filter( + fn (PlaylistScreenRegion $psr) => $psr->getRegion()?->getId() == $regionId + ); + + $inputPlaylists = $regionAndPlaylists['playlists']; + $inputPlaylistIds = array_map(fn (array $entry): string => $entry['id'], $inputPlaylists); + + // Remove playlist screen regions that should not exist in region. + /** @var PlaylistScreenRegion $existingPSR */ + foreach ($existingPlaylistScreenRegionsInRegion as $existingPSR) { + if (!in_array($existingPSR->getPlaylist()?->getId(), $inputPlaylistIds)) { + $screen->removePlaylistScreenRegion($existingPSR); + } + } - // Looping through playlists connected to region - foreach ($regionAndPlaylists['playlists'] as $inputPlaylist) { - // Checking if playlists exists + // Add or update the input playlists. + foreach ($inputPlaylists as $inputPlaylist) { $playlist = $this->playlistRepository->findOneBy(['id' => $inputPlaylist['id']]); if (is_null($playlist)) { - throw new InvalidArgumentException('Unknown playlist resource'); + throw new InvalidArgumentException(sprintf('Unknown playlist resource (id: %s)', $inputPlaylist['id'])); } - // See if relation already exists - $existingPlaylistScreenRegion = $this->playlistScreenRegionRepository->findOneBy([ - 'screen' => $screen, - 'region' => $region, + $existingPlaylistScreenRegionRelation = $this->playlistScreenRegionRepository->findOneBy([ 'playlist' => $playlist, + 'region' => $region, + 'screen' => $screen, ]); - if (is_null($existingPlaylistScreenRegion)) { - // If relation does not exist, create new PlaylistScreenRegion + if (!is_null($existingPlaylistScreenRegionRelation)) { + $existingPlaylistScreenRegionRelation->setWeight($inputPlaylist['weight'] ?? 0); + } else { $newPlaylistScreenRegionRelation = new PlaylistScreenRegion(); $newPlaylistScreenRegionRelation->setPlaylist($playlist); $newPlaylistScreenRegionRelation->setRegion($region); $newPlaylistScreenRegionRelation->setScreen($screen); $newPlaylistScreenRegionRelation->setWeight($inputPlaylist['weight'] ?? 0); - /** @psalm-suppress InvalidArgument */ - $playlistScreenRegionCollection->add($newPlaylistScreenRegionRelation); - } else { - // Update weight, add existing relation - $existingPlaylistScreenRegion->setWeight($inputPlaylist['weight'] ?? 0); - /** @psalm-suppress InvalidArgument */ - $playlistScreenRegionCollection->add($existingPlaylistScreenRegion); + $screen->addPlaylistScreenRegion($newPlaylistScreenRegionRelation); } } - $region->setPlaylistScreenRegions($playlistScreenRegionCollection); } } // Maps ids of existing groups - if (isset($object->groups) && isset($screen)) { + if (isset($object->groups)) { $groupCollection = new ArrayCollection(); foreach ($object->groups as $group) { $groupToSave = $this->groupRepository->findOneBy(['id' => $group]); @@ -134,4 +147,38 @@ protected function fromInput(mixed $object, Operation $operation, array $uriVari return $screen; } + + private function validateRegionsAndPlaylists(array $regions): void + { + foreach ($regions as $region) { + $this->validateRegion($region); + + foreach ($region['playlists'] as $playlist) { + $this->validatePlaylist($playlist); + } + } + } + + private function validateRegion(array $region): void + { + if (!isset($region['regionId']) || !is_string($region['regionId'])) { + throw new InvalidArgumentException('All regions must specify a valid Ulid'); + } + + if (!isset($region['playlists']) || !is_array($region['playlists'])) { + throw new InvalidArgumentException('All regions must specify a list of playlists'); + } + } + + private function validatePlaylist(array $playlist): void + { + if (!isset($playlist['id']) || !is_string($playlist['id'])) { + throw new InvalidArgumentException('All playlists must specify a valid Ulid'); + + } + + if (isset($playlist['weight']) && !is_integer($playlist['weight'])) { + throw new InvalidArgumentException('Playlists weight must be an integer'); + } + } } diff --git a/tests/Api/ScreensTest.php b/tests/Api/ScreensTest.php index 19c3e9e4..65d5271b 100644 --- a/tests/Api/ScreensTest.php +++ b/tests/Api/ScreensTest.php @@ -7,12 +7,27 @@ use App\Entity\ScreenLayout; use App\Entity\ScreenLayoutRegions; use App\Entity\Tenant\Playlist; +use App\Entity\Tenant\PlaylistScreenRegion; use App\Entity\Tenant\Screen; use App\Entity\Tenant\ScreenGroup; use App\Tests\AbstractBaseApiTestCase; +use Doctrine\ORM\EntityManager; class ScreensTest extends AbstractBaseApiTestCase { + private ?EntityManager $entityManager; + + protected function setUp(): void + { + parent::setUp(); + + $kernel = self::bootKernel(); + + $this->entityManager = $kernel->getContainer() + ->get('doctrine') + ->getManager(); + } + public function testGetCollection(): void { $response = $this->getAuthenticatedClient('ROLE_ADMIN')->request('GET', '/v2/screens?itemsPerPage=5', ['headers' => ['Content-Type' => 'application/ld+json']]); @@ -168,12 +183,21 @@ public function testCreateInvalidScreen(): void public function testUpdateScreen(): void { + $playlistScreenRegionRepository = $this->entityManager->getRepository(PlaylistScreenRegion::class); + $playlistScreenRegionCountBefore = $playlistScreenRegionRepository->count([]); + + $playlistIri = $this->findIriBy(Playlist::class, ['title' => 'playlist_abc_3']); + $playlistUlid = $this->iriHelperUtils->getUlidFromIRI($playlistIri); + $regionIri = $this->findIriBy(ScreenLayoutRegions::class, ['title' => 'full']); + $regionUlid = $this->iriHelperUtils->getUlidFromIRI($regionIri); + $client = $this->getAuthenticatedClient('ROLE_ADMIN'); - $iri = $this->findIriBy(Screen::class, ['tenant' => $this->tenant]); + $iri = $this->findIriBy(Screen::class, ['title' => 'screen_abc_1']); - $client->request('PUT', $iri, [ + $response = $client->request('PUT', $iri, [ 'json' => [ 'title' => 'Updated title', + 'regions' => [['playlists' => [['id' => $playlistUlid, 'weight' => 0]], 'regionId' => $regionUlid]], ], 'headers' => [ 'Content-Type' => 'application/ld+json', @@ -185,7 +209,10 @@ public function testUpdateScreen(): void '@type' => 'Screen', '@id' => $iri, 'title' => 'Updated title', + 'regions' => ['/v2/screens/'.$response->toArray()['id'].'/regions/'.$regionUlid.'/playlists'], ]); + $playlistScreenRegionCountAfter = $playlistScreenRegionRepository->count([]); + $this->assertEquals($playlistScreenRegionCountBefore, $playlistScreenRegionCountAfter, 'PlaylistScreenRegion count should not change'); } public function testDeleteScreen(): void