Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/playwright.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ on:
jobs:
playwright:
name: "Playwright Tests"
runs-on: ubuntu-latest
runs-on: ubuntu-22.04
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Playwright isn't running out of the box on Ubuntu 24.04 at the moment due to some missing dependencies.

microsoft/playwright#30368

steps:
- name: "Checkout repository"
uses: "actions/checkout@v4"
Expand Down
10 changes: 6 additions & 4 deletions admin_ui/src/components/FormAdd.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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">{{
Expand All @@ -25,7 +25,7 @@
</ul>
</div>

<div v-show="!successMessage">
<div v-if="!successMessage">
<FormErrors :errors="errors" v-if="errors.length > 0" />

<form
Expand Down Expand Up @@ -93,8 +93,6 @@ export default defineComponent({
},
methods: {
resetForm() {
const form = this.$refs.form as HTMLFormElement
form.reset()
this.successMessage = null
this.errors = []
},
Expand Down Expand Up @@ -185,4 +183,8 @@ export default defineComponent({
h1 {
text-transform: capitalize;
}

p#success_message {
white-space: pre-wrap;
}
</style>
9 changes: 6 additions & 3 deletions admin_ui/src/components/NewForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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: {
Expand All @@ -37,7 +38,9 @@ export default defineComponent({
},
setup() {
return {
getType
getFormat,
getType,
isNullable
}
}
})
Expand Down
4 changes: 4 additions & 0 deletions admin_ui/src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,10 @@ export const getType = (property: Property): string => {
return (property.type || property.anyOf?.[0].type) as string
}

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
Expand Down
11 changes: 10 additions & 1 deletion admin_ui/src/utils.ts
Original file line number Diff line number Diff line change
@@ -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"

Expand Down Expand Up @@ -140,6 +145,10 @@ export function convertFormValue(params: {
if (value == "null") {
value = null
} else if (property.extra?.nullable && value == "") {
// TODO We can potentially remove this in the future - isNullable does
// what we need.
value = null
} else if (isNullable(property) && value == "") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic with the current version of Piccolo - in the Pydantic model for tables, they default to optional, unless required=True.

This PR fixes it:

piccolo-orm/piccolo#1141

But need to decide whether it's a good idea or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what the implication here is, maybe @sinisaos has some thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks both for testing it.

I'll change this slightly, so it's completely backwards compatible - i.e. the current behaviour for tables, and for custom forms it uses isNullable.

value = null
} else if (getType(property) == "array") {
value = JSON.parse(String(value))
Expand Down
25 changes: 25 additions & 0 deletions e2e/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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()
8 changes: 4 additions & 4 deletions piccolo_admin/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,13 @@ class GroupItem(BaseModel):


class GroupedTableNamesResponseModel(BaseModel):
grouped: t.Dict[str, t.List[str]] = Field(default_factory=list)
grouped: t.Dict[str, t.List[str]] = Field(default_factory=dict)
ungrouped: t.List[str] = Field(default_factory=list)


class GroupedFormsResponseModel(BaseModel):
grouped: t.Dict[str, t.List[FormConfigResponseModel]] = Field(
default_factory=list
default_factory=dict
)
ungrouped: t.List[FormConfigResponseModel] = Field(default_factory=list)

Expand Down Expand Up @@ -680,7 +680,7 @@ def __init__(

private_app.add_route(
path="/change-password/",
route=change_password(
route=change_password( # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the Starlette type annotations are wrong here - we're just passing in a HTTPEndpoint.

login_url="./../../public/login/",
session_table=session_table,
read_only=read_only,
Expand Down Expand Up @@ -786,7 +786,7 @@ def __init__(

public_app.add_route(
path="/logout/",
route=session_logout(session_table=session_table),
route=session_logout(session_table=session_table), # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about type annotations.

methods=["POST"],
)

Expand Down
2 changes: 2 additions & 0 deletions piccolo_admin/example/forms/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]
55 changes: 55 additions & 0 deletions piccolo_admin/example/forms/nullable.py
Original file line number Diff line number Diff line change
@@ -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",
)
8 changes: 2 additions & 6 deletions requirements/test-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,5 @@ flake8==7.0.0
piccolo[postgres,sqlite]>=1.16.0
playwright==1.41.2
pytest-playwright==0.4.4
httpx>=0.20.0
# This is needed because newer versions of FastAPI use a Starlette version with
# a `TestClient` which breaks rate limiting.
# The changes to `TestClient` will likely be reverted in a future release, in
# which case we can remove this version pin.
fastapi==0.106.0
httpx==0.28.1
fastapi==0.115.6
15 changes: 15 additions & 0 deletions tests/test_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ def test_forms(self):

response = client.get("/api/forms/")
self.assertEqual(response.status_code, 200)

self.assertEqual(
response.json(),
[
Expand All @@ -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",
},
],
)

Expand Down Expand Up @@ -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": [
{
Expand Down
Loading