430 Fix bug with optional fields in custom forms (#432)

* fix bug with optional fields in custom forms

* try older Ubuntu version for Playwright tests

microsoft/playwright#30368

* fix linter errors

* remove fastapi version pin

We need the latest version to get around a breaking change in Starlette's test client:

Kludex/starlette#2770

* ignore linter errors

I'm pretty sure the type annotations on Starlette are wrong - passing a HTTPEndpoint in should be allowed.

* add logic to work out if form fields are optional

* added a mega form for testing

* fix a small bug with resetting the form

It would remove all of the defaults - we're better just letting Vue recreate the form.

* allow new line characters in form response

* update unit tests with new form

* rename form

* add a playwright test

* refactor so It's backwards compatible

Author: dantownsend

admin_ui/src/components/FormAdd.vue

@@ -10,7 +10,7 @@
 
         <div v-show="successMessage">
             <h1>{{ $t("Form submitted") }}</h1>
-            <p>{{ successMessage }}</p>
+            <p id="success_message">{{ successMessage }}</p>
             <ul>
                 <li>
                     <a href="#" @click.prevent="resetForm">{{
@@ -25,7 +25,7 @@
             </ul>
         </div>
 
-        <div v-show="!successMessage">
+        <div v-if="!successMessage">
             <FormErrors :errors="errors" v-if="errors.length > 0" />
 
             <form
@@ -93,8 +93,6 @@ export default defineComponent({
     },
     methods: {
         resetForm() {
-            const form = this.$refs.form as HTMLFormElement
-            form.reset()
             this.successMessage = null
             this.errors = []
         },
@@ -185,4 +183,8 @@ export default defineComponent({
 h1 {
     text-transform: capitalize;
 }
+
+p#success_message {
+    white-space: pre-wrap;
+}
 </style>

admin_ui/src/components/NewForm.vue

@@ -11,10 +11,11 @@
                 v-bind:columnName="String(columnName)"
                 v-bind:type="getType(property)"
                 v-bind:value="property.default"
+                v-bind:isNullable="isNullable(property)"
                 v-bind:timeResolution="
                     schema?.extra?.time_resolution[columnName]
                 "
-                v-bind:format="property.format"
+                v-bind:format="getFormat(property)"
             />
         </div>
     </div>
@@ -23,7 +24,7 @@
 <script lang="ts">
 import { defineComponent, type PropType } from "vue"
 import InputField from "./InputField.vue"
-import { type Schema, getType } from "@/interfaces"
+import { type Schema, getType, getFormat, isNullable } from "@/interfaces"
 
 export default defineComponent({
     props: {
@@ -37,7 +38,9 @@ export default defineComponent({
     },
     setup() {
         return {
-            getType
+            getFormat,
+            getType,
+            isNullable
         }
     }
 })

admin_ui/src/interfaces.ts

@@ -176,6 +176,11 @@ export const getType = (property: Property): string => {
     return (property.type || property.anyOf?.[0].type) as string
 }
 
+// Determines if the property is nullable based off the OpenAPI type.
+export const isNullable = (property: Property): boolean => {
+    return (property.anyOf ?? []).filter((i) => i.type == "null").length > 0
+}
+
 export const getFormat = (property: Property): string | undefined => {
     if (property.format) {
         return property.format

admin_ui/src/utils.ts

@@ -1,4 +1,9 @@
-import { type Schema, type OrderByConfig, getType } from "@/interfaces"
+import {
+    type Schema,
+    type OrderByConfig,
+    getType,
+    isNullable
+} from "@/interfaces"
 import router from "./router"
 import moment from "moment"
 
@@ -135,13 +140,25 @@ export function convertFormValue(params: {
 }): any {
     let { key, value, schema } = params
 
+    if (value == "null") {
+        return null
+    }
+
     const property = schema.properties[key]
 
-    if (value == "null") {
-        value = null
-    } else if (property.extra?.nullable && value == "") {
-        value = null
-    } else if (getType(property) == "array") {
+    const nullable = property.extra?.nullable
+
+    if (nullable == true && value == "") {
+        return null
+    }
+
+    // For Piccolo custom forms, there is no `extra` attribute, instead we
+    // have to look at the OpenAPI schema:
+    if (nullable == undefined && isNullable(property) && value == "") {
+        return null
+    }
+
+    if (getType(property) == "array") {
         value = JSON.parse(String(value))
     }
 

e2e/test_forms.py

@@ -2,7 +2,9 @@
 
 from piccolo_admin.example.forms.csv import FORM as CSV_FORM
 from piccolo_admin.example.forms.image import FORM as IMAGE_FORM
+from piccolo_admin.example.forms.nullable import FORM as NULLABLE_FORM
 
+from .conftest import BASE_URL
 from .pages import FormPage, LoginPage
 
 
@@ -54,3 +56,26 @@ def test_image_form(page: Page, dev_server):
 
     download = download_info.value
     assert download.suggested_filename == "movie_listings.jpg"
+
+
+def test_nullable_form(page: Page, dev_server):
+    """
+    Make sure a form with nullable fields can be submitted successfully.
+    """
+    login_page = LoginPage(page=page)
+    login_page.reset()
+    login_page.login()
+
+    form_page = FormPage(
+        page=page,
+        form_slug=NULLABLE_FORM.slug,
+    )
+    form_page.reset()
+
+    with page.expect_response(
+        lambda response: response.url
+        == f"{BASE_URL}/api/forms/nullable-fields/"
+        and response.request.method == "POST"
+        and response.status == 200
+    ):
+        form_page.submit_form()

piccolo_admin/example/forms/__init__.py

@@ -2,10 +2,12 @@
 from .csv import FORM as CSV_FORM
 from .email import FORM as EMAIL_FORM
 from .image import FORM as IMAGE_FORM
+from .nullable import FORM as MEGA_FORM
 
 FORMS = [
     CALCULATOR_FORM,
     CSV_FORM,
     EMAIL_FORM,
     IMAGE_FORM,
+    MEGA_FORM,
 ]

piccolo_admin/example/forms/nullable.py

@@ -0,0 +1,55 @@
+import datetime
+import typing as t
+
+from pydantic import BaseModel
+from starlette.requests import Request
+
+from piccolo_admin.endpoints import FormConfig
+
+
+class NullableFieldsModel(BaseModel):
+    """
+    Used for testing a wide variety of field types.
+    """
+
+    boolean_field: bool = True
+    boolean_field_nullable: t.Optional[bool] = None
+
+    float_field: float = 1.0
+    float_field_nullable: t.Optional[float] = None
+
+    integer_field: int = 1
+    integer_field_nullable: t.Optional[int] = None
+
+    string_field: str = "Hello world"
+    string_nullable: t.Optional[str] = None
+
+    list_field: t.List[str] = ["a", "b", "c"]
+    list_field_nullable: t.Optional[t.List[str]] = None
+
+    time_field: datetime.time = datetime.time(hour=12, minute=30)
+    time_field_nullable: t.Optional[datetime.time] = None
+
+    date_field: datetime.date = datetime.date(year=1999, month=12, day=31)
+    date_field_nullable: t.Optional[datetime.date] = None
+
+    datetime_field: datetime.datetime = datetime.datetime(
+        year=1999, month=12, day=31, hour=12, minute=30
+    )
+    datetime_field_nullable: t.Optional[datetime.datetime] = None
+
+    timedelta_field: datetime.timedelta = datetime.timedelta(hours=1)
+    timedelta_field_nullable: t.Optional[datetime.timedelta] = None
+
+
+async def handle_form(request: Request, data: NullableFieldsModel) -> str:
+    return data.model_dump_json(indent=4)
+
+
+FORM = FormConfig(
+    name="Nullable fields",
+    pydantic_model=NullableFieldsModel,
+    endpoint=handle_form,
+    description="Used for testing nullable fields.",
+    form_group="Test forms",
+)

tests/test_endpoints.py

@@ -279,6 +279,7 @@ def test_forms(self):
 
         response = client.get("/api/forms/")
         self.assertEqual(response.status_code, 200)
+
         self.assertEqual(
             response.json(),
             [
@@ -305,6 +306,11 @@ def test_forms(self):
                     "slug": "download-schedule",
                     "description": "Download the schedule for the day.",
                 },
+                {
+                    "description": "Used for testing nullable fields.",
+                    "name": "Nullable fields",
+                    "slug": "nullable-fields",
+                },
             ],
         )
 
@@ -522,6 +528,15 @@ def test_forms_grouped(self):
                             "slug": "booking-form",
                         }
                     ],
+                    "Test forms": [
+                        {
+                            "description": (
+                                "Used for testing nullable fields."
+                            ),
+                            "name": "Nullable fields",
+                            "slug": "nullable-fields",
+                        }
+                    ],
                 },
                 "ungrouped": [
                     {