Opened 4 years ago

Closed 5 days ago

#17464 closed New feature (wontfix)

Ease specifying non-form model attributes with the generic CreateView

Reported by: madjar Owned by: madjar
Component: Generic views Version: 1.3
Severity: Normal Keywords:
Cc: madjar, 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 madjar 4 years ago.
First patch, without documentation
BaseCreateView-default_fields.diff (4.9 KB) - added by madjar 4 years ago.

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by madjar

First patch, without documentation

comment:1 Changed 4 years ago by madjar

  • Cc madjar added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 4 years ago by madjar

comment:2 Changed 4 years ago by madjar

  • Owner changed from nobody to madjar
  • Status changed from new to assigned

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

comment:3 Changed 4 years ago by apollo13

  • Cc florian+django@… added
  • Triage Stage changed from Unreviewed to 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 Changed 4 years ago by madjar

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 16 months 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 12 months ago by timgraham

  • Patch needs improvement set
  • Summary changed from [patch] Work with partial forms with the generic CreateView to Ease specifying non-form model attributes with the generic CreateView

comment:7 Changed 5 days ago by madjar

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 5 days ago by timgraham

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 5 days ago by madjar

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 5 days ago by timgraham

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

comment:11 Changed 5 days ago by madjar

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 5 days ago by timgraham

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 5 days ago by madjar

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

Thanks for your time!

comment:14 Changed 5 days ago by timgraham

  • Resolution set to wontfix
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top