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

Tue Mar 27 06:13:46 UTC 2012

Just wanted to follow-up here on an IRC conversation Asheesh and I had
last week (I think).

I think the common goal is for missions to be more configuration than
code, at least to the extent possible. On one extreme we have the
Windows Setup mission, which is really just a series of templates.
Putting together a mission like that should be as easy as listing the
templates in a Python file and pointing the URL conf at that list (as
opposed the having to write a view for each right now). The Svn
Mission is more complex, but still encompasses a series to steps. It'd
be nice to make that "configuration" aspect as obvious as possible.

I'm not certain what abstractions are missing, but I'm going to try
and write some mission-writing-docs (probably this weekend) so people
have a better sense of how to apply class based views in OH.

Happy to answer questions, etc, in the interim.

NRY

On Sun, Mar 18, 2012 at 7:10 PM, Nathan Yergler <nathan at yergler.net> wrote:
> 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