Opened 8 years ago

Closed 8 years ago

#5126 closed (invalid)

Populate `initial_data` of forms from model instance

Reported by: Christopher Lenz <cmlenz@…> Owned by: nobody
Component: Forms Version: master
Severity: Keywords: feature
Cc: akaihol+django@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Okay, I've looked for ways to populate the initial data of a form created by newforms.form_for_model() from an existing model instance.

I can't use form_for_instance because I want to add custom clean_ hooks, so the pattern I'm using looks like this:

class MyForm(forms.form_for_model(MyModel, fields=['foo', 'bar'])):
    def clean_foo(self):
        # ...

Now when I create a form instance, I can't see a clean way to populate the inital data of the form properly. I've tried the following, but it doesn't work for relations, plus it's really ugly:

  form = MyForm(initial=myinstance.__dict__)

The only option seems to be overriding the __init__(), but that's a pain as I'd have to do that for every form. I guess I could probably work around this somehow, but I can't believe this isn't a problem others are seeing, so I'm attaching a patch that overrides the default __init__() to take the instance as first (optional) argument, which makes a lot of sense to me. The code would then look like this:

  form = MyForm(myinstance)

Kindly asking for review/comments. Thanks.

Attachments (7)

ticket5126.diff (1.5 KB) - added by Christopher Lenz <cmlenz@…> 8 years ago.
Patch as described in ticket description
ticket5126_2.diff (2.4 KB) - added by Christopher Lenz <cmlenz@…> 8 years ago.
Another patch with an added init_from_instance function
ticket5126_3.diff (1.9 KB) - added by Christopher Lenz <cmlenz@…> 8 years ago.
New patch, pass instance via initial parameter in constructor
ticket5126_4.diff (2.6 KB) - added by Christopher Lenz <cmlenz@…> 8 years ago.
Regenerated full patch
initial-default.diff (965 bytes) - added by Kevin Menard 8 years ago.
Alternative fix to Chris's problem.
initial-data-newforms-admin.diff (526 bytes) - added by Petr Marhoun <petr.marhoun@…> 8 years ago.
initial-data-trunk.diff (514 bytes) - added by Petr Marhoun <petr.marhoun@…> 8 years ago.

Download all attachments as: .zip

Change History (21)

Changed 8 years ago by Christopher Lenz <cmlenz@…>

Patch as described in ticket description

comment:1 follow-up: Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

I came up with the following snippet for this reason, which I was considering opening a ticket for but never got around to: http://www.djangosnippets.org/snippets/199/

A side note, vars(instance) is a much prettier way to access the __dict__.

I'm unconvinced on the merits of this ticket... but I'll pass to design decision.

Changed 8 years ago by Christopher Lenz <cmlenz@…>

Another patch with an added init_from_instance function

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by Christopher Lenz <cmlenz@…>

  • Summary changed from Override constructor of `form_for_model` class to take model instance to Populate `initial_data` of forms from model instance

Replying to SmileyChris:

I'm unconvinced on the merits of this ticket...

Then let me try a bit harder ;-)

Currently, if you don't use form_for_instance, there's simply no way in the API to use a model instance to populate the initial data of the form—short of workarounds such as the snippet you linked to. I think we can agree that this is a pretty common thing to want to do with newforms, and frankly, I'm puzzled by the omission.

If there's no argument about the why, let's move to the how. I see three possible options to add this to the API, not necessarily mutually exclusive:

  1. A module-level function in django.newforms.models (see my second patch):
      form = MyForm()
      forms.init_from_instance(form, myinstance)
    

This is the only option I can see for custom Form classes, unless you want to add coupling between the regular forms code and the model-aware forms stuff, or add an intermediary ModelForm subclass or something like that.

  1. An added regular instance method on the Form subclass produced by form_for_model()
  form = MyForm()
  form.init(myinstance)

This is a bit nicer from an OO perspective.

  1. Finally, an overridden constructor as initially proposed by this ticket:
  form = MyForm(myinstance)

(A variant of this last form would to accept the model instance as value of the initial keyword argument.)

IMHO it's obvious which of those is the nicest, but I'd buy any of them.

To conclude, what I'd suggest is ticket5126_2.diff my second patch, which adds the init_from_instance function for use with custom Form classes, but still overrides the constructor
.

Changed 8 years ago by Christopher Lenz <cmlenz@…>

New patch, pass instance via initial parameter in constructor

comment:3 in reply to: ↑ 2 Changed 8 years ago by Christopher Lenz <cmlenz@…>

Replying to Christopher Lenz <cmlenz@gmx.de>:

  1. Finally, an overridden constructor as initially proposed by this ticket:
  form = MyForm(myinstance)

(A variant of this last form would to accept the model instance as value of the initial keyword argument.)

Actually, that's a much better approach. Using the first parameter as in the first two patches sucks for the case where you're not interested in the initial data. So make that example:

  form = MyForm(initial=myinstance)

This is implemented by the third patch, which I think is really good and unobtrusive, and fully backwards compatible.

Changed 8 years ago by Christopher Lenz <cmlenz@…>

Regenerated full patch

comment:4 Changed 8 years ago by Christopher Lenz <cmlenz@…>

s/third patch/fourth patch.

Doh, sometimes patch management isn't that easy.

Changed 8 years ago by Kevin Menard

Alternative fix to Chris's problem.

comment:5 follow-up: Changed 8 years ago by Kevin Menard

Christopher,

I've attached a patch that I think addresses the same problems you've had. This patch is fairly simplistic in nature and all it does is set the form field's initial value to that of the model's default value, if one is provided. Using this patch, my admin forms in the newforms-admin branch have been populated with the correct initial values.

Please try it out and let me know if it does what you need.

comment:6 in reply to: ↑ 5 Changed 8 years ago by Christopher Lenz <cmlenz@…>

Replying to Kevin Menard:

Christopher,

I've attached a patch that I think addresses the same problems you've had. This patch is fairly simplistic in nature and all it does is set the form field's initial value to that of the model's default value, if one is provided. Using this patch, my admin forms in the newforms-admin branch have been populated with the correct initial values.

Please try it out and let me know if it does what you need.

While somewhat related, your change doesn't have quite the same scope/intention. The functionality I need is initializing the defaults from a model instance, not just from the defaults in the model class.

I.e. I have a custom newforms form that has not been created by form_for_instance, and I want to populate the initial data from a model instance. That's the use case, it's not covered by the newforms API, and AFAICT your patch wouldn't change that. Which doesn't mean your patch is bad or wrong, just that it's addressing a different issue. I would suggest creating a separate ticket.

comment:7 Changed 8 years ago by Kevin Menard

Thanks for the feedback. I'll open a new ticket then. Clearly I didn't understand your initial problem well enough.

comment:8 Changed 8 years ago by anonymous

Per Christopher's suggestion, I've created issue #5238 for my tangentially related problem. The patch I applied to this issue should be considered useless.

Changed 8 years ago by Petr Marhoun <petr.marhoun@…>

Changed 8 years ago by Petr Marhoun <petr.marhoun@…>

comment:9 follow-up: Changed 8 years ago by Petr Marhoun <petr.marhoun@…>

There is method initial_data in the newforms-admin branch:

from django.newforms.models import initial_data
form = MyForm(initial=initial_data(myinstance))

I think that this method should be accessible as django.newforms.initial_data (it is very usefull) - see the patch initial-data-newforms-admin.diff (only one word is added).

The patch initial-data-trunk.diff is invalid - I didn't know that this method isn't in trunk.

comment:10 Changed 8 years ago by PhiR

  • Keywords feature added

comment:11 Changed 8 years ago by anonymous

  • Cc akaihol+django@… added

comment:12 in reply to: ↑ 9 Changed 8 years ago by akaihola

Replying to Petr Marhoun <petr.marhoun@gmail.com>:

There is method initial_data in the newforms-admin branch:

It doesn't seem to handle foreign keys or dates correctly.

The snippet mentioned above doesn't handle dates either, I'll try to fix that.

comment:13 Changed 8 years ago by SmileyChris

akaihola - could you open a new ticket for fixing the date problem please?

comment:14 Changed 8 years ago by brosner

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

This has been fixed since ModelForm was committed to trunk. There is now a model_to_dict helper function in django.newforms.models. Pleae reopen if I am incorrect that the recent changes to trunk have not fixed this problem.

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