[PUI] form error fix (#7689)

* Make initial data query wait until options query is complete

* Fix form error issues

- Form fields were being re-constructed

* Update playwright tests - check for form error message

* Prevent reconstruction of form fields

* Hide form elements until OPTIONS request is complete

* Fix for <ChoiceField />

- "value" must be stringified!

* Handle undefined choice values

* Add "batch code" to stock detail page

* Fix for initial focus

* Allow form field definition to change externally

* Force override of fetched data

* Update playwright tests

* Add backup value

* Cleanup initialdataquery

* Unit test updates

* Test updates

* Tweak API Form

* Adjust playwright test

src/frontend/package.json

@@ -52,7 +52,7 @@
         "dayjs": "^1.11.10",
         "embla-carousel-react": "^8.1.6",
         "html5-qrcode": "^2.3.8",
-        "mantine-datatable": "^7.11.1",
+        "mantine-datatable": "^7.11.2",
         "react": "^18.3.1",
         "react-dom": "^18.3.1",
         "react-grid-layout": "^1.4.4",

src/frontend/src/components/forms/ApiForm.tsx

@@ -249,6 +249,31 @@ export function ApiForm({
     [props.url, props.pk, props.pathParams]
   );
 
+  // Define function to process API response
+  const processFields = (fields: ApiFormFieldSet, data: NestedDict) => {
+    const res: NestedDict = {};
+
+    for (const [k, field] of Object.entries(fields)) {
+      const dataValue = data[k];
+
+      if (
+        field.field_type === 'nested object' &&
+        field.children &&
+        typeof dataValue === 'object'
+      ) {
+        res[k] = processFields(field.children, dataValue);
+      } else {
+        res[k] = dataValue;
+
+        if (field.onValueChange) {
+          field.onValueChange(dataValue, data);
+        }
+      }
+    }
+
+    return res;
+  };
+
   // Query manager for retrieving initial data from the server
   const initialDataQuery = useQuery({
     enabled: false,
@@ -261,79 +286,51 @@ export function ApiForm({
       props.pathParams
     ],
     queryFn: async () => {
-      try {
+      return await api
-        // Await API call
+        .get(url)
-        let response = await api.get(url);
+        .then((response: any) => {
+          // Process API response
+          const fetchedData: any = processFields(fields, response.data);
 
-        // Define function to process API response
+          // Update form values, but only for the fields specified for this form
-        const processFields = (fields: ApiFormFieldSet, data: NestedDict) => {
+          form.reset(fetchedData);
-          const res: NestedDict = {};
+          return fetchedData;
-
+        })
-          // TODO: replace with .map()
+        .catch(() => {
-          for (const [k, field] of Object.entries(fields)) {
+          return {};
-            const dataValue = data[k];
-
-            if (
-              field.field_type === 'nested object' &&
-              field.children &&
-              typeof dataValue === 'object'
-            ) {
-              res[k] = processFields(field.children, dataValue);
-            } else {
-              res[k] = dataValue;
-
-              if (field.onValueChange) {
-                field.onValueChange(dataValue, data);
-              }
-            }
-          }
-
-          return res;
-        };
-
-        // Process API response
-        const initialData: any = processFields(fields, response.data);
-
-        // Update form values, but only for the fields specified for this form
-        form.reset(initialData);
-
-        // Update the field references, too
-        Object.keys(fields).forEach((fieldName) => {
-          if (fieldName in initialData) {
-            let field = fields[fieldName] ?? {};
-            fields[fieldName] = {
-              ...field,
-              value: initialData[fieldName]
-            };
-          }
         });
-
-        return response;
-      } catch (error) {
-        console.error('ERR: Error fetching initial data:', error);
-        // Re-throw error to allow react-query to handle error
-        return {};
-      }
     }
   });
 
   useEffect(() => {
-    let _fields = props.fields ?? {};
+    let _fields: any = props.fields || {};
+    let _initialData: any = props.initialData || {};
+    let _fetchedData: any = initialDataQuery.data || {};
 
-    // Ensure default values override initial field spec
     for (const k of Object.keys(_fields)) {
+      // Ensure default values override initial field spec
       if (defaultValues[k]) {
         _fields[k].value = defaultValues[k];
       }
+
+      // Ensure initial data overrides default values
+      if (_initialData && _initialData[k]) {
+        _fields[k].value = _initialData[k];
+      }
+
+      // Ensure fetched data overrides also
+      if (_fetchedData && _fetchedData[k]) {
+        _fields[k].value = _fetchedData[k];
+      }
     }
 
     setFields(_fields);
-  }, [props.fields, defaultValues, initialDataQuery.data]);
+  }, [props.fields, props.initialData, defaultValues, initialDataQuery.data]);
 
   // Fetch initial data on form load
   useEffect(() => {
     // Fetch initial data if the fetchInitialData property is set
-    if (props.fetchInitialData) {
+    if (!optionsLoading && props.fetchInitialData) {
       queryClient.removeQueries({
         queryKey: [
           'form-initial-data',
@@ -346,22 +343,16 @@ export function ApiForm({
       });
       initialDataQuery.refetch();
     }
-  }, [props.fetchInitialData]);
+  }, [props.fetchInitialData, optionsLoading]);
 
-  const isLoading = useMemo(
+  const isLoading: boolean = useMemo(
     () =>
       isFormLoading ||
       initialDataQuery.isFetching ||
       optionsLoading ||
       isSubmitting ||
       !fields,
-    [
+    [isFormLoading, initialDataQuery, isSubmitting, fields, optionsLoading]
-      isFormLoading,
-      initialDataQuery.isFetching,
-      isSubmitting,
-      fields,
-      optionsLoading
-    ]
   );
 
   const [initialFocus, setInitialFocus] = useState<string>('');
@@ -381,7 +372,7 @@ export function ApiForm({
       });
     }
 
-    if (isLoading || initialFocus == focusField) {
+    if (isLoading) {
       return;
     }
 
@@ -533,6 +524,14 @@ export function ApiForm({
     props.onFormError?.();
   }, [props.onFormError]);
 
+  if (optionsLoading || initialDataQuery.isFetching) {
+    return (
+      <Paper mah={'65vh'}>
+        <LoadingOverlay visible zIndex={1010} />
+      </Paper>
+    );
+  }
+
   return (
     <Stack>
       <Boundary label={`ApiForm-${id}`}>
@@ -546,13 +545,15 @@ export function ApiForm({
             {/* Form Fields */}
             <Stack gap="sm">
               {(!isValid || nonFieldErrors.length > 0) && (
-                <Alert radius="sm" color="red" title={t`Error`}>
+                <Alert radius="sm" color="red" title={t`Form Error`}>
-                  {nonFieldErrors.length > 0 && (
+                  {nonFieldErrors.length > 0 ? (
                     <Stack gap="xs">
                       {nonFieldErrors.map((message) => (
                         <Text key={message}>{message}</Text>
                       ))}
                     </Stack>
+                  ) : (
+                    <Text>{t`Errors exist for one or more form fields`}</Text>
                   )}
                 </Alert>
               )}
@@ -570,23 +571,22 @@ export function ApiForm({
                 )}
               </Boundary>
               <Boundary label={`ApiForm-${id}-FormContent`}>
-                {!isLoading && (
+                <FormProvider {...form}>
-                  <FormProvider {...form}>
+                  <Stack gap="xs">
-                    <Stack gap="xs">
+                    {Object.entries(fields).map(([fieldName, field]) => {
-                      {!optionsLoading &&
+                      return (
-                        Object.entries(fields).map(([fieldName, field]) => (
+                        <ApiFormField
-                          <ApiFormField
+                          key={fieldName}
-                            key={fieldName}
+                          fieldName={fieldName}
-                            fieldName={fieldName}
+                          definition={field}
-                            definition={field}
+                          control={form.control}
-                            control={form.control}
+                          url={url}
-                            url={url}
+                          setFields={setFields}
-                            setFields={setFields}
+                        />
-                          />
+                      );
-                        ))}
+                    })}
-                    </Stack>
+                  </Stack>
-                  </FormProvider>
+                </FormProvider>
-                )}
               </Boundary>
               <Boundary label={`ApiForm-${id}-PostFormContent`}>
                 {props.postFormContent}

src/frontend/src/components/forms/fields/ChoiceField.tsx

@@ -1,6 +1,6 @@
 import { Select } from '@mantine/core';
 import { useId } from '@mantine/hooks';
-import { useCallback, useMemo } from 'react';
+import { useCallback, useEffect, useMemo } from 'react';
 import { FieldValues, UseControllerReturn } from 'react-hook-form';
 
 import { ApiFormFieldType } from './ApiFormField';
@@ -10,7 +10,8 @@ import { ApiFormFieldType } from './ApiFormField';
  */
 export function ChoiceField({
   controller,
-  definition
+  definition,
+  fieldName
 }: {
   controller: UseControllerReturn<FieldValues, any>;
   definition: ApiFormFieldType;
@@ -23,6 +24,8 @@ export function ChoiceField({
     fieldState: { error }
   } = controller;
 
+  const { value } = field;
+
   // Build a set of choices for the field
   const choices: any[] = useMemo(() => {
     let choices = definition.choices ?? [];
@@ -48,6 +51,14 @@ export function ChoiceField({
     [field.onChange, definition]
   );
 
+  const choiceValue = useMemo(() => {
+    if (!value) {
+      return '';
+    } else {
+      return value.toString();
+    }
+  }, [value]);
+
   return (
     <Select
       id={fieldId}
@@ -57,7 +68,7 @@ export function ChoiceField({
       {...field}
       onChange={onChange}
       data={choices}
-      value={field.value}
+      value={choiceValue}
       label={definition.label}
       description={definition.description}
       placeholder={definition.placeholder}

src/frontend/src/components/forms/fields/TextField.tsx

@@ -56,6 +56,9 @@ export default function TextField({
       error={error?.message}
       radius="sm"
       onChange={(event) => onTextChange(event.currentTarget.value)}
+      onBlur={(event) => {
+        onChange(event.currentTarget.value);
+      }}
       rightSection={
         value && !definition.required ? (
           <IconX size="1rem" color="red" onClick={() => onTextChange('')} />

src/frontend/src/forms/PartForms.tsx

@@ -183,6 +183,7 @@ export function usePartParameterFields(): ApiFormFieldSet {
         }
       },
       data: {
+        type: fieldType,
         field_type: fieldType,
         choices: fieldType === 'choice' ? choices : undefined,
         adjustValue: (value: any) => {

src/frontend/src/pages/stock/StockDetail.tsx

@@ -152,6 +152,12 @@ export default function StockDetail() {
         name: 'available_stock',
         label: t`Available`,
         icon: 'quantity'
+      },
+      {
+        type: 'text',
+        name: 'batch',
+        label: t`Batch Code`,
+        hidden: !stockitem.batch
       }
       // TODO: allocated_to_sales_orders
       // TODO: allocated_to_build_orders

src/frontend/src/tables/part/PartCategoryTable.tsx

@@ -77,6 +77,7 @@ export function PartCategoryTable({ parentId }: { parentId?: any }) {
     url: ApiEndpoints.category_list,
     title: t`New Part Category`,
     fields: partCategoryFields(),
+    focus: 'name',
     initialData: {
       parent: parentId
     },

src/frontend/src/tables/stock/StockLocationTable.tsx

@@ -98,6 +98,7 @@ export function StockLocationTable({ parentId }: { parentId?: any }) {
     url: ApiEndpoints.stock_location_list,
     title: t`Add Stock Location`,
     fields: stockLocationFields(),
+    focus: 'name',
     initialData: {
       parent: parentId
     },

src/frontend/tests/baseFixtures.ts

@@ -64,6 +64,8 @@ export const test = baseTest.extend({
           .indexOf(
             'Support for defaultProps will be removed from function components in a future major release'
           ) < 0 &&
+        msg.text() !=
+          'Failed to load resource: the server responded with a status of 400 (Bad Request)' &&
         url != 'http://localhost:8000/api/user/me/' &&
         url != 'http://localhost:8000/api/user/token/' &&
         url != 'http://localhost:8000/api/barcode/' &&

src/frontend/tests/pages/pui_part.spec.ts

@@ -203,7 +203,7 @@ test('PUI - Pages - Part - Parameters', async ({ page }) => {
 
   // Select the "Color" parameter template (should create a "choice" field)
   await page.getByLabel('related-field-template').fill('Color');
-  await page.getByText('Part color').click();
+  await page.getByRole('option', { name: 'Color Part color' }).click();
   await page.getByLabel('choice-field-data').click();
   await page.getByRole('option', { name: 'Green' }).click();
 

src/frontend/tests/pages/pui_scan.spec.ts

@@ -128,5 +128,4 @@ test('PUI - Pages - Index - Scan (General)', async ({ page }) => {
   await page.getByRole('button', { name: 'Toggle Fullscreen' }).click();
   await page.waitForTimeout(1000);
   await page.getByRole('button', { name: 'Toggle Fullscreen' }).click();
-  await page.waitForTimeout(1000);
 });

src/frontend/tests/pui_general.spec.ts

@@ -195,4 +195,16 @@ test('PUI - Company', async ({ page }) => {
   await page.getByRole('cell', { name: 'Carla Tunnel' }).waitFor();
   await page.getByRole('tab', { name: 'Attachments' }).click();
   await page.getByRole('tab', { name: 'Notes' }).click();
+
+  // Let's edit the company details
+  await page.getByLabel('action-menu-company-actions').click();
+  await page.getByLabel('action-menu-company-actions-edit').click();
+
+  await page.getByLabel('text-field-name').fill('');
+  await page.getByLabel('text-field-website').fill('invalid-website');
+  await page.getByRole('button', { name: 'Submit' }).click();
+
+  await page.getByText('This field may not be blank.').waitFor();
+  await page.getByText('Enter a valid URL.').waitFor();
+  await page.getByRole('button', { name: 'Cancel' }).click();
 });

src/frontend/tests/pui_settings.spec.ts

@@ -1,4 +1,4 @@
-import { test } from './baseFixtures.js';
+import { expect, test } from './baseFixtures.js';
 import { baseUrl } from './defaults.js';
 import { doQuickLogin } from './login.js';
 
@@ -48,7 +48,42 @@ test('PUI - Admin', async ({ page }) => {
   await page.getByRole('tab', { name: 'Label Templates' }).click();
   await page.getByRole('tab', { name: 'Report Templates' }).click();
   await page.getByRole('tab', { name: 'Plugins' }).click();
-  await page.getByRole('tab', { name: 'Machines' }).click();
+
+  // Adjust some "location type" items
+  await page.getByRole('tab', { name: 'Location Types' }).click();
+
+  // Edit first item
+  await page.getByLabel('row-action-menu-0').click();
+  await page.getByRole('menuitem', { name: 'Edit' }).click();
+  await expect(page.getByLabel('text-field-name')).toHaveValue('Room');
+  await expect(page.getByLabel('text-field-description')).toHaveValue('A room');
+  await page.getByLabel('text-field-name').fill('Large Room');
+  await page.waitForTimeout(500);
+  await page.getByLabel('text-field-description').fill('A large room');
+  await page.waitForTimeout(500);
+  await page.getByRole('button', { name: 'Submit' }).click();
+
+  // Edit second item
+  await page.getByLabel('row-action-menu-1').click();
+  await page.getByRole('menuitem', { name: 'Edit' }).click();
+  await expect(page.getByLabel('text-field-name')).toHaveValue('Box (Large)');
+  await expect(page.getByLabel('text-field-description')).toHaveValue(
+    'Large cardboard box'
+  );
+  await page.getByRole('button', { name: 'Cancel' }).click();
+
+  // Edit first item again (revert values)
+  await page.getByLabel('row-action-menu-0').click();
+  await page.getByRole('menuitem', { name: 'Edit' }).click();
+  await expect(page.getByLabel('text-field-name')).toHaveValue('Large Room');
+  await expect(page.getByLabel('text-field-description')).toHaveValue(
+    'A large room'
+  );
+  await page.getByLabel('text-field-name').fill('Room');
+  await page.waitForTimeout(500);
+  await page.getByLabel('text-field-description').fill('A room');
+  await page.waitForTimeout(500);
+  await page.getByRole('button', { name: 'Submit' }).click();
 });
 
 test('PUI - Admin - Unauthorized', async ({ page }) => {

src/frontend/tests/pui_tables.spec.ts

@@ -62,6 +62,4 @@ test('PUI - Tables - Columns', async ({ page }) => {
   // De-select some items
   await page.getByRole('menuitem', { name: 'Description' }).click();
   await page.getByRole('menuitem', { name: 'Stocktake' }).click();
-
-  await page.waitForTimeout(2500);
 });

src/frontend/yarn.lock

@@ -4356,10 +4356,10 @@ make-dir@^4.0.0:
   dependencies:
     semver "^7.5.3"
 
-mantine-datatable@^7.11.1:
+mantine-datatable@^7.11.2:
-  version "7.11.1"
+  version "7.11.2"
-  resolved "https://registry.yarnpkg.com/mantine-datatable/-/mantine-datatable-7.11.1.tgz#a77ab8fc151569998ae2ef479dc535f48694cb05"
+  resolved "https://registry.yarnpkg.com/mantine-datatable/-/mantine-datatable-7.11.2.tgz#800cf3b91158089616f905f257461683f018c73d"
-  integrity sha512-YpTdmk1rRHengIyqjS2QI2GOIVtVUvBRdLClGJPnT1Sallunhx7aCFJWsvm3FQJO6yriz+/DexgQaOt8hZ6GNQ==
+  integrity sha512-4TUBw/LXJF+S5DpES26c+0CDFfVwUsO5or2bChHBZqg04Hpoev87i/JvRpuNgzvqRJaZ/EKqkSCuc1ldOrFgWg==
 
 markdown-table@^3.0.0:
   version "3.0.3"