search

nyergler / nested-formset

Nest Django formsets for multi-level editing.
BSD 3-Clause "New" or "Revised" License
87 stars 21 forks source link

nested-formset should handle has_changed #13

Closed mbertheau closed 9 years ago

mbertheau commented 10 years ago

In the block - building - tenant model I get null value in column "building_id" violates not-null constraint when I try to create a new building with a tenant if I don't specify any field values for the building but enough for the tenant. The reason is that has_changed() for the building form is False and therefore no building is created that the tenant can use for its foreign key.

Putting validation on the building form doesn't help because validation is short-circuited in the case of has_changed() == False.

The issue can be resolved by passing a form class as the form argument to nestedformset_factory that has the has_changed method overridden as follows:

def has_changed(self):
    return super(VariantForm, self).has_changed() or self.nested.has_changed()

However I feel that this is the responsibility of the nested-formset package and I don't see an obvious non-hacky way to implement it there.

nyergler commented 10 years ago

I'm not sure I totally understand the issue: is this occurring when you're adding a building + tenant, and leaving all fields empty for the building?

mbertheau commented 10 years ago

Yes.

nyergler commented 10 years ago

If the middle model (building in this case) doesn't have any required fields, that sort of feels like a poorly designed model.

I'd merge a PR that did as you suggested in the default formset (assuming it has unit tests), but I'm unlikely to work on this myself.

mbertheau commented 10 years ago

The problem does occur in the standard case with a number of required fields on the middle model. The reason is that the validation step for the middle model is skipped completely if has_changed() returns False, see https://github.com/django/django/blob/master/django/forms/forms.py#L362

nyergler commented 10 years ago

Ah, got it. Thanks for pointing that out, I didn't realize has_changed impacted the behavior of full_clean. This does sound like something the library should handle, and probably not too difficult to address (probably much in the way you mentioned it could be done by the caller).