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

[Jeremy Kerr]

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.

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. 

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.

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


> 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

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

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.

Cheers,


Jeremy