Merge pull request #220 from os2display/feature/bug-screen-playlist-r…

…egion-no-coding-standards Feature/bug screen playlist region no coding standards
os2display · Oct 25, 2024 · 50d064c · 50d064c
2 parents 2df47b4 + 8b605c8
commit 50d064c
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 55 deletions.
diff --git 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
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<files psalm-version="5.22.2@d768d914152dbbf3486c36398802f74e80cfde48">
+<files psalm-version="5.26.1@d747f6500b38ac4f7dfc5edbcae6e4b637d7add0">
   <file src="src/Command/Feed/CreateFeedSourceCommand.php">
     <RiskyTruthyFalsyComparison>
       <code><![CDATA[$ulid]]></code>
@@ -525,11 +525,6 @@
       <code><![CDATA[$value]]></code>
     </MissingParamType>
   </file>
-  <file src="src/Security/AzureOidcAuthenticator.php">
-    <RiskyTruthyFalsyComparison>
-      <code><![CDATA[empty($signInName)]]></code>
-    </RiskyTruthyFalsyComparison>
-  </file>
   <file src="src/Security/Voter/UserVoter.php">
     <MissingTemplateParam>
       <code><![CDATA[UserVoter]]></code>
@@ -565,7 +560,6 @@
     </LessSpecificReturnStatement>
     <MissingParamType>
       <code><![CDATA[$data]]></code>
-      <code><![CDATA[$object]]></code>
     </MissingParamType>
     <MissingTemplateParam>
       <code><![CDATA[ProcessorInterface]]></code>
@@ -670,21 +664,12 @@
     </PossiblyNullPropertyAssignmentValue>
   </file>
   <file src="src/State/ScreenProcessor.php">
-    <NullableReturnStatement>
+    <InvalidReturnStatement>
       <code><![CDATA[$screen]]></code>
-    </NullableReturnStatement>
-    <PossiblyNullReference>
-      <code><![CDATA[setCreatedBy]]></code>
-      <code><![CDATA[setDescription]]></code>
-      <code><![CDATA[setEnableColorSchemeChange]]></code>
-      <code><![CDATA[setLocation]]></code>
-      <code><![CDATA[setModifiedBy]]></code>
-      <code><![CDATA[setOrientation]]></code>
-      <code><![CDATA[setResolution]]></code>
-      <code><![CDATA[setScreenLayout]]></code>
-      <code><![CDATA[setSize]]></code>
-      <code><![CDATA[setTitle]]></code>
-    </PossiblyNullReference>
+    </InvalidReturnStatement>
+    <InvalidReturnType>
+      <code><![CDATA[Screen]]></code>
+    </InvalidReturnType>
   </file>
   <file src="src/State/ScreenProvider.php">
     <PossiblyNullPropertyAssignmentValue>

diff --git 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
@@ -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
@@ -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
@@ -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