Feature/bug screen playlist region no coding standards

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)

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>

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"

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);

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');
+        }
+    }
 }