Opened 5 years ago

Closed 17 months 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 5 years ago.
First patch, without documentation
BaseCreateView-default_fields.diff (4.9 KB) - added by Georges Dubus 5 years ago.

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by Georges Dubus

First patch, without documentation

comment:1 Changed 5 years ago by Georges Dubus

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 ?

Changed 5 years ago by Georges Dubus

comment:2 Changed 5 years ago by Georges Dubus

Owner: changed from nobody to Georges Dubus
Status: newassigned

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

comment:3 Changed 5 years ago by Florian Apolloner

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 Changed 5 years ago by Georges Dubus

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 Changed 3 years ago by aidan@…

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 Changed 2 years ago by Tim Graham

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 Changed 17 months ago by Georges Dubus

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

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

comment:8 Changed 17 months ago by Tim Graham

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 Changed 17 months ago by Georges Dubus

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 Changed 17 months ago by Tim Graham

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

comment:11 Changed 17 months ago by Georges Dubus

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 Changed 17 months ago by Tim Graham

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 Changed 17 months ago by Georges Dubus

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

Thanks for your time!

comment:14 Changed 17 months ago by Tim Graham

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