Merge pull request #1299 from danskernesdigitalebibliotek/release/202…

…4-26-0

Release/2024 26 0

docs/architecture/adr-008-error-handling.md

@@ -0,0 +1,59 @@
+# Architecture Decision Record: Error handling in the React Apps
+
+## Context
+
+We needed to handle errors thrown in the apps, both in requests by fetchers but
+also exceptions thrown at runtime.
+
+Errors were already handled in the initial implementation of this project.
+An [Error Boundary](https://react.dev/reference/react/use#displaying-an-error-to-users-with-error-boundary)
+was already implemented but we were lacking two important features:
+
+* Every app shouldn't show its error in their own scope. We wanted to centralise
+the error rendering for the end user
+* All errors should NOT be caught by the Error Boundary an thereby block the
+whole app.
+
+## Decision
+
+### Show the errors in one place
+
+To solve the problem with each app showing its own error, we decided to make use
+of [React's Portal system](https://react.dev/reference/react-dom/createPortal).
+The host (most likely dpl-cms) that includes the apps tells the apps via a
+config data prop what the container id of error wrapper is. Then the Error
+boundary system makes sure to use that id when rendering the error.
+
+### Handle errors differently depending on type
+
+Each app is wrapped with an Error Boundary. In the initial implementation
+that meant that if any exception was thrown the Error Boundary would catch
+any exception and showing an error message to the end user.
+Furthermore the error boundary makes sure the errors are being logged to `error.log`.
+
+Exceptions can be thrown in the apps at runtime both as a result
+of a failing request to a service or on our side.
+The change made to the error system in this context was to distinguish
+between the request errors.
+Some data for some services are being considered to be essential for the apps to
+work, others are not.
+To make sure that not all fetching errors are being caught we have created a
+`queryErrorHandler` in `src/components/store.jsx`. The `queryErrorHandler` looks
+at the type of error/instance of error that is being thrown
+and decides if the Error Boundary should be used or not.
+At the moment of this writing there are two type of errors: critical and non-critical.
+The critical ones are being caught by the Error Boundary and the non-critical
+are only ending up in the error log and are not blocking the app.
+
+## Consequences
+
+By using the Portal system we have solved the problem about showing multiple
+errors instead of a single global one.
+
+By choosing to distinguish different error types by looking at their instance name
+we can decide which fetch errors should be blocking the app and which should not.
+In both cases the errors are being logged and we can trace them in our logs.
+
+## Alternatives considered
+
+None.

src/apps/advanced-search/AdvancedSearch.tsx

@@ -30,7 +30,11 @@ const AdvancedSearch: React.FC<AdvancedSearchProps> = ({ pageSize }) => {
 
   // Only react on url parameters on the initial render.
   useEffectOnce(() => {
-    const advancedSearchQuery = getUrlQueryParam("advancedSearchQuery");
+    // We have to remove brackets if multiple filters were used so that we can
+    // translate the string back to an object.
+    const advancedSearchQuery = getUrlQueryParam("advancedSearchQuery")
+      ?.replace("(", "")
+      .replace(")", "");
     if (advancedSearchQuery) {
       // TODO: Add runtime validation
       // If the value does not match the type because of url tampering, type

src/apps/advanced-search/helpers.tsx

@@ -47,19 +47,28 @@ const translateFilterToCql = (
   filterToTranslate: MultiselectOption[],
   cqlKey: keyof typeof advancedSearchFilters
 ) => {
-  return filterToTranslate.reduce((acc: string, curr: MultiselectOption) => {
-    let filterTranslation = "";
-    const relation = acc.trim() === "" ? " AND" : " OR";
-    if (curr.value === "all") {
-      return `${acc}`;
-    }
-    filterTranslation = filterTranslation.concat(
-      relation,
-      ` ${advancedSearchFilters[cqlKey]}=`,
-      `'${curr.value}'`
-    );
-    return acc + filterTranslation;
-  }, "");
+  let translation = filterToTranslate.reduce(
+    (acc: string, curr: MultiselectOption) => {
+      let filterTranslation = "";
+      const relation = acc.trim() === "" ? " AND" : " OR";
+      if (curr.value === "all") {
+        return `${acc}`;
+      }
+      filterTranslation = filterTranslation.concat(
+        relation,
+        ` ${advancedSearchFilters[cqlKey]}=`,
+        `'${curr.value}'`
+      );
+      return acc + filterTranslation;
+    },
+    ""
+  );
+  // If multiple values are selected in a single filter, we need to wrap them in
+  // parentheses & add move the opening AND clause before the parenthesis opening.
+  if (filterToTranslate.length > 1) {
+    translation = ` AND (${translation.split(" AND")[1]})`;
+  }
+  return translation;
 };
 
 const translateFiltersToCql = (
@@ -86,12 +95,26 @@ const translateFiltersToCql = (
   return translatedFilters;
 };
 
+export const wrapFiltersInParentheses = (filters: string) => {
+  // No filters, no wrapping needed.
+  if (filters.trim() === "") {
+    return "";
+  }
+  // If there's only one clause, no wrapping is needed either.
+  if (!filters.includes(" OR ")) {
+    return filters;
+  }
+  // The filter string always start with " AND", so we can work with that.
+  const splitFiltersArray = filters.split(" AND", 2);
+  return `${splitFiltersArray.join(" AND (")})`;
+};
+
 export const translateSearchObjectToCql = (
   searchObject: AdvancedSearchQuery
 ) => {
   const rowsAsCql = translateRowsToCql(searchObject.rows);
   const filtersAsCql = translateFiltersToCql(searchObject.filters);
-  return rowsAsCql + filtersAsCql;
+  return `${rowsAsCql}${filtersAsCql}`;
 };
 
 export const shouldAdvancedSearchButtonBeDisabled = (

src/apps/reservation-list/modal/pause-reservation/pause-reservation.tsx

@@ -28,31 +28,24 @@ const PauseReservation: FC<PauseReservationProps> = ({ id, user }) => {
   const { close } = useModalButtonHandler();
   const { pauseReservation } = getModalIds();
   const saveFormId = useId();
-
   const currentDate = dayjs().format("YYYY-MM-DD");
   const [startDate, setStartDate] = useState<string>(currentDate);
   const [endDate, setEndDate] = useState<string>("");
   const pauseActive = user?.onHold?.from && user?.onHold?.to;
+  const [isLoading, setIsLoading] = useState(false);
 
   const save = useCallback(
     (localStartDate?: string, localEndDate?: string) => {
       if (!user) {
         return;
       }
-      // TODO: consolidate with the other save patron function
-      // be aware the defaults are not necessarily the same in the different save patron functions
-      const saveData = {
-        preferredPickupBranch: user.preferredPickupBranch,
-        receiveEmail: user.receiveEmail,
-        receivePostalMail: user.receivePostalMail,
-        receiveSms: user.receiveSms
-      } as Patron;
-
+      setIsLoading(true);
+      // TODO: Create a hook for this that fetches + updates user data.
+      const saveData = { ...user } as Patron;
       saveData.onHold = {
         from: localStartDate === "" ? undefined : localStartDate,
         to: localEndDate === "" ? undefined : localEndDate
       };
-
       mutate(
         {
           data: { patron: saveData }
@@ -62,10 +55,13 @@ const PauseReservation: FC<PauseReservationProps> = ({ id, user }) => {
             queryClient.invalidateQueries(
               getGetPatronInformationByPatronIdV2QueryKey()
             );
+            setIsLoading(false);
             close(pauseReservation as string);
           },
           // todo error handling, missing in figma
-          onError: () => {}
+          onError: () => {
+            setIsLoading(false);
+          }
         }
       );
     },
@@ -145,6 +141,7 @@ const PauseReservation: FC<PauseReservationProps> = ({ id, user }) => {
             type="submit"
             form={saveFormId}
             className="btn-primary btn-filled btn-small"
+            disabled={isLoading}
           >
             {t("pauseReservationModalSaveButtonLabelText")}
           </button>