Opened 8 years ago

Closed 8 years ago

#5828 closed (fixed)

Changeset 6613 seems to have broken edit inline

Reported by: Øyvind Saltvik <oyvind@…> Owned by: nobody
Component: contrib.admin Version: newforms-admin
Severity: Keywords:
Cc: brosner@…, akaihol+djtick@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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.

Attachments (1)

6614_is_empty_fix.diff (4.5 KB) - added by brosner 8 years ago.
adds an is_empty to widget classes w/tests.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by Karen Tracey <kmtracey@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:2 Changed 8 years ago by brosner

  • Cc brosner@… added

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.

comment:3 Changed 8 years ago by 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....

comment:4 Changed 8 years ago by brosner

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.

comment:5 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by brosner

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

comment:7 Changed 8 years ago by 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. :)

comment:8 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by brosner

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 ;)

Changed 8 years ago by brosner

adds an is_empty to widget classes w/tests.

comment:10 Changed 8 years ago by brosner

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.

comment:11 follow-up: Changed 8 years ago by 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?

comment:12 in reply to: ↑ 11 Changed 8 years ago by 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.

comment:13 Changed 8 years ago by akaihola

  • Cc akaihol+djtick@… added
  • Has patch set

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.

comment:14 Changed 8 years ago by akaihola

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.

comment:15 Changed 8 years ago by brosner

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.

comment:16 Changed 8 years ago by akaihola

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.

comment:17 Changed 8 years ago by Øyvind Saltvik <oyvind@…>

The boolean problem is with the add forms, right?

comment:18 Changed 8 years ago by brosner

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.

comment:19 Changed 8 years ago by jkocherhans

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.
Back to Top