Opened 16 years ago

Closed 8 years ago

#7975 closed Uncategorized (fixed)

Admin inlines incorrectly report "this field is required" if inline model has field with default value

Reported by: Russell Keith-Magee Owned by: Brian Rosner
Component: contrib.admin Version: dev
Severity: Normal Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider the following model:

class Person(models.Model):
    name = models.CharField(max_length=128)

class Membership(models.Model):
    person = models.ForeignKey(Person)
    date_joined = models.DateTimeField(default=datetime.now)

with the following admin definition:

class MembershipInline(admin.TabularInline):
    model = Membership
    extra = 3

class PersonAdmin(admin.ModelAdmin):
    inlines = (MembershipInline,)
    
admin.site.register(Person, PersonAdmin)

If you try to add or edit an instance of Person in the admin, you won't be able to save unless you fill in _all_ the extra lines in the inline.

This appears to be because the default value that is used to populate the inline form is confused with legitimate user-entered content. As a result, all the empty extra rows appear to have been partially filled out, so the validator raises an error for the 'missing' fields.

If you remove the default value from the date_joined field, the problem goes away.

Attachments (3)

callable_initial_fix_r8640.diff (6.8 KB ) - added by Manuel Saelices 16 years ago.
Patch that fixes this extreme bug, but with no tests yet.
callable_initial_fix_r8640.2.diff (6.8 KB ) - added by Manuel Saelices 16 years ago.
Only a typo in a doc string
callable_initial_fix_with_tests_r8776.diff (11.6 KB ) - added by Manuel Saelices 16 years ago.
Patch with tests

Download all attachments as: .zip

Change History (26)

comment:1 by Russell Keith-Magee, 16 years ago

At the very least, this should be documented as a limitation for inlined models. Ideally, it shouldn't be raised as a validation error. The easiest solution I can think of is to ignore any field with a default value when establishing if a row has been edited. Brian or Joseph might have some better ideas.

comment:2 by Karen Tracey, 16 years ago

This has come up before and been fixed, though I'm not sure it was ever fixed for this specific case. I think the significant thing in your example is that your default is a callable (datetime.now) that is returning a different value when the form is submitted than it returned when the form was created. It's not a general problem with having defaults in the inlines, nor even with callable defaults (changing the example to use just a DateField and date.today() for the default works, assuming you don't allow the clock to tick past midnight while you stare at the unsubmitted form). (BTW you also need to add at least one more field to the Membership model to hit the error, as it is all the inlines appear to be fully complete with just the default DateTime filled in.)

comment:3 by Eric Holscher, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:4 by Jacob, 16 years ago

Wow.

This one is awesome.

comment:5 by Jacob, 16 years ago

Triage Stage: Design decision neededAccepted

After talking with Malcolm, I think the solution is to put a hidden field with the original value for anything with a callable default. A bit nasty, yes, but really the only way to fix this.

comment:6 by Manuel Saelices, 16 years ago

Owner: changed from nobody to Manuel Saelices
Status: newassigned

comment:7 by Manuel Saelices, 16 years ago

Resolution: worksforme
Status: assignedclosed

I've testing this ticket, and the validation error doesn't appears. Instead of this, other strange thing occurs. All new rows are created, with default value. But this is a normal behaviour, because the confusion still there. This will be happend also in normal (not inline) forms.

I mark this as worksforme.

comment:8 by Karen Tracey, 16 years ago

Resolution: worksforme
Status: closedreopened

Notice the parenthetical remark I added above:

(BTW you also need to add at least one more field to the Membership model to hit the error, as it is all the inlines appear to be fully complete with just the default DateTime filled in.)

That explains the behavior you describe. To hit the error message you need to have at least one more field in Membership model, otherwise the inlines all appear to be completely filled in and they are simply added. This problem still exists, it's just the original models provided don't (and never did) actually demonstrate the problem.

comment:9 by Manuel Saelices, 16 years ago

@kmtracey, ok, now I see the bug, and I understand all.

@jacob, @malcolm, I'm looking for your proposal of hidden fields, but it is very difficult to fix this without changing a lot of code, because:

  • model fields knows about callable default.
  • forms has no idea about callable defaults.
  • widget _has_changed method doesn't know about other values than initial and data_value, and has no access to form field.

I think the most "success" approach may be this:

formfield method in model field, who knows that default is a callable, could create a form field with a new optional parameter (i.e. hidden_initial=True). Then, inside field constructor, we create a default widget passing same hidden_initial (another optional parameter we must to add).

But we still has problems. Look at a fragment code like this:

field = forms.CharField(initial='hello', hidden_initial=True)
field.widget = MyCustomWidget()

In last code, CharField.__init__ will create a widget with hidden_initial, but after that we replace the widget again. Widget don't know about field, and this implies that render method will never add a <input type="hidden" ... />. This can be workaround with a property in base Field class, with a setter.

Now, next step was modify _get_changed_data method in BaseForm class. The code is:

    def _get_changed_data(self):
        if self._changed_data is None:
            self._changed_data = []
            for name, field in self.fields.items():
                prefixed_name = self.add_prefix(name)
                data_value = field.widget.value_from_datadict(self.data, self.files, prefixed_name)
                initial_value = self.initial.get(name, field.initial)
                if field.widget._has_changed(initial_value, data_value):
                    self._changed_data.append(name)
        return self._changed_data
    changed_data = property(_get_changed_data)

There are two approachs here:

  1. Change value_from_datadict, to maybe return a NOT_CHANGED marker (or something like that), and then _has_changed returns False. I don't like this much.
  2. Change _get_changed_data itself to do logic. Something like:
       data_value = field.widget.value_from_datadict(self.data, self.files, prefixed_name)
       if not field.hidden_initial:
           initial_value = self.initial.get(name, field.initial) 
       else:
           initial_value = self.initial.get(name, field.widget.hidden_initial_from_datadict(self.data, self.files, prefixed_name)
       if field.widget._has_changed(initial_value, data_value):
           ...
    

I don't know if I'm in right direction. Jacob, Malcolm, could you help me?

comment:10 by Manuel Saelices, 16 years ago

From last comment, I will add, that maybe is not necessary to add hidden_initial to widget. Instead of, you can change field rendering.

comment:11 by Manuel Saelices, 16 years ago

ummm... after dinner, I refreshed myself and I've seen that thing only affect to BoundField. I will think again in a better solution.

comment:12 by Manuel Saelices, 16 years ago

I will attach a patch. I've found a lot of problems that I've never imaging. I consider that this patch is relatively good thinking in those problems.

Ok, patch have no tests and changes in docs, and possibly there are better variable names. I will wait for core devs review and write tests if core devs accept my patch as "good begin".

by Manuel Saelices, 16 years ago

Patch that fixes this extreme bug, but with no tests yet.

by Manuel Saelices, 16 years ago

Only a typo in a doc string

comment:13 by Manuel Saelices, 16 years ago

Has patch: set
Needs tests: set

comment:14 by Jacob, 16 years ago

Keywords: 1.0-blocker added

by Manuel Saelices, 16 years ago

Patch with tests

comment:15 by Manuel Saelices, 16 years ago

Needs tests: unset

comment:16 by Brian Rosner, 16 years ago

Owner: changed from Manuel Saelices to Brian Rosner
Status: reopenednew

comment:17 by Brian Rosner, 16 years ago

This is looking good. I will give this a through review very soon. Thanks msaelices!

comment:18 by Brian Rosner, 16 years ago

Resolution: fixed
Status: newclosed

(In [8816]) Fixed #7975 -- Callable defaults in inline model formsets now work correctly. Based on patch from msaelices. Thanks for your hard work msaelices.

comment:19 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

comment:20 by Mike Lissner, 8 years ago

Easy pickings: unset
Severity: Normal
Type: Uncategorized
UI/UX: unset

This is an old bug, and I worry that my test case isn't right somehow, but I'm fairly certain I'm hitting the same issue. I have an inline model that has default drop down values, and I can't save the parent model without the inline throwing a big error about required fields that aren't filled in.

My models are more complex than we want in a bug report, but I figured I'd pipe up in case others are encountering this on Django 1.8.2 as well.

comment:21 by Zopieux, 8 years ago

I do have the same issue in Django version 1.8.5.
When saving a valid Contestant, admin complains about event_type being required on the CorrectionInline even though min_num is zero.

class Contestant(models.Model):
    pass

class ContestantCorrection(models.Model):
    contestant = models.ForeignKey(Contestant, related_name='corrections')
    event_type = models.PositiveSmallIntegerField(choices=[whatever], db_index=True)
class CorrectionInline(admin.StackedInline):
    model = Correction
    min_num = 0
    extra = 1

class ContestantAdmin(admin.ModelAdmin):
    inlines = [CorrectionInline]
Last edited 8 years ago by Zopieux (previous) (diff)

comment:22 by Zopieux, 8 years ago

Resolution: fixed
Status: closednew

comment:23 by Tim Graham, 8 years ago

Resolution: fixed
Status: newclosed

This ticket has been closed for 7 years. Please open a new report with more details to reproduce (a sample project is nice if possible). I couldn't reproduce a problem with what you provided in your comment. Thanks!

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