[Django] #17795: kwargs not passed on by django.views.generic.edit import ProcessFormView
Django
--------------------------------------+------------------------
Reporter: ed.crewe@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.4-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------
Although these do get attached to self.kwargs for use within the class,
this causes problems for decorators of the get_context_data method.
kwargs which you may expect to be available for use by decorators you
create are unset by this mixin.
NB: In my case this was related to testing user object permissions - hence
the need to have the context data
Patching as below fixes it:
A mixin that processes a form on POST.
"""
def get(self, request, *args, **kwargs):
form_class = self.get_form_class()
form = self.get_form(form_class)
-- return self.render_to_response(self.get_context_data(form=form))
++ kwargs['form'] = form
++ return self.render_to_response(self.get_context_data(**kwargs))
--
Ticket URL: <https://code.djangoproject.com/ticket/17795>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: ed.crewe@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by anonymous):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Same issue in
dates.BaseDateListView.get
list.BaseListView.get
detail.BaseDetailView.get
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:1>
Django
Reporter: ed.crewe@… | Owner: nobody
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
> Although these do get attached to self.kwargs for use within the class,
> this causes problems for decorators of the get_context_data method.
>
> kwargs which you may expect to be available for use by decorators you
> create are unset by this mixin.
>
> NB: In my case this was related to testing user object permissions -
> hence the need to have the context data
>
> Patching as below fixes it:
>
> A mixin that processes a form on POST.
> """
> def get(self, request, *args, **kwargs):
> form_class = self.get_form_class()
> form = self.get_form(form_class)
> -- return self.render_to_response(self.get_context_data(form=form))
> ++ kwargs['form'] = form
> ++ return self.render_to_response(self.get_context_data(**kwargs))
Although these do get attached to self.kwargs for use within the class,
this causes problems for decorators of the get_context_data method.
kwargs which you may expect to be available for use by decorators you
create are unset by this mixin.
NB: In my case this was related to testing user object permissions - hence
the need to have the context data
Patching as below fixes it:
{{{
A mixin that processes a form on POST.
"""
def get(self, request, *args, **kwargs):
form_class = self.get_form_class()
form = self.get_form(form_class)
-- return self.render_to_response(self.get_context_data(form=form))
++ kwargs['form'] = form
++ return self.render_to_response(self.get_context_data(**kwargs))
}}}
--
Comment (by jezdez):
Updated ticket description.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:2>
Django
Reporter: ed.crewe@… | Owner: nobody
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> Although these do get attached to self.kwargs for use within the class,
> this causes problems for decorators of the get_context_data method.
>
> kwargs which you may expect to be available for use by decorators you
> create are unset by this mixin.
>
> NB: In my case this was related to testing user object permissions -
> hence the need to have the context data
>
> Patching as below fixes it:
>
> {{{
> A mixin that processes a form on POST.
> """
> def get(self, request, *args, **kwargs):
> form_class = self.get_form_class()
> form = self.get_form(form_class)
> -- return self.render_to_response(self.get_context_data(form=form))
> ++ kwargs['form'] = form
> ++ return self.render_to_response(self.get_context_data(**kwargs))
>
> }}}
Although these do get attached to self.kwargs for use within the class,
this causes problems for decorators of the get_context_data method.
kwargs which you may expect to be available for use by decorators you
create are unset by this mixin.
NB: In my case this was related to testing user object permissions - hence
the need to have the context data
Patching as below fixes it:
{{{
A mixin that processes a form on POST.
"""
def get(self, request, *args, **kwargs):
form_class = self.get_form_class()
form = self.get_form(form_class)
-- return self.render_to_response(self.get_context_data(form=form))
++ kwargs['form'] = form
++ return self.render_to_response(self.get_context_data(**kwargs))
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:3>
Django
Reporter: ed.crewe@… | Owner: nobody
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by jezdez):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:4>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Fandekasp):
* owner: nobody => Fandekasp
* cc: lemaire.adrien@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:5>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Changes (by Fandekasp):
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:6>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Fandekasp):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
@ed.crewe , I couldn't reproduce your problem, I don't understand how
you're testing your user object permissions. I always do it through
get_object or dispatch, and didn't get such problem.
I wrote a test that does basically the same thing, by adding a kwarg in an
overrided get() instead of get_context_data, and insure that this new
kwarg isn't lost in ProcessFormView.get(). I don't like my test, so if you
could give me your use case, I'll replace the test by a proper one.
I'm wondering if I should do the same with post ? Again, I need to
understand the use case before doing so.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:7>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by anonymous):
Hi,
I have attached a file with the monkey patches I am using of class views -
and an example decorator that demonstrates the issue.
So why it is desirable for get_context_data to include the views object(s)
in the kwargs it is passed - rather than just getting them from the view
attributes, ie. self.object - so that standard decorators for the context
data method can be used which need access to a views object(s) or possibly
other attributes.
In this case an object permissions test decorator.
Thanks,
Ed
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:8>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Fandekasp):
* needs_better_patch: 1 => 0
Comment:
I think we're good now. I wrote responsible tests thanks to @acdha idea of
using RequestFactory for that case.
I realize that there is no test for BaseDateListView, so I omitted that
one, let me know if I should write tests for the archive views.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:9>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by julien):
* needs_better_patch: 0 => 1
Comment:
The patch looks good. If you could add tests for `BaseDateListView` that
would be great. Also some minor comments:
* Verify the actual value of the 'foo' context variables instead of just
verifying their existence.
* Instead of updating `kwargs` (e.g.: `kwargs['form'] = form`) before
passing it to `self.get_context_data()`, use named arguments, i.e.:
`self.get_context_data(form=form, **kwargs)`, mainly for consistency with
the rest of the codebase.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:10>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Fandekasp):
* needs_better_patch: 1 => 0
Comment:
Done. I just needed to write a test for one of {{{DateBaseListView}}}
subclasses, I wrote one for ArchiveIndexViewTests.
actual value of 'foo' is verified.
BaseDetailView.get() now consistent with the rest of the code.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:11>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Has patch: 1 | checkin
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by julien):
* stage: Accepted => Ready for checkin
Comment:
Great, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:12>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 1 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* cc: bmispelon@… (added)
Comment:
For reference, commit f04bb6d798b07aa5e7c1d99d700fa6ddc7d39e62 added this
behavior to `TemplateView`.
I'm wondering if it'd be a good idea to just add this behavior in
`ContextMixin` instead of re-implementing it in each view.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:13>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version:
Component: Generic views | 1.4-beta-1
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
I'm not sure if this requires any changes per the last comment, but the
patch no longer applies cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:14>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Component: Generic views | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by polmuz):
* cc: polmuz (added)
* needs_better_patch: 1 => 0
* version: 1.4-beta-1 => master
* stage: Accepted => Ready for checkin
Comment:
Hi, I created a PR that merges cleanly using the patch provided. I think
it's ready for checking again given that adding the behavior in
ContextMixin doesn't seem to be straightforward.
https://github.com/django/django/pull/1631
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:15>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Generic views | Resolution:
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_docs: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
This behavior change needs a mention in the release notes and probably the
docs themselves.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:16>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Generic views | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mjtamlyn):
I want to -1 this change. URL keyword arguments are not context data. IMO
the behaviour of TemplateView is wrong here, and has bitten me before
where I've had to change my urlconf.
For a concrete exple of why this is bad, consider the situation where I
have a URL pointing to a form view which renders different forms depending
on the url. Then I call that URL kwarg 'form' and this code breaks.
The correct place to add things to the context is in get context data, not
by arbitrarily adding all URL kwargs to the context in all views.
This patch would break existing running code for me.
As an aside, note that view is put into the context now, so if you really
want to you can access the URL kwargs as view.kwargs
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:17>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Generic views | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mjtamlyn):
This is related to #19878
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:18>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Generic views | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by loic84):
@mjtamlyn: +1, it bit me too. It's a problem with TemplateView which if I
remember well, did that for backward compat with the old
`direct_to_template`.
It'd be great to deprecate that...
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:19>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: Generic views | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mjtamlyn):
That's what #19878 is about. the details of the deprecation are tricky.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:20>
Django
Reporter: ed.crewe@… | Owner: Fandekasp
Cleanup/optimization | Version: master
Component: Generic views | Resolution: wontfix
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 1
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Marking as won't fix given the backwards compatibility concerns.
--
Ticket URL: <https://code.djangoproject.com/ticket/17795#comment:21>