Opened 3 years ago

Closed 3 years ago

#17148 closed Bug (fixed)

WizardView uses get_context_data incorrectly

Reported by: bradley.ayers@… Owned by: nobody
Component: contrib.formtools Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

https://code.djangoproject.com/browser/django/trunk/django/contrib/formtools/wizard/views.py#L538

However docs for every other generic view say get_context_data only takes kwargs, not args. I think it should be changed to form=form

Attachments (3)

17148.diff (3.6 KB) - added by Bradley Ayers <bradley.ayers@…> 3 years ago.
17148.alternative.diff (1.4 KB) - added by julien 3 years ago.
trac-17148.diff (6.2 KB) - added by steph 3 years ago.
Updated patch and extended tests

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by bradley.ayers@…

  • Component changed from Uncategorized to contrib.formtools
  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug
  • Version changed from 1.3 to SVN

comment:2 Changed 3 years ago by julien

  • Triage Stage changed from Unreviewed to Accepted

This is a minor thing, but it makes sense to try and stay consistent here.

comment:3 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

It became a problem when I wrote a mixin that expected the kwargs protocol and it didn't work :(

comment:4 Changed 3 years ago by julien

It'd be great if you could provide a test case illustrating that mixin problem. The patch itself should then be trivial.

comment:5 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

Something along the lines of:

class NoopMixin(object):
    def get_context_data(self, **kwargs):
        return super(TestMixin, self).get_context_data(**kwargs)


class TestWizard(NoopMixin, WizardView)
    pass
    
    
view = TestWizard.as_view([forms.Form])

comment:6 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

Sorry:

class NoopMixin(object):
    def get_context_data(self, **kwargs):
        return super(NoopMixin, self).get_context_data(**kwargs)

comment:7 Changed 3 years ago by julien

Thanks, your tickets are much appreciated. If you could provide patches while your mind is fresh working with formtools, it'd help fast-track all of those changes.

comment:8 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

I'll see if I can whip something up in a few hours.

Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

comment:9 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

  • Has patch set

Changed 3 years ago by julien

comment:10 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Design decision needed

I'm not sure that what was originally reported here is necessary a problem. It's OK for a class to define a different signature for a method it overrides. In the context of the WizardView the form is quite important, so it would make sense to make it a required parameter in WizardView.get_context_data()'s signature -- if we decide to do so, then it would be a matter of clarifying the documentation. It could also be argued that consistency should be preserved with every other view and therefore that WizardView.get_context_data()'s signature should also be (self, **kwargs). I haven't personally used formtools in real projects yet so I prefer to leave that decision to someone who has (or perhaps to someone like jezdez who's done a lot of work on this). So for now, I'll switch this ticket to DDN.

I've noticed, however, a related but different issue. TemplateView.get_context_data() only accepts **kwargs, yet it is also passed *args by WizardView.get_context_data(). I have corrected that in the attached patch.

comment:11 Changed 3 years ago by Bradley Ayers <bradley.ayers@…>

I want to elaborate on the issue I had. I wrote a generic mixin that implements get_context_data(self, **kwargs), and my inheritance was:

class MyView(MyMixin, WizardView):
    pass

I suppose I could have just switched the order of the inheritance. I'm not sure if this would solve every scenario like this though.

comment:12 Changed 3 years ago by julien

I see. The test case in your original patch only contains a single inheritance. If you could provide a test case using some mixins and where the current method signature poses a problem, then that would make the normalisation of signatures more compelling.

comment:13 Changed 3 years ago by jezdez

  • Triage Stage changed from Design decision needed to Accepted

Note the related tickets #16074 and #17242 which I think describe the general problem.

Changed 3 years ago by steph

Updated patch and extended tests

comment:14 Changed 3 years ago by steph

I just uploaded a new patch with more tests. I think this is ready for checkin.

comment:15 Changed 3 years ago by julien

  • Resolution set to fixed
  • Status changed from new to closed

In [17231]:

Fixed #17148 -- Fixed the signature of WizardView.get_context_data() to play nicer with mixin inheritance. Thanks, Bradley Ayers and Stephan Jaekel.

Note: See TracTickets for help on using tickets.
Back to Top