Merge pull request os2display#368 from os2display/feature/v3-fix-screen-saving

V3: Fix screen saving

Author: tuj

CHANGELOG.md

@@ -28,6 +28,8 @@ All notable changes to this project will be documented in this file.
 - Aligned environment variable names.
 - Aligned with v. 2.6.0.
 - Added relations checksum feature flag.
+- Fixes saving issues described in issue where saving resulted in infinite spinner.
+- Fixed loading of routes containing null string values.
 
 ### NB! Prior to 3.x the project was split into separate repositories
 

assets/admin/components/screen/screen-form.jsx

@@ -206,7 +206,6 @@ function ScreenForm({
               <FormInput
                 name="location"
                 type="text"
-                required
                 label={t("screen-location-label")}
                 helpText={t("screen-location-placeholder")}
                 value={screen.location}

assets/admin/components/screen/screen-manager.jsx

@@ -1,4 +1,4 @@
-import { useEffect, useState } from "react";
+import { useEffect, useRef, useState } from "react";
 import { useTranslation } from "react-i18next";
 import { useNavigate } from "react-router-dom";
 import {
@@ -13,6 +13,15 @@ import {
 import idFromUrl from "../util/helpers/id-from-url";
 import { set } from "lodash/object";
 
+const orientationOptions = [
+  { title: "Vertikal", "@id": "vertical" },
+  { title: "Horisontal", "@id": "horizontal" },
+];
+const resolutionOptions = [
+  { title: "4K", "@id": "4K" },
+  { title: "HD", "@id": "HD" },
+];
+
 /**
  * The screen manager component.
  *
@@ -34,16 +43,8 @@ function ScreenManager({
   initialState = null,
 }) {
   const { t } = useTranslation("common", { keyPrefix: "screen-manager" });
-  const [saveWithoutClose, setSaveWithoutClose] = useState(false);
+  const saveWithoutCloseRef = useRef(false);
   const navigate = useNavigate();
-  const orientationOptions = [
-    { title: "Vertikal", "@id": "vertical" },
-    { title: "Horisontal", "@id": "horizontal" },
-  ];
-  const resolutionOptions = [
-    { title: "4K", "@id": "4K" },
-    { title: "HD", "@id": "HD" },
-  ];
   const headerText =
     saveMethod === "PUT" ? t("edit-screen-header") : t("create-screen-header");
   const [loadingMessage, setLoadingMessage] = useState("");
@@ -61,14 +62,6 @@ function ScreenManager({
     { data: postData, error: saveErrorPost, isSuccess: isSaveSuccessPost },
   ] = usePostV2ScreensMutation();
 
-  /** If the screen is saved, display the success message */
-  useEffect(() => {
-    if (isSaveSuccessPost || isSaveSuccessPut) {
-      displaySuccess(t("success-messages.saved-screen"));
-      setSavingScreen(false);
-    }
-  }, [isSaveSuccessPost, isSaveSuccessPut]);
-
   /** If the screen is saved with error, display the error message */
   useEffect(() => {
     if (saveErrorPut || saveErrorPost) {
@@ -78,14 +71,14 @@ function ScreenManager({
       );
       setSavingScreen(false);
     }
-  }, [saveErrorPut, saveErrorPost]);
+  }, [saveErrorPut, saveErrorPost, t]);
 
   /** If the screen is not loaded, display the error message */
   useEffect(() => {
     if (loadingError) {
       displayError(t("error-messages.load-screen-error", { id }), loadingError);
     }
-  }, [loadingError]);
+  }, [loadingError, id, t]);
 
   /**
    * Set state on change in input field
@@ -94,8 +87,7 @@ function ScreenManager({
    * @param {object} props.target - Event target.
    */
   const handleInput = ({ target }) => {
-    let localFormStateObject = { ...formStateObject };
-    localFormStateObject = JSON.parse(JSON.stringify(localFormStateObject));
+    const localFormStateObject = JSON.parse(JSON.stringify(formStateObject));
     set(localFormStateObject, target.id, target.value);
     setFormStateObject(localFormStateObject);
   };
@@ -126,9 +118,9 @@ function ScreenManager({
    *
    * @returns {Array | null} A mapped array with group ids or null
    */
-  function mapGroups() {
-    if (formStateObject.inScreenGroups) {
-      return formStateObject.inScreenGroups.map((group) => {
+  function mapGroups(screenState) {
+    if (screenState?.inScreenGroups instanceof Array) {
+      return screenState.inScreenGroups.map((group) => {
         return idFromUrl(group);
       });
     }
@@ -142,11 +134,9 @@ function ScreenManager({
    * @returns {Array | null} A mapped array with playlist ids and weight
    *   filtered by region id or null
    */
-  function getPlaylistsByRegionId(regionId) {
-    const { playlists } = formStateObject;
-
+  function getPlaylistsByRegionId(playlists, regionId) {
     return playlists
-      .filter(({ region }) => idFromUrl(region) === idFromUrl(regionId))
+      .filter(({ region }) => region === regionId)
       .map((playlist, index) => {
         return { id: idFromUrl(playlist["@id"]), weight: index };
       });
@@ -157,8 +147,9 @@ function ScreenManager({
    * @param {Array} array The array to remove from.
    */
   function removeFromArray(itemId, array) {
-    if (array.indexOf(itemId) >= 0) {
-      array.splice(array.indexOf(itemId), 1);
+    const index = array.indexOf(itemId);
+    if (index >= 0) {
+      array.splice(index, 1);
     }
   }
 
@@ -167,16 +158,19 @@ function ScreenManager({
    *
    * @returns {Array | null} A mapped array with playlist, regions and weight or null
    */
-  function mapPlaylistsWithRegion() {
+  function mapPlaylistsWithRegion(playlists, regions) {
     const returnArray = [];
-    const { playlists, regions } = formStateObject;
-    const regionIds = regions.map((r) => idFromUrl(r["@id"]));
+    const regionIds = regions.map((r) =>
+      typeof r === "string" ? idFromUrl(r, 1) : idFromUrl(r["@id"]),
+    );
 
     // The playlists all have a regionId, the following creates a unique list of relevant regions If there are not
     // playlists, then an empty playlist is to be saved per region
     let playlistRegions = [];
     if (playlists?.length > 0) {
-      playlistRegions = [...new Set(playlists.map(({ region }) => region))];
+      playlistRegions = [
+        ...new Set(playlists.map(({ region }) => idFromUrl(region))),
+      ];
     }
 
     // Then the playlists are mapped by region Looping through the regions that have a playlist connected...
@@ -187,19 +181,18 @@ function ScreenManager({
 
       // Add regionsId and connected playlists to the returnarray
       returnArray.push({
-        playlists: getPlaylistsByRegionId(regionId),
-        regionId: idFromUrl(regionId),
+        playlists: getPlaylistsByRegionId(playlists, regionId),
+        regionId,
       });
     });
+
     // The remaining regions are added with empty playlist arrays.
-    if (regionIds.length > 0) {
-      regionIds.forEach((regionId) =>
-        returnArray.push({
-          playlists: [],
-          regionId: idFromUrl(regionId),
-        }),
-      );
-    }
+    regionIds.forEach((regionId) =>
+      returnArray.push({
+        playlists: [],
+        regionId,
+      }),
+    );
     return returnArray;
   }
 
@@ -208,25 +201,26 @@ function ScreenManager({
    *
    * @returns {string} Orientation or empty string
    */
-  function getOrientation() {
-    const { orientation } = formStateObject;
-    return orientation ? orientation[0]["@id"] : "";
+  function getOrientation(screenState) {
+    const { orientation } = screenState;
+    return orientation && orientation.length > 0 ? orientation[0]["@id"] : "";
   }
 
   /**
    * Gets resolution for submitting
    *
    * @returns {string} Resolution or empty string
    */
-  function getResolution() {
-    const { resolution } = formStateObject;
+  function getResolution(screenState) {
+    const { resolution } = screenState;
     return resolution && resolution.length > 0 ? resolution[0]["@id"] : "";
   }
 
   /** Handles submit. */
   const handleSubmit = () => {
     setSavingScreen(true);
     setLoadingMessage(t("loading-messages.saving-screen"));
+
     const localFormStateObject = JSON.parse(JSON.stringify(formStateObject));
     const {
       title,
@@ -249,34 +243,36 @@ function ScreenManager({
         layout,
         location,
         enableColorSchemeChange,
-        resolution: getResolution(),
-        groups: mapGroups(),
-        orientation: getOrientation(),
-        regions: mapPlaylistsWithRegion(),
+        resolution: getResolution(localFormStateObject),
+        groups: mapGroups(localFormStateObject),
+        orientation: getOrientation(localFormStateObject),
+        regions: mapPlaylistsWithRegion(
+          localFormStateObject.playlists,
+          localFormStateObject.regions,
+        ),
       }),
     };
 
-    setLoadingMessage(t("loading-messages.saving-screen"));
-
     if (saveMethod === "POST") {
       PostV2Screens(saveData);
     } else if (saveMethod === "PUT") {
       PutV2Screens({ ...saveData, id });
     }
   };
 
-  const handleSubmitWithRedirect = () => {
-    setSaveWithoutClose(true);
+  const handleSaveAndStay = () => {
+    saveWithoutCloseRef.current = true;
     handleSubmit();
   };
 
   /** Handle submitting is done. */
   useEffect(() => {
-    if (isSaveSuccessPut || isSaveSuccessPost) {
+    if (isSaveSuccessPost || isSaveSuccessPut) {
+      displaySuccess(t("success-messages.saved-screen"));
       setSavingScreen(false);
 
-      if (saveWithoutClose) {
-        setSaveWithoutClose(false);
+      if (saveWithoutCloseRef.current) {
+        saveWithoutCloseRef.current = false;
 
         if (isSaveSuccessPost) {
           navigate(`/screen/edit/${idFromUrl(postData["@id"])}`);
@@ -285,13 +281,13 @@ function ScreenManager({
         navigate("/screen/list");
       }
     }
-  }, [isSaveSuccessPut, isSaveSuccessPost]);
+  }, [isSaveSuccessPut, isSaveSuccessPost, postData, navigate, t]);
 
   return (
     <>
       {formStateObject && (
         <ScreenForm
-          handleSubmitWithoutRedirect={handleSubmitWithRedirect}
+          handleSubmitWithoutRedirect={handleSaveAndStay}
           screen={formStateObject}
           orientationOptions={orientationOptions}
           resolutionOptions={resolutionOptions}

assets/admin/components/screen/util/campaign-icon.jsx

@@ -28,10 +28,10 @@ function CampaignIcon({ id, delay = 1000 }) {
 
   const { data: campaigns, isLoading } = useGetV2ScreensByIdCampaignsQuery(
     { id },
-    { skip: !getData },
+    { skip: !getData || !id },
   );
   const { data: groups, isLoading: isLoadingScreenGroups } =
-    useGetV2ScreensByIdScreenGroupsQuery({ id }, { skip: !getData });
+    useGetV2ScreensByIdScreenGroupsQuery({ id }, { skip: !getData || !id });
 
   useEffect(() => {
     if (campaigns) {

assets/admin/components/util/fetch-data-hook.js

@@ -10,6 +10,10 @@ function useFetchDataHook(apiCall, ids, params = {}, key = "id") {
   useEffect(() => {
     if (!ids || ids.length === 0) return;
 
+    // Filter out null/undefined/empty IDs
+    const validIds = ids.filter((id) => id != null && id !== "");
+    if (validIds.length === 0) return;
+
     async function fetchItems() {
       setLoading(true);
 

assets/admin/components/util/helpers/id-from-url.jsx

@@ -5,7 +5,7 @@
  */
 function idFromUrl(string, index = 0) {
   if (typeof string === "string") {
-    return string.match(/[A-Za-z0-9]{26}/g)[index];
+    return (string.match(/[A-Za-z0-9]{26}/g) ?? [])[index] ?? "";
   }
   return "";
 }