Opened 13 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)
Change History (16)
by , 13 years ago
Attachment: | Added-a-get_default_instance-method-to-BaseCreateVie.diff added |
---|
comment:1 by , 13 years ago
Cc: | 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 , 13 years ago
Attachment: | BaseCreateView-default_fields.diff added |
---|
comment:2 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I just uploaded a patch for the second approach. This one has documentation.
comment:3 by , 13 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
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 , 13 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 , 11 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 , 10 years ago
Patch needs improvement: | set |
---|---|
Summary: | [patch] Work with partial forms with the generic CreateView → Ease specifying non-form model attributes with the generic CreateView |
comment:8 by , 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 , 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 , 9 years ago
For that particular case, you could override FormMixin.get_form_kwargs()
to pass the user to the form.
comment:11 by , 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 , 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 , 9 years ago
Well, I don't have a compelling argument, so let's close this!
Thanks for your time!
comment:14 by , 9 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
First patch, without documentation