Issue 2550929
Created on 2016-10-21 01:37 by rouilj, last changed 2016-11-28 11:59 by schlatterbeck.
Messages | |||
---|---|---|---|
msg5912 | Author: [hidden] (rouilj) | Date: 2016-10-21 01:37 | |
I emailed Ralf a couple of times, but I haven't heard back from him. I hope somebody else may know what the problem was that he was fixing. In my issue.item.html page, I have a field: msg-1@messagetype that is set by default. When the form is committed, there is a form element ('msg', '-1'): {'messagetype': '1'} If there is only a property change (e.g. add a new user to the nosy list, or set the status...) that does not include a change note, the error: Edit Error: 'content' is displayed. I bisected the code to figure out what changed and it looks like cc4f4ee46d88 makes my form fail. The diff for cc4f4ee46d88 is: diff -r 71643a839c80 -r cc4f4ee46d88 roundup/cgi/form_parser.py --- a/roundup/cgi/form_parser.py Thu Jul 14 22:03:48 2016 -0400 +++ b/roundup/cgi/form_parser.py Mon Jul 18 13:21:43 2016 +0200 @@ -594,11 +594,9 @@ # new item (any class) with no content - ignore del all_props[(cn, id)] elif isinstance(self.db.classes[cn], hyperdb.FileClass): - if id is not None and id.startswith('-'): - if not props.get('content', ''): - del all_props[(cn, id)] - elif props.has_key('content') and not props['content']: - raise FormError (self._('File is empty')) + # Avoid emptying the file + if props.has_key('content') and not props['content']: + del props ['content'] return all_props, all_links def parse_file(self, fpropdef, fprops, v): with the checkin comment of: Fix file attribute parsing Fix bug introduced when allowing multiple file attachments. With the original code (prefixed by -), the "msg -1" tuple in the form is removed because "msg -1" has no content property in the form. This I claim is correct operation in my case. With the new code, the "msg -1" property is passed through and something goes boom later on in the code. I am not quite sure what the failure was in the multiple file case. Reverting cc4f4ee46d88, I can upload 1 or 3 files at once using a "input type=file multiple" field. I don't have any empty files after the upload. I am tempted to just revert the change unless somebody can tell me what this patch was supposed to fix so I can reproduce the original problem and try to fix it. Comments? -- rouilj |
|||
msg5913 | Author: [hidden] (schlatterbeck) | Date: 2016-10-21 12:17 | |
Here my Mail that didn't make it to you: " Looks like the old code deleted *all* properties of a FileClass (which happens to match for messages, too). My new code only checks the case that the content is empty and removes an empty content property. So re-adding > - if id is not None and id.startswith('-'): > - del all_props[(cn, id)] probably should do the trick, I have no time to test this now, though. So I would change the existing code to: # Avoid emptying the file if props.has_key('content') and not props['content']: del props ['content'] if id is not None and id.startswith('-'): del all_props[(cn, id)] The problem I was fixing is that an item-template for "file" did no longer work for me. The changes above will not create a file/msg with an empty content in case some other properties are filled in but *will* allow changing properties for an already-existing file or message. But not tested :-) Let me know if you have time to test this, or if I should look into this " I've now committed this change (after verifying all tests pass). Please check if this fixes your problem and maybe you can add a test for this. Thanks Ralf |
|||
msg5914 | Author: [hidden] (rouilj) | Date: 2016-10-22 21:36 | |
Hi Ralf: Looks like your reply got caught in my spam trap I missed seeing it when I emptied my spam folder. Sorry about that. Your initial fix didn't work. I never have a content key since there is no message. As a result your if clause was never entered. While I was working on it, I identified three cases: 1) File exists but content is empty. Remove content from form so file contents are not clobbered. This is I believe your use case. 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. This is my original use case. 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). I tried uploading 0 length file during manual testing expecting it to fail and it did. So I got another use case 8-). How I handle 3 may be controversial but: we can't remove the content field as in case 1. New files MUST have a content field otherwise we generate the very unfriendly "Edit Error: 'content'" message to the user. We can't treat the entry as in case 2 and transparently remove the reference to the new file id when content is empty, This will stop the file from being created. But it will be confusing to the user as s/he won't see the (0 length) file created and there will be no feedback. I don't think adding: "your file xyzzy was not created" as part of an OK message is ok (even if I could figure out how to add that up to the UI). 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 you 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. I got annoyed at having to reselect files to upload every time I hit an error 8-). There are no tests as I am not sure how to make/run tests cases with alternate schemas required to make this testable. I checked my code changes in. Can you run your use case test against it and make sure it works. If it does, I'll close this ticket out. -- rouilj |
|||
msg5917 | Author: [hidden] (rouilj) | Date: 2016-11-03 00:36 | |
Ralf, did you get a chance to test the current roundup head against your use case? |
|||
msg5918 | Author: [hidden] (rouilj) | Date: 2016-11-28 00:32 | |
Ralf, do you have an update on this? Thanks. -- rouilj |
|||
msg5919 | Author: [hidden] (schlatterbeck) | Date: 2016-11-28 11:59 | |
Just tested this, works for me, thanks for tracking this down. Ralf |
History | |||
---|---|---|---|
Date | User | Action | Args |
2016-11-28 11:59:34 | schlatterbeck | set | status: open -> fixed resolution: fixed messages: + msg5919 |
2016-11-28 00:32:48 | rouilj | set | messages: + msg5918 |
2016-11-03 00:36:23 | rouilj | set | messages: + msg5917 |
2016-10-22 21:36:07 | rouilj | set | messages: + msg5914 |
2016-10-21 12:17:09 | schlatterbeck | set | status: new -> open nosy: + schlatterbeck messages: + msg5913 |
2016-10-21 01:37:58 | rouilj | create |