Opened 12 years ago

Closed 9 years ago

#17464 closed New feature (wontfix)

Ease specifying non-form model attributes with the generic CreateView

Reported by: Georges Dubus Owned by: Georges Dubus
Component: Generic views Version: 1.3
Severity: Normal Keywords:
Cc: Georges Dubus, florian+django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Here is a scenario to explain the problem :

In a multi-user blog application, when a post is created, its author field is set to the current user. We have a generic view like this :

class PostCreateView(CreateView):
    model = Post
    form_class = modelform_factory(Post, fields=('content',))

No valid object can be created with this view, because the objects are always missing the author field.

Currently, I could solve this problem by defining this method :

    def get_form_kwargs(self):
        kwargs = super(PostCreateView, self).get_form_kwargs()
        kwargs.update({'instance': Post(author=self.request.user)})
        return kwargs

I have the feeling this is kind of hackish for a use case that seems common.

I've attached a patch that enable us to write this :

    def get_default_instance(self):
        return Post(author=self.request.user)

There is a test in the patch. Please review it, and if you find the change useful, I will also add the documentation.

Attachments (2)

Added-a-get_default_instance-method-to-BaseCreateVie.diff (4.0 KB ) - added by Georges Dubus 12 years ago.
First patch, without documentation
BaseCreateView-default_fields.diff (4.9 KB ) - added by Georges Dubus 12 years ago.

Download all attachments as: .zip

Change History (16)

by Georges Dubus, 12 years ago

First patch, without documentation

comment:1 by Georges Dubus, 12 years ago

Cc: Georges Dubus added

After some reflexion, it may be better to have a pair default_fields/get_default_fields like for the other parameters (initial, form_class, ...).

It may require a little more code, but will be more consistent, and a bit clearer for the final user :

    default_fields = {'status': 'new'}
    def get_default_fields(self):
        return {'author': self.request.user}

Any opinion on this ?

by Georges Dubus, 12 years ago

comment:2 by Georges Dubus, 12 years ago

Owner: changed from nobody to Georges Dubus
Status: newassigned

I just uploaded a patch for the second approach. This one has documentation.

comment:3 by Florian Apolloner, 12 years ago

Cc: florian+django@… added
Triage Stage: UnreviewedAccepted

It would be nice to have something like that, I am just not convinced if it's the best idea. Eg the admin does support save_form/save_model (https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py#L698) -- something similar might be doable here too and would cover more than setting of nonform values (eg settings values based on form and request values etc…)

comment:4 by Georges Dubus, 12 years ago

I'm not sure adding both save_form and save_model would be useful. Do you think save_model would be enough?

Also, is

obj = form.save(commit=False)
obj.save()

strictly equivalent to

form.save()

? According to https://code.djangoproject.com/browser/django/trunk/django/forms/models.py#L84, there is also some m2m work in the second one, so shouldn't this be

obj = form.save(commit=False)
form.save_m2m()
obj.save()

?

comment:5 by aidan@…, 10 years ago

Yeah, seems pretty strange there's not an obvious way to do this. We can set the initial data fairly easily:

    def get_initial(self):
        return {'property_id': self.kwargs['property_id']}

Why not the instance data?

    def get_instance_kwargs(self):
        return {'property_id': self.kwargs['property_id']}

comment:6 by Tim Graham, 10 years ago

Patch needs improvement: set
Summary: [patch] Work with partial forms with the generic CreateViewEase specifying non-form model attributes with the generic CreateView

comment:7 by Georges Dubus, 9 years ago

4 years later, in a Django sprint, I come back to working on this :)

https://github.com/madjar/django/tree/ticket_17464

comment:8 by Tim Graham, 9 years ago

That patch doesn't make it clear to me why it's preferable or needed to have the logic in the view as opposed to the form.

comment:9 by Georges Dubus, 9 years ago

The use case I have in mind populates an user field from self.request.user, so I guess this needs to be in the view (I can't access the request in the form logic, right?).

comment:10 by Tim Graham, 9 years ago

For that particular case, you could override FormMixin.get_form_kwargs() to pass the user to the form.

comment:11 by Georges Dubus, 9 years ago

Like in the code example in the ticket description? I found it a little too hackish, hence the ticket. However, that was four years ago and the opinion I had about it is far away, so I guess we could also just consider this problem solved and close the ticket :)

comment:12 by Tim Graham, 9 years ago

I don't have a really strong opinion, but it seems to me that setting attributes on the form instance before hand can be useful during validation, etc. It's also nice not to have redundant hooks such that developers wonder which one is best. I don't mind closing the ticket unless a more compelling argument is presented.

comment:13 by Georges Dubus, 9 years ago

Well, I don't have a compelling argument, so let's close this!

Thanks for your time!

comment:14 by Tim Graham, 9 years ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top