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

[Guilherme Salgado]

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.

> 
> 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.

> 
> Are you expecting MultiplePatchForm to be able to handle multiple actions in 
> future? If not, we should be using MultiplePatchForm.action in the template 
> too.

I don't foresee that, but I'm happy to change the template to use it.  I
guess you mean that to avoid the hard-coding of the action in multiple
places?

> 
> > +    def save(self, instance):
> 
> Why remove the commit parameter? Not that we're using it, but just curious if 
> there's a reason for this.

I've removed it just because it was unused and untested, but I can
revert this if you feel it's worth keeping it. 
(Most python projects I've worked on seem to take this approach to
unused code, given the unnecessary burden of maintaining unused code and
how trivial it usually is to add it back if ever needed.)

> 
> 
> > diff --git a/apps/patchwork/views/__init__.py
> > b/apps/patchwork/views/__init__.py index 3f50380..c5ffeab 100644
> > --- a/apps/patchwork/views/__init__.py
> > +++ b/apps/patchwork/views/__init__.py
> > @@ -19,7 +19,8 @@
> > 
> > 
> >  from base import *
> > -from patchwork.utils import Order, get_patch_ids, set_patches
> > +from patchwork.utils import (
> > +    Order, get_patch_ids, bundle_actions, set_bundle)
> 
> I'd prefer using the backquote for line continuations here

Is that for consistency with the rest of the code?  I ask because
python's preferred way (as stated in PEP-8) is to use parentheses.

> 
> > +    if request.method == 'POST':
> > +        data = request.POST
> > +    user = request.user
> > +    properties_form = None
> > +    if (user.is_authenticated()
> > +        and project in user.get_profile().maintainer_projects.all()):
> > +        properties_form = MultiplePatchForm(project, data = data)
> 
> Hm, (I know this is in the original code too), this is duplicating the 
> Patch.is_editable functionality, and misses some of the cases there.

Do you have any suggestions on how to improve it?  I'd be happy to do
it.

> 
> Overall, what are you trying to achieve with this patch? Would be nice to 
> clean up generic_list, but I think we may need to tweak the approach a little.

I just wanted to clean up generic_list, and as I said above, moving the
form processing functionality to the form class made sense to me, but I
understand that may not be the preferred way in django.

-- 
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/20110309/e52cdeb6/attachment-0001.pgp>