Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

414 Be able to download files via custom forms #416

Merged
merged 13 commits into from
Oct 3, 2024

Conversation

dantownsend
Copy link
Member

@dantownsend dantownsend commented Sep 30, 2024

Resolves #414

Remaining tasks:

  • Allow file downloads via custom forms
  • Add tests
  • Add docs

For a future PR:

  • Add a streaming file response option for large files

@dantownsend dantownsend added the enhancement New feature or request label Sep 30, 2024
piccolo_admin/example.py Outdated Show resolved Hide resolved
piccolo_admin/example.py Outdated Show resolved Hide resolved
@sinisaos
Copy link
Member

sinisaos commented Oct 1, 2024

@dantownsend I think this is a great feature. I left a couple of comments. Maybe it's for some other PR, but now I noticed that Pydantic validation errors not displayed well in UI.

error

@dantownsend
Copy link
Member Author

@dantownsend I think this is a great feature. I left a couple of comments. Maybe it's for some other PR, but now I noticed that Pydantic validation errors not displayed well in UI.

error

That's interesting - definitely needs improvement! Thanks for highlighting it.

@sinisaos
Copy link
Member

sinisaos commented Oct 2, 2024

@dantownsend Our parseErrorResponse utility function works fine, but the problem was in the responseType blob in the axios post request. When we use responseType blob, we need to use the text() method to read the content from the Blob. We need to replace this part of code

if (axios.isAxiosError(error) && error.response) {
this.errors = parseErrorResponse(
error.response.data,
error.response.status
)
}

with this

if (axios.isAxiosError(error) && error.response) {
    const responseData =
        error.request.responseType === "blob" &&
        error.response.data instanceof Blob &&
        error.response.data.type &&
        error.response.data.type
            .toLowerCase()
            .indexOf("json") != -1
            ? JSON.parse(await error.response.data.text())
            : error.response.data
    this.errors = parseErrorResponse(
        responseData,
        error.response.status
    )
}

After that we displayed the error messages nicely as before.

error

Hope that helps.

@dantownsend
Copy link
Member Author

@dantownsend Our parseErrorResponse utility function works fine, but the problem was in the responseType blob in the axios post request. When we use responseType blob, we need to use the text() method to read the content from the Blob. We need to replace this part of code

if (axios.isAxiosError(error) && error.response) {
this.errors = parseErrorResponse(
error.response.data,
error.response.status
)
}

with this

if (axios.isAxiosError(error) && error.response) {
    const responseData =
        error.request.responseType === "blob" &&
        error.response.data instanceof Blob &&
        error.response.data.type &&
        error.response.data.type
            .toLowerCase()
            .indexOf("json") != -1
            ? JSON.parse(await error.response.data.text())
            : error.response.data
    this.errors = parseErrorResponse(
        responseData,
        error.response.status
    )
}

After that we displayed the error messages nicely as before.

error

Hope that helps.

That's really helpful - thanks!

I wasn't sure what negative effects responseType blob would have - it's required for binary files to download correctly.

@sinisaos
Copy link
Member

sinisaos commented Oct 2, 2024

I wasn't sure what negative effects responseType blob would have - it's required for binary files to download correctly.

@dantownsend Exactly, responseType blob is required to download binaries correctly. With the changes from the previous comment, we downloaded the files correctly and formatted the error messages correctly.

@dantownsend
Copy link
Member Author

OK, I've added the blob -> JSON conversion logic now.

@dantownsend
Copy link
Member Author

I think the PR is done now.

It ended up being huge because I broke down example.py into multiple files because it was getting extremely large.

By breaking it up, it also means I can more easily include chunks of it in the Sphinx docs using literalinclude.

Warning - to run the app locally now, you do python -m piccolo_admin.example.app.

@dantownsend
Copy link
Member Author

@sinisaos Do you see anything else, or am I good to merge?

I know it's a massive PR, with a lot of noise in it.

@sinisaos
Copy link
Member

sinisaos commented Oct 3, 2024

@dantownsend I tried this PR and everything works and looks great. I have nothing to add and I don't see anything that would be a problem. It's great that you split example.py into several files in the example app. It's much more transparent.

@dantownsend
Copy link
Member Author

@sinisaos Cool, thanks a lot - I really appreciate it 👍

@dantownsend dantownsend merged commit 8279ec8 into master Oct 3, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Be able to download files via custom forms
2 participants