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

Add MultipleFileField to handle and clean multiple files #465

Merged
merged 6 commits into from May 10, 2023

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented May 3, 2023

Fixes #464

This adds the update suggested at the last link below to conform to the security release below:

#410
https://www.djangoproject.com/weblog/2023/may/03/security-releases/
django/django@eed53d0

To test:

  • Build should be green (using latest Django 3.2.19)
  • Multiple file upload should still be working
  • When uploading multiple files to file annotations, ALL files should be validated (e.g. to prevent empty files to be uploaded) instead of only the last file being validated.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

See #466 which exposes another underlying issue related to the validation of the files uploaded via the FilesAnnotationForm (and allows to create a scenario to test this PR).
While testing the two PRs together, it should be noted that this PR requires Django 3.2.19 as the API for auto-setting multiple: true when allow_multiple_selected is set to True was introduced in django/django@eed53d0. In other terms, with this PR and Django 3.2.18, the multiple selection is disabled. Django should be set to 3.2.19 as the minimum version via setup.py

@will-moore
Copy link
Member Author

will-moore commented May 4, 2023

Excluding to allow testing of #466 ahead of this...

EDIT: remove flag

@will-moore
Copy link
Member Author

Testing on merge-ci:

  • upload 1 or more non-empty files works -> Creates FileAnnoations
  • upload multiple Files which includes an empty file first, last or middle (if 3) fails silently - (form is invalid).

@@ -62,6 +62,28 @@
) % help_button


#################################################################
# Custom widget and validation for multiple file uploads
Copy link
Member

@sbesson sbesson May 5, 2023

Choose a reason for hiding this comment

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

Having looked at how the code is organized, all custom widget/fields are declared omeroweb.webclient.custom_forms and imported here to construct the webclient forms.



class MultipleFileInput(forms.ClearableFileInput):
allow_multiple_selected = True
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding a comment and/or a reference to the documentation explaining why we are subclassing these Django classes.

@dominikl
Copy link
Member

dominikl commented May 5, 2023

Functional test looks good 👍 If there's one empty file included in a multi file upload then none of the files get uploaded/attached, not even the non-empty ones. Is that expected?

@sbesson
Copy link
Member

sbesson commented May 5, 2023

@dominikl that's the expectation as per the new field class. If any of the files fails validation, then the entire multi-file upload is cancelled.

The other place that uses a FileField is the photo upload in My account and this shows an informative message if an empty file is uploaded

Screenshot 2023-05-05 at 12 58 02

@will-moore is there a way to propagate the error message to the user in the files annotation UI?

@will-moore
Copy link
Member Author

@dominikl - yes the whole form is considered invalid if any file is invalid.
We don't have any handling of invalid forms here (same as other annotations). Could add that, but probably not essential in this PR?

@knabar knabar added this to the 5.20.0 milestone May 5, 2023
@dominikl
Copy link
Member

dominikl commented May 5, 2023

👍 I don't think either that it has to go into this PR, better get it out as soon as possible.

@jburel
Copy link
Member

jburel commented May 9, 2023

omeroweb/webclient/custom_forms.py:385:89: E501 line too long (94 > 88 characters)

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

The last commit follows the new recommendation from Django documentation with a new custom class and restores compatibility with Django 3.2.19.

The last commit will need to be retested functionally but from a code review perspective, no objection to moving forward with the current suggestion.

Copy link
Member

@knabar knabar left a comment

Choose a reason for hiding this comment

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

Code looks good to me

@pwalczysko
Copy link
Member

Tested on merge-ci, user-3. From jenkins, this PR is included on https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-python-superbuild-push/1447/console

Merged PRs:
  - PR 425 will-moore 'Add hook for mapr or other app to extend webclient main page'
  - PR 456 kkoz 'Allow Tiled Image Histograms'
  - PR 462 will-moore 'OME.formatDate handles null input'
  - PR 465 will-moore 'Add MultipleFileField to handle and clean multiple files'
  - PR 467 knabar 'Add change log for 5.20.0'
  - PR 469 will-moore 'Temp setGroupForSession to edit current group'

The test:

  1. Create an empty file using touch on the cli and try to attach it in the Attachments harmonica of webclient.
  2. Try to attach the empty file also with other, non-empty files
  3. Try to upload the empty file as a photo in user settings of web UI

The tests went as expected, the file upload was rejected in all cases where the empty file was selected, either with non-empty files or alone.
The upload of photos and files (non-empty ones) still works.

LGTM

@jburel jburel merged commit 2e1b020 into ome:master May 10, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility with latest Django 3.2.19 security release
6 participants