[PATCH] Refactor the generic_list() view to make it less complicated.
jk at ozlabs.org
Wed Mar 9 14:00:39 EST 2011
> @@ -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
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
> + 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.
More information about the Patchwork