430 Fix bug with optional fields in custom forms#432
Conversation
| playwright: | ||
| name: "Playwright Tests" | ||
| runs-on: ubuntu-latest | ||
| runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
It looks like Playwright isn't running out of the box on Ubuntu 24.04 at the moment due to some missing dependencies.
We need the latest version to get around a breaking change in Starlette's test client: Kludex/starlette#2770
I'm pretty sure the type annotations on Starlette are wrong - passing a HTTPEndpoint in should be allowed.
| private_app.add_route( | ||
| path="/change-password/", | ||
| route=change_password( | ||
| route=change_password( # type: ignore |
There was a problem hiding this comment.
I'm pretty sure the Starlette type annotations are wrong here - we're just passing in a HTTPEndpoint.
| public_app.add_route( | ||
| path="/logout/", | ||
| route=session_logout(session_table=session_table), | ||
| route=session_logout(session_table=session_table), # type: ignore |
There was a problem hiding this comment.
See above about type annotations.
It would remove all of the defaults - we're better just letting Vue recreate the form.
admin_ui/src/utils.ts
Outdated
| // TODO We can potentially remove this in the future - isNullable does | ||
| // what we need. | ||
| value = null | ||
| } else if (isNullable(property) && value == "") { |
There was a problem hiding this comment.
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:
But need to decide whether it's a good idea or not.
There was a problem hiding this comment.
I don't really understand what the implication here is, maybe @sinisaos has some thoughts?
There was a problem hiding this comment.
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.
|
I also tested this PR against the latest Piccolo ORM PR and in my opinion everything works fine. |
|
I made this backwards compatible, so it doesn't require any changes to |
3 participants
Related to #430