[PATCH] Refactor the generic_list() view to make it less complicated.

[Guilherme Salgado]

On Wed, 2011-03-09 at 08:52 -0300, Guilherme Salgado wrote:
> Hi Jeremy,
> 
> On Wed, 2011-03-09 at 11:00 +0800, Jeremy Kerr wrote:
> > Hi Guilherme,
> > 
> > > @@ -205,7 +206,30 @@ class MultiplePatchForm(forms.Form):
> > >          self.fields['delegate'] = OptionalDelegateField(project = project,
> > >                  required = False)
> > > 
> > > -    def save(self, instance, commit = True):
> > > +    def process(self, user, action, patches, context):
> > > +        errors = []
> > > +        if not self.is_valid() or action not in self.actions:
> > > +            return ['The submitted form data was invalid']
> > > +
> > > +        for patch in patches:
> > > +            if not patch.is_editable(user):
> > > +                errors.append(
> > > +                    "You don't have permissions to edit patch '%s'"
> > > +                    % patch.name)
> > > +                continue
> > > +
> > > +            self.save(patch)
> > > +
> > > +        if len(patches) == 1:
> > > +            context.add_message("1 patch updated")
> > > +        elif len(patches) > 1:
> > > +            context.add_message("%d patches updated" % len(patches))
> > > +        else:
> > > +            context.add_message("No patches selected; nothing updated")
> > 
> > The add_message calls don't account for the patches that may have been
> > skipped (ie, patch.is_editable() == False).
> > 
> > However, I'm not sure that this code belongs in the form layer; these things 
> > seem much more like view functionality. Hence the set_patches and set_bundle 
> > functions.
> 
> I don't know much about the django way of doing things here, but it felt
> to me like having a form able to render itself and process the submitted
> data would make for something easier to be reused. There's also the fact
> that the data processing function was passed the form as its first
> argument, so it made even more sense to me to have it as a form method.
> It's possible I'm biased though, as I'm used to working on zope3/ztk,
> where a form is just a specialized view.

I've had a look around for some Django examples and they all seem to let
views do the processing of form submissions as you suggested -- I
suppose Django forms are really meant just to render/validate the
fields/data.

> > 
> > Instead of a process() function on the form, how about we refactor set_bundle 
> > and set_patches to do the work of process(): take a list of patches, and apply 
> > the some processing to each. 
> 
> It should be trivial to do so, and I'm happy to do it if that's the
> preferred way of doing things in django.

I'll do this change, then, and it'd be nice if you could tell me whether
or not you'd like me to do any of the other changes I mentioned in the
previous email, so that I include them all in the next version of the
patch.

Cheers,

-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110316/b43eb94a/attachment.pgp>