Changeset 6613 seems to have broken edit inline

[Øyvind Saltvik <oyvind@…>]

All edit_inlines have started being required; i.e. when saving, and the rest of the blank rows get validation errors saying they need to be filled in.

[Karen Tracey <kmtracey@…>]

Yes, I have the same problem with newforms-admin branch after the 6613/4 merge. Specifically the problem seems to be with having picked up the fix for #5104. Now checkboxes, if they are not checked, are returning a value of False instead of None from value_from_datadict. But this makes BaseForm.is_empty (a newforms-admin specific function? I can't find it on trunk...) think that the form is not entirely empty, leading to the validation errors instead of just ignoring the empty edit-inline forms. Sorry, though, I have no idea what the correct fix might be.

[Brian Rosner]

I can verify this is a problem too. I have currently working on a fix since I need it for work. If someone else can get to it faster that would be great. Good catch Karen, it does look like like adding False to the list of checks:

​http://code.djangoproject.com/browser/django/branches/newforms-admin/django/newforms/forms.py#L197

However, I haven't tested that to know if it works. Will do so tomorrow.

[Karen Tracey <kmtracey@…>]

Yes, adding False to that list of "empty" values does fix the problem, in that the extra empty edit-inline items are not flagged as invalid. I just don't know if there are any other field types that might return False where False is not equivalent to "not specified". If there are, then the fix would make is_empty potentially start returning True for forms that are not entirely empty. Perhaps not likely (you'd need a form whose only specified field was this hypothetical set-to-False one)...but given my unfamiliarity with forms and fields it seems possible. Or maybe I'm just imagining the potential problem....

[Brian Rosner]

Ah, that is a good point I had not thought of ;) Well, it looks like this is now finally addressing another problem with the InlineFormSets which should not be displaying a deletion checkbox for add forms. That looks to be the correct fix as well as fixing that issue in general. I will get to work on a patch now.

[jkocherhans]

After thinking about this for a couple of days, I think an is_empty method on fields is the way to go, and then the form level is_empty would call field.is_empty(). The current implementation of that method is a total hack and now is probably a good time to clean it up. I won't be able to get to it for awhile, but it should be a fairly straightforward (but lengthy) patch if someone else wants to take it on. I would be happy to field questions.

I'll try to get my form_for_queryset patch + tests committed in the next couple of days so people can add to that test suite instead of creating a new one.

[Brian Rosner]

Wouldn't it make more sense to put the is_empty method on the individual widgets?

[jkocherhans]

Actually, yes. That would make a lot more sense. Maybe I should look at code before writing replies instead of waving my hands around. :)

[Karen Tracey <kmtracey@…>]

Ah, good. I had a long car ride this morning where I came to the tentative conclusion that the individual [fields or widgets, I didn't know which] ought to be answering the question "are you empty?". Glad to see I wasn't totally out in left field in my thinking there. Brian I'd be happy to test out a patch if you'd find that helpful.

[Brian Rosner]

Great. I am working on a patch now. I will post something soon. Karen, that would be very helpful since it seems we are a few of the handful of people using newforms-admin ;)

[Brian Rosner]

adds an is_empty to widget classes w/tests.

[Brian Rosner]

Ok, I have attached a patch that passes all tests and seems to work fine with inline models. I think it would be correct behavior for the CheckboxInput to return as empty when the value is not True. I did the same thing with NullBooleanSelect since it is similar to CheckboxInput that cleans the data in value_from_datadict. Let me know if shouldn't be the correct behavior. I also created an is_empty method on MultiWidget to handle the hacked cases in the original is_empty method in BaseForm.

[Karen Tracey <kmtracey@…>]

I've had just a brief bit of time to try out the patch and it works for me so far. One oddity I noticed is if you have 3 "additional" edit-inline forms, and try to use the 2nd one for adding a new item (while leaving the 1st entirely empty), it doesn't work. All the required fields on the first one are flagged "this field is required". Is this behavior intentional/correct...that when adding an entry you must use the "additional" forms in order?

[jkocherhans]

Replying to Karen Tracey <kmtracey@gmail.com>:

One oddity I noticed is if you have 3 "additional" edit-inline forms, and try to use the 2nd one for adding a new item (while leaving the 1st entirely empty), it doesn't work. All the required fields on the first one are flagged "this field is required". Is this behavior intentional/correct...that when adding an entry you must use the "additional" forms in order?

That behavior is intentional. The validator works through the add forms backwards and once it finds a non-empty one, all the rest are required.

[Antti Kaihola]

The patch works at least in my fairly straightforward case and applies cleanly to rev 6718. BTW, a kinda related bug is #5919 -- adding new instances with inlines is broken.

[Antti Kaihola]

I noticed a slight usability problem. I have a related model which only has a foreign key to the parent model and an e-mail field. So in the inline view I see the e-mail field and the delete checkbox. If I both clear the e-mail field and check the delete checkbox, I get a validation error for the blank e-mail address, which doesn't make sense since I'm removing it anyway.

[Brian Rosner]

Interesting point. Sounds like the deletion needs to happen before any validation. akaihola, could you open a new ticket about that. That is unrelated to this ticket, but is a good point. I'd hate for this ticket to be fixed and your issue doesn't get fixed as it should.

[Antti Kaihola]

Another observation: if the inlined model has a BooleanField with default=True, you can't save the parent object unless you clear the checkboxes for that field on the blank items.

[Øyvind Saltvik <oyvind@…>]

The boolean problem is with the add forms, right?

[Brian Rosner]

akaihola,

Yes, I came across that as well. I have actually opened a ticket to fix that problem with a BooleanField and other fields with a default value. Take a look at #5878.

[jkocherhans]

(In [6837]) newforms-admin: Fixed #5828. You can leave inline add forms empty again. Thanks brosner.