[OH-Dev] Problem solved for now -- Re: Trying to Use Form Validation in Class Based Generic Views

[Nathan Yergler]

Comments inline...

On Sun, Mar 18, 2012 at 11:32 AM, Asheesh Laroia <lists at asheesh.org> wrote:
> Excerpts from Nathan Yergler's message of Sun Mar 18 13:23:32 -0400 2012:
>> Sorry for the delayed reply, we had a major-minor meltdown at work on
>> Thursday that sort of sucked up all of my ability to think about
>> software for 48 hours. I took a look at the test in question, and
>> after pulling things apart for a bit, believe I found the issue. Well,
>> two issues.
>>
>> First, the magic keyword argument for overriding the context is
>> extra_context_data, not extra_context. So checkout_submit() was
>> passing in an extra_context kwarg and MissionBaseView.get_context_data
>> was looking for extra_context_data. That's the first problem.
>>
>> The second was/is that Checkout.get_context_data is clobbering the
>> svn_checkout_form with a new form instance. Making that conditional
>> (if 'svn_checkout_form' not in data: data['svn_checkout_form'] =
>> forms.CheckoutForm()) takes care of that problem. See the attached
>> diff (dont_clobber.patch) for both changes.
>
> for dont_clobber.patch:
>
> In classic open source poor-communication style, an equivalent patch
> to that was pushed to openhatch/oh-mainline:master a few days ago.
>

No worries, it was actually useful to start with a failing test so I
had a way into the code, otherwise I think I would've been a little
lost.

> But the reason I wanted your advice especially is your experience
> with generic Django class-based views, which your follow-on patch
> is about! (:

Excellent.

>
>> Thinking about the larger problem (using the same handler for get +
>> post, with some additional data from a post), I think we can clean
>> things up a little. Is this Checkout example a good one for the
>> problem? The only thing that seems to be getting passed in as
>> extra_context_data is a possible error message and form instance. My
>> first instinct is that this looks a lot like the FormResponseView. Is
>> that something you've looked at and discarded? I have a more invasive
>> patch attached, form_views.patch, that I *think* cleans things up and
>> eliminates the need to fake the request method. The form processing
>> views in Django can be complex and intimidating, but they're pretty
>> powerful. I'd love to hear what people think about the latter patch
>> (and I'm happy to help apply it more widely to the codebase over
>> time). Even if the problem isn't limited to forms, I suspect a similar
>> idiom can be applied: a .post() handler that calls .get() with the
>> result of a special method like self.get_extra_context(). I think
>> that'd make it easier to follow and debug the code (ie, no wondering
>> why your method is GET when you "know" you made a POST).
>
> I think this is a lot like FormResponseView, actually. When learning a
> new technology (like Django generic class-based views), I try to learn
> as much as fits in my head then and build something, so I feel like I
> have solid ground to stand on. That's how we ended up using the Template-
> related views even though this is really a FormResponseView.
>
> I think that moving all these to FormResponseView would be just great.
>
> My idea with using class-based views is to make the training missions'
> code be mostly configuration, not code; that way, we can make it very
> easy for people to write new training missions, without necessarily
> writing much code. And also, it minimizes bugs due to incorrect
> repetition.
>

Concur. Getting the class inheritance right can be tough for people
who really want to leverage Django's class based views, so I think
having a base OpenHatch view makes a lot of sense.

> e.g.
>
> * I would move success_url to be a string (the urls.py short name) in
>  the Form subclass.

The view name or the resolved URL? When I initially wrote my patch I
had something like:

class View:
    success_url = reverse('view-name')

which didn't work since reversing a URL requires importing modules,
and importing the view module required reversing the URL (ka-boom). If
you want to make it the view name, I suggest naming the property
something like "success_view", and overriding get_success_url in your
parent view. That avoids assigning semantics to success_url that
differ from Django's and simplifies the child classes.

>
> * class Checkout should have:
>    mission_step_name = 'svn_checkout'
>  and then the default form_valid() machinery should set the mission step
>  as completed.

Makes sense.

>
> * Perhaps all the missions forms should implicitly take the username as
>  a forms_kwarg? But then you couldn't use these mixins with non-Missions
>  forms, which would be odd.
>

I could go either way with this one... if all the existing mission
forms (or > 80%) use a username right now, then I say do it. If you
want to further reduce the burden for contributors just give them a
form super class that has a different __init__. If you really want to
be magical you could write get_form_kwargs such that it sees if you're
dealing with a subclass of UserMissionForm and adds the username kwarg
if you are.

How many of the mission forms do you think have username-dependencies right now?

> * Then we could write a super-simple write-your-own-training-missions
>  HOWTO.

Cool. I'd be happy to help with that from a class-based-view perspective.

>
> This is the kind of thing that I intended for handling in these tickets:
>
> * http://openhatch.org/bugs/issue645
>
> * http://openhatch.org/bugs/issue666
>
> (surely others also)
>
> As I understand things, Jacquie plans to make some further code commits to
> do some of this move. I think we should keep conversing and sketching the
> code out here on OH-Dev -- Jacquie would probably appreciate the concreteness
> we're bringing to the conversation, as compared to my vagueness on those
> bugs!
>
> So Nathan (and all) -- do you think the above sorts of things I'd want
> to change are possible?

Yup.

And I've attached an updated patch with most of the code changes. It
also contains one other change, renaming template references from
"svn_checkout_form" to "form". That puts you in sync with what the
Django form views expect, and allows us to eliminate one more custom
override.

NRY

>
> -- Asheesh.
> _______________________________________________
> Devel mailing list
> Devel at lists.openhatch.org
> http://lists.openhatch.org/mailman/listinfo/devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mission_form_views.patch
Type: text/x-patch
Size: 7571 bytes
Desc: not available
URL: <http://lists.openhatch.org/pipermail/devel/attachments/20120318/6b2708e4/attachment-0001.bin>