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

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

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

Ralf, did you get a chance to test the current roundup head against your
use case?

Ralf, do you have an update on this?

Thanks.

-- rouilj

Just tested this, works for me, thanks for tracking this down.
Ralf