Code

Opened 9 years ago

Closed 7 years ago

#1207 closed defect (invalid)

[patch] Manipulators ignore default values, fields can't be omitted from forms

Reported by: akaihola Owned by: jacob
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: brenocon@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

My model has several fields with default values. I'd like to be able to omit those fields from a form and have defaults used when creating a new object.

See http://groups.google.com/group/django-users/browse_frm/thread/5e134e5aa43bafe5/d50c9ebb0935cb20 and http://simon.bofh.ms/logger/django/2006/1/12 for discussion.

Attachments (1)

dont-clobber-new_data.patch (476 bytes) - added by brenocon@… 8 years ago.
patch so new_data doesnt get bogus [None] value filled in by do_html2python

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by brenocon@…

  • Type changed from enhancement to defect

The specific user-level error seems to happen here:

m = Item.AddManipulator()
m.do_html2python(new_data)

that step adds in ALL of the model's fields to new_data, even if they weren't there before. If a field wasn't there before, it doesn't fill in the default value, instead [None] (new_data is a MultiValueDict.)

There seem to be 2 places in the manipulator logic that try to use the default value, but since new_data has already been clobbered with None values for all fields, neither place seems to get called in this situation. (1) at AutomaticManipulator.save() there is logic to use get_default() in some cases; and also (2) save() sometimes calls Field.get_manipulator_new_data(), and the base class form of that method also has logic to use get_default().

(1): django/db/models/manipulators.py:84 or so
(2): django/db/models/fields/init__.py:274
(line numbers as of rev [2893])

It seems to me that the problem is do_html2python is being destructive. It should be AutomaticManipulator.save()'s responsibility to add in missing values; Manipulator.do_html2python should only convert things already inside new_data. Since AM.save() already has logic for get_default()'ing, I presume this is the logic the authors had in mind.

Since Manipulator.do_html2python() seems to be fighting AM.save() I'm marking this as a bug... maybe respecting-defaults functionality used to exist before?

This all might be less confusing with a callchain illustration. The user programmer does 2 calls which in turn call other things...

(1)
Manipulator.do_2html -> FormField.convert_post_data(new_data)

then you next call:
(2)
AM.save() -> ?Field.get_default(); and also
AM.save() -> Field.get_manipulator_new_data(new_data) -> Field.get_defaiult()
(I think these are model Fields, not FormFields, but i'm not sure)

There's something I don't understand going on in FormField.convert_post_data(). It is a second place that's checking for whether field names exist in new_data. (AM.save() being the other place that I know of.) I couldn't make heads or tails of convert_post_data() or the assumptions it was making of other objects it was using; therefore I just put the fix into do_html2python().

My patch is attached. It ensures that fields' default values get assigned when saving via the AddManipulator. I'd write test code but I don't know how with form data and all else... if I get more time to work on this (spent forever already!) I'll try to do so.

I'm wondering if this bug has something to do with #1584, since the logic dealing with that is hidden up in the AM.save() code that was not being called correctly. (Though I haven't checked thoroughly enough to know.)

Finally.. it seems note the reporter of #908 had a similar problem to this bug.

Changed 8 years ago by brenocon@…

patch so new_data doesnt get bogus [None] value filled in by do_html2python

comment:2 Changed 8 years ago by brenocon@…

crap some of my formatting got screwed up. the user-level code that seems erroneous to me is:

m = Item.AddManipulator()
m.do_html2python(new_data)

The call chains are:

(1) Manipulator.do_2html -> FormField.convert_post_data(new_data)

(2) AM.save()

  • - - -> Field.get_default(); and also
  • - - -> Field.get_manipulator_new_data(new_data) -> Field.get_default()

(i'm not actually sure these are Field's, I'm guessing)

comment:3 Changed 8 years ago by brenocon@…

  • Summary changed from Manipulators ignore default values, fields can't be omitted from forms to [PATCH] Manipulators ignore default values, fields can't be omitted from forms

I take back what I said near the bottom of my big post, my patch doesn't seem to affect #1584 at all.

comment:4 Changed 8 years ago by James Bennett <ubernostrum@…>

This feels like we need to decide, and document more clearly up-front, what default really means, because it ends up being confusing -- there's the way it actually works, and there's the way that a lot of people seem to assume it works, and they're two different things. Both make sense, but we need to settle on which one is best and document it aggressively.

For the record, here are the two different interpretations, which differ on when exactly the value should be filled in:

  1. The way it actually works is that the default value is pre-filled in the FormWrapper instance, so that when the form is displayed to a user, the value will be in there -- the user can then leave it as-is, or change it if desired.
  2. The way many people seem to assume it works is that the value will be filled after the fact if the form comes in with no value for the field in question.

The disadvantage of (1) is that, in order to take advantage of the default value, you must actually include the field in what the end user sees; otherwise, the field comes back with no value and the only reasonable thing for Django to assume is that the user manually changed the value from its default to blank; often this will end up tripping a validation error, because the undisplayed field is usually going to be required.

The disadvantage of (2) is that it we could end up having to do lots of second-guessing about input values -- did the user really mean to submit an empty string (which is not inconceivable), in which case it should be left alone, or did the user leave the field alone (or was the field left out of the end user's display), in which case it should be replaced with the default. That seems like it could be quite a can of worms.

comment:5 Changed 8 years ago by anonymous

i dont see a problem with submiting empty strings

if the string is empty it gets a variable in the post request,
if the field is missing it gets no variable in the request

comment:6 Changed 8 years ago by jeff@…

I definitely would go for #2. In my mind, it makes much more sense.

comment:7 Changed 8 years ago by adrian

  • Summary changed from [PATCH] Manipulators ignore default values, fields can't be omitted from forms to [patch] Manipulators ignore default values, fields can't be omitted from forms

comment:8 Changed 8 years ago by anonymous

  • Cc brenocon@… added

comment:9 Changed 7 years ago by SmileyChris

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

Manipulators as they are now, are on the way out. This doesn't seem worth fixing before they go.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.