UIPBEX-58: Add error checking for invalid config. (#118)

* Wrap 'useInitialValues' in try/catch and update test * Add better safety checks for 'dtoTo*' methods * Fix error * Log error Co-authored-by: Noah Overcash <[email protected]> --------- Co-authored-by: Noah Overcash <[email protected]>
folio-org · Apr 25, 2024 · fb9769f · fb9769f
1 parent 31c80a3
commit fb9769f
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 40 deletions.
diff --git a/src/api/dto/from/dtoToAggregateCriteria.test.ts b/src/api/dto/from/dtoToAggregateCriteria.test.ts
@@ -61,4 +61,16 @@ describe('dtoToAggregateCriteria', () => {
       },
     ],
   ])('with EUR dtoToAggregateCriteria(%s) === %s', (input, expected) => expect(dtoToAggregateCriteria(input, { currency: 'EUR' } as StripesType, intlEu)).toEqual(expected));
+
+  test.each<[BursarExportFilterAggregate | undefined, CriteriaAggregate | undefined]>([
+    [
+      {
+        type: 'Aggregate',
+        property: 'TOTAL_AMOUNT',
+        condition: 'GREATER_THAN',
+        amount: ('' as unknown) as number,
+      },
+      undefined
+    ],
+  ])('with undefined amount dtoToAggregateCriteria(%s) === %s', (input, expected) => expect(dtoToAggregateCriteria(input, { currency: 'USD' } as StripesType, intlEu)).toEqual(expected));
 });
diff --git a/src/api/dto/from/dtoToAggregateCriteria.ts b/src/api/dto/from/dtoToAggregateCriteria.ts
@@ -10,10 +10,13 @@ import { BursarExportFilterAggregate } from '../dto-types';
 
 // inverse of ../to/aggregateCriteriaToFilterDto
 export default function dtoToAggregateCriteria(
-  filter: BursarExportFilterAggregate | undefined,
+  filter: Partial<BursarExportFilterAggregate> | undefined,
   stripes: StripesType,
   intl: IntlShape,
 ): CriteriaAggregate | undefined {
+  if (!filter || typeof filter.property !== 'string' || typeof filter.condition !== 'string' || typeof filter.amount !== 'number') {
+    return undefined;
+  }
   switch (filter?.property) {
     case CriteriaTokenType.NUM_ROWS:
       return {

diff --git a/src/api/dto/from/dtoToCriteria.ts b/src/api/dto/from/dtoToCriteria.ts
@@ -17,7 +17,7 @@ import {
 } from '../dto-types';
 
 export function dtoToConditionCriteria(
-  filter: BursarExportFilterCondition,
+  filter: Partial<BursarExportFilterCondition>,
   feeFineTypes: FeeFineTypeDTO[],
   locations: LocationDTO[],
   stripes: StripesType,
@@ -27,38 +27,38 @@ export function dtoToConditionCriteria(
     return {
       type: CriteriaGroupType.ALL_OF,
       // eslint-disable-next-line @typescript-eslint/no-use-before-define
-      criteria: filter.criteria.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)),
+      criteria: filter.criteria?.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)) ?? [],
     };
   } else {
     return {
       type: CriteriaGroupType.ANY_OF,
       // eslint-disable-next-line @typescript-eslint/no-use-before-define
-      criteria: filter.criteria.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)),
+      criteria: filter.criteria?.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)) ?? [],
     };
   }
 }
 
 export function dtoToNegationCriteria(
-  filter: BursarExportFilterNegation,
+  filter: Partial<BursarExportFilterNegation>,
   feeFineTypes: FeeFineTypeDTO[],
   locations: LocationDTO[],
   stripes: StripesType,
   intl: IntlShape,
 ): CriteriaGroup {
   // NOR gets displayed as "None of" in the UI, so we flatten the inner OR
-  if (filter.criteria.type === 'Condition' && filter.criteria.operation === AndOrOperator.OR) {
+  if (filter.criteria?.type === 'Condition' && filter.criteria?.operation === AndOrOperator.OR) {
     return {
       type: CriteriaGroupType.NONE_OF,
       // eslint-disable-next-line @typescript-eslint/no-use-before-define
-      criteria: filter.criteria.criteria.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)),
+      criteria: filter.criteria.criteria?.map((criteria) => dtoToCriteria(criteria, feeFineTypes, locations, stripes, intl)) ?? [],
     };
   }
 
   // otherwise, just negate the single inner criteria
   return {
     type: CriteriaGroupType.NONE_OF,
     // eslint-disable-next-line @typescript-eslint/no-use-before-define
-    criteria: [dtoToCriteria(filter.criteria, feeFineTypes, locations, stripes, intl)],
+    criteria: filter.criteria ? [dtoToCriteria(filter.criteria, feeFineTypes, locations, stripes, intl)] : [],
   };
 }
 
@@ -91,7 +91,7 @@ export function getLocationProperties(
 
 // inverse of ../to/criteriaToFilterDto
 export default function dtoToCriteria(
-  filter: BursarExportFilterDTO,
+  filter: Partial<BursarExportFilterDTO>,
   feeFineTypes: FeeFineTypeDTO[],
   locations: LocationDTO[],
   stripes: StripesType,
@@ -102,13 +102,13 @@ export default function dtoToCriteria(
       return {
         type: CriteriaTerminalType.AGE,
         operator: filter.condition as ComparisonOperator,
-        numDays: filter.numDays.toString(),
+        numDays: filter.numDays?.toString() ?? '',
       };
     case CriteriaTerminalType.AMOUNT:
       return {
         type: CriteriaTerminalType.AMOUNT,
         operator: filter.condition as ComparisonOperator,
-        amountCurrency: intl.formatNumber(filter.amount / 100, { style: 'currency', currency: stripes.currency }),
+        amountCurrency: intl.formatNumber((filter.amount ?? 0) / 100, { style: 'currency', currency: stripes.currency }),
       };
     case CriteriaTerminalType.FEE_FINE_OWNER:
       return {
@@ -118,14 +118,14 @@ export default function dtoToCriteria(
     case CriteriaTerminalType.FEE_FINE_TYPE:
       return {
         type: CriteriaTerminalType.FEE_FINE_TYPE,
-        feeFineTypeId: filter.feeFineTypeId,
-        feeFineOwnerId: getFeeFineOwnerId(filter.feeFineTypeId, feeFineTypes),
+        feeFineTypeId: filter.feeFineTypeId ?? '',
+        feeFineOwnerId: getFeeFineOwnerId(filter.feeFineTypeId ?? '', feeFineTypes),
       };
     case CriteriaTerminalType.LOCATION:
       return {
         type: CriteriaTerminalType.LOCATION,
-        locationId: filter.locationId,
-        ...getLocationProperties(filter.locationId, locations),
+        locationId: filter.locationId ?? '',
+        ...getLocationProperties(filter.locationId ?? '', locations),
       };
     case CriteriaTerminalType.PATRON_GROUP:
       return {

diff --git a/src/hooks/useInitialValues.test.tsx b/src/hooks/useInitialValues.test.tsx
@@ -12,31 +12,50 @@ jest.mock('../api/queries');
 (useLocations as any).mockReturnValue({ isSuccess: false });
 (useTransferAccounts as any).mockReturnValue({ isSuccess: false });
 
-test('initial values hook', async () => {
-  const { result, rerender } = renderHook(() => useInitialValues(), {
-    wrapper: ({ children }: { children: React.ReactNode }) => withIntlConfiguration(<div>{children}</div>),
+describe('useInitialValues', () => {
+  test('initial values hook', async () => {
+    const { result, rerender } = renderHook(() => useInitialValues(), {
+      wrapper: ({ children }: { children: React.ReactNode }) => withIntlConfiguration(<div>{children}</div>),
+    });
+
+    await waitFor(() => expect(result.current).toBeNull());
+
+    (useCurrentConfig as any).mockReturnValue({ isSuccess: true });
+    rerender();
+    expect(result.current).toBeNull();
+
+    (useFeeFineTypes as any).mockReturnValue({ isSuccess: true });
+    rerender();
+    expect(result.current).toBeNull();
+
+    (useLocations as any).mockReturnValue({ isSuccess: true });
+    rerender();
+    expect(result.current).toBeNull();
+
+    (useTransferAccounts as any).mockReturnValue({ isSuccess: true });
+    rerender();
+    await waitFor(() => expect(result.current).toEqual('values'));
+
+    // should not change back
+    (useTransferAccounts as any).mockReturnValue({ isSuccess: false });
+    rerender();
+    await waitFor(() => expect(result.current).toEqual('values'));
   });
 
-  await waitFor(() => expect(result.current).toBeNull());
+  test('initial values hook with invalid data', async () => {
+    const { result, rerender } = renderHook(() => useInitialValues(), {
+      wrapper: ({ children }: { children: React.ReactNode }) => withIntlConfiguration(<div>{children}</div>),
+    });
 
-  (useCurrentConfig as any).mockReturnValue({ isSuccess: true });
-  rerender();
-  expect(result.current).toBeNull();
+    (useTransferAccounts as any).mockReturnValue({ isSuccess: false });
 
-  (useFeeFineTypes as any).mockReturnValue({ isSuccess: true });
-  rerender();
-  expect(result.current).toBeNull();
+    // should be null on fail
+    rerender();
+    await waitFor(() => expect(result.current).toBeNull());
 
-  (useLocations as any).mockReturnValue({ isSuccess: true });
-  rerender();
-  expect(result.current).toBeNull();
+    (useTransferAccounts as any).mockReturnValue({ isSuccess: true });
+    rerender();
 
-  (useTransferAccounts as any).mockReturnValue({ isSuccess: true });
-  rerender();
-  await waitFor(() => expect(result.current).toEqual('values'));
-
-  // should not change back
-  (useTransferAccounts as any).mockReturnValue({ isSuccess: false });
-  rerender();
-  await waitFor(() => expect(result.current).toEqual('values'));
+    await waitFor(() => expect(result.current).toEqual('values'));
+  });
 });
diff --git a/src/hooks/useInitialValues.ts b/src/hooks/useInitialValues.ts
@@ -29,10 +29,13 @@ export default function useInitialValues() {
     if (!currentConfig.isSuccess || !feeFineTypes.isSuccess || !locations.isSuccess || !transferAccounts.isSuccess) {
       return;
     }
-
-    setInitialValues(
-      dtoToFormValues(currentConfig.data, localeWeekdays, feeFineTypes.data, locations.data, transferAccounts.data, stripes, intl)
-    );
+    try {
+      setInitialValues(
+        dtoToFormValues(currentConfig.data, localeWeekdays, feeFineTypes.data, locations.data, transferAccounts.data, stripes, intl)
+      );
+    } catch (e) {
+      console.error('Unable to load initial values', e);
+    }
   }, [
     currentConfig.isSuccess,
     localeWeekdays,