Mercurial > p > roundup > code
comparison roundup/cgi/form_parser.py @ 5171:5b63cfc95255
issue2550929 fix for my use case, need Ralf to check for his.
While working from Ralf's latest notes, I identified three cases:
1) File exists but content is empty.
Remove content from form so file contents are not clobbered.
2) New file to be created (id is -1, -2 ...) but there is no
content field defined in form. Remove all fields referencing the
new id.
3) New file to be created (id is -1, -2 ...) and there is a content
field defined in form. Let it be processed normally with no
changes. Do this even if the content value is empty (allow
uploading empty files).
3 may be controversial.
We can't remove the content field as in 1. For new files there
MUST be a content field otherwise we generate the "Edit Error:
'content'" message to the user.
We can't treating the entry as in 2 and transparently remove the
reference to the new file id when content is empty, This will stop the
file from being created and will be confusing as the user won't see the
(0 length) file created and there will be no feedback.
We could:
Return an error to the user telling them they can't upload an
empty file.
But why ask the user to resubmit just because of a 0 length file?
Also although Ralf did a great job retaining all form values on an
error, it looks like the file upload element of the form doesn't get
it's values retained. (Indeed this may be impossible to do. I don't
want the server to be able to create a hidden file upload button
pre-filled with file names just waiting for me to submit the form.)
So accepting the 0 length file I think is the best alternative.
There are no tests as I am not sure how to make/run tests cases with
alternate schemas required to make this testable.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sat, 22 Oct 2016 17:23:18 -0400 |
| parents | 2ae3954e972f |
| children | 270003714e5f |
comparison
equal
deleted
inserted
replaced
| 5170:2ae3954e972f | 5171:5b63cfc95255 |
|---|---|
| 592 for (cn, id), props in all_props.items(): | 592 for (cn, id), props in all_props.items(): |
| 593 if id is not None and id.startswith('-') and not props: | 593 if id is not None and id.startswith('-') and not props: |
| 594 # new item (any class) with no content - ignore | 594 # new item (any class) with no content - ignore |
| 595 del all_props[(cn, id)] | 595 del all_props[(cn, id)] |
| 596 elif isinstance(self.db.classes[cn], hyperdb.FileClass): | 596 elif isinstance(self.db.classes[cn], hyperdb.FileClass): |
| 597 # Avoid emptying the file | 597 # three cases: |
| 598 if props.has_key('content') and not props['content']: | 598 # id references existng file. If content is empty, |
| 599 del props ['content'] | 599 # remove content from form so we don't wipe |
| 600 # existing file contents. | |
| 601 # id is -1, -2 ... I.E. a new file. | |
| 602 # if content is not defined remove all fields that | |
| 603 # reference that file. | |
| 604 # if content is defined, let it pass through even if | |
| 605 # content is empty. Yes people can upload/create | |
| 606 # empty files. | |
| 607 print("props are %r",props) | |
| 608 print("class is: %s",cn) | |
| 609 if props.has_key('content'): | |
| 610 if id is not None and \ | |
| 611 not id.startswith('-') and \ | |
| 612 not props['content']: | |
| 613 # This is an existing file with emtpy content | |
| 614 # value in the form. | |
| 615 del props ['content'] | |
| 616 else: | |
| 617 # this is a new file without any content property. | |
| 600 if id is not None and id.startswith('-'): | 618 if id is not None and id.startswith('-'): |
| 601 del all_props[(cn, id)] | 619 del all_props[(cn, id)] |
| 620 # if this is a new file with content (even 0 length content) | |
| 621 # allow it through and create the zero length file. | |
| 602 return all_props, all_links | 622 return all_props, all_links |
| 603 | 623 |
| 604 def parse_file(self, fpropdef, fprops, v): | 624 def parse_file(self, fpropdef, fprops, v): |
| 605 # try to determine the file content-type | 625 # try to determine the file content-type |
| 606 fn = v.filename.split('\\')[-1] | 626 fn = v.filename.split('\\')[-1] |