Patchwork Refactor the generic_list() view to make it less complicated.

login
register
mail settings
Submitter Guilherme Salgado
Date Feb. 25, 2011, 7:31 p.m.
Message ID <20110225193135.9524.64622.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/84565/
State Accepted
Headers show

Comments

Guilherme Salgado - Feb. 25, 2011, 7:31 p.m.
When a form is submitted it now delegates to separate processing functions
according to the action.  Apart from being more readable it's now a lot easier
to add extra forms for processing lists of patches.

Signed-off-by: Guilherme Salgado <guilherme.salgado@linaro.org>
---
 0 files changed, 0 insertions(+), 0 deletions(-)
Jeremy Kerr - March 9, 2011, 3 a.m.
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
Guilherme Salgado - March 9, 2011, 11:52 a.m.
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 - March 16, 2011, 9:53 p.m.
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,
Jeremy Kerr - March 30, 2011, 6:38 a.m.
Hi Guilherme,

You've covered some of the points in a subsequent email, so just responding to 
the outstanding issues here.

In general, do you mind if we leave this refactoring until we have the 
notification branch stable and merged? It'll mean we have to do less to merge 
the branches together.

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

Exactly; the action string is defined in one place, and used as-is in multiple 
templates.

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

The prototype for the save() method is defined in the django.forms.Form API, 
so I'd rather leave this in.


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

Yeah, mostly for consistency.

BTW, (minor nit) if we're going for maximum PEP-8 compatibility, could you use 
the remainder of the first line, and indent the following lines to suit? ie:

    if width == 0 and height == 0 and (color == 'red' or
                                       emphasis is None):

rather than:

    if width == 0 and height == 0 and (
            color == 'red' or emphasis is None):


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

I think it'll be okay to leave as-is for now; I'll review what we need to do 
in this situation.

One general thing: could you remove the trailing full-stop from your patch 
subjects? This means there's one less thing for me to edit when applying.

Cheers,


Jeremy

Patch

diff --git a/apps/patchwork/forms.py b/apps/patchwork/forms.py
index caa1949..cb7a94e 100644
--- a/apps/patchwork/forms.py
+++ b/apps/patchwork/forms.py
@@ -197,6 +197,7 @@  class MultipleBooleanField(forms.ChoiceField):
             raise ValueError('Unknown value: %s' % value)
 
 class MultiplePatchForm(forms.Form):
+    actions = ['update']
     state = OptionalModelChoiceField(queryset = State.objects.all())
     archived = MultipleBooleanField()
 
@@ -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")
+
+        return errors
+
+    def save(self, instance):
         opts = instance.__class__._meta
         if self.errors:
             raise ValueError("The %s could not be changed because the data "
@@ -225,8 +249,7 @@  class MultiplePatchForm(forms.Form):
 
             setattr(instance, f.name, data[f.name])
 
-        if commit:
-            instance.save()
+        instance.save()
         return instance
 
 class UserPersonLinkForm(forms.Form):
diff --git a/apps/patchwork/utils.py b/apps/patchwork/utils.py
index 5df6404..e41ffb6 100644
--- a/apps/patchwork/utils.py
+++ b/apps/patchwork/utils.py
@@ -18,8 +18,7 @@ 
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
 
-from patchwork.forms import MultiplePatchForm
-from patchwork.models import Bundle, Project, BundlePatch, UserProfile
+from patchwork.models import Bundle, Project, BundlePatch
 from django.shortcuts import get_object_or_404
 
 def get_patch_ids(d, prefix = 'patch_id'):
@@ -138,47 +137,3 @@  def set_bundle(user, project, action, data, patches, context):
     bundle.save()
 
     return []
-
-
-def set_patches(user, project, action, data, patches, context):
-    errors = []
-    form = MultiplePatchForm(project = project, data = data)
-
-    try:
-        project = Project.objects.get(id = data['project'])
-    except:
-        errors = ['No such project']
-        return (errors, form)
-
-    str = ''
-
-    # this may be a bundle action, which doesn't modify a patch. in this
-    # case, don't require a valid form, or patch editing permissions
-    if action in bundle_actions:
-        errors = set_bundle(user, project, action, data, patches, context)
-        return (errors, form)
-
-    if not form.is_valid():
-        errors = ['The submitted form data was invalid']
-        return (errors, form)
-
-    for patch in patches:
-        if not patch.is_editable(user):
-            errors.append('You don\'t have permissions to edit the ' + \
-                    'patch "%s"' \
-                    % patch.name)
-            continue
-
-        if action == 'update':
-            form.save(patch)
-            str = 'updated'
-
-
-    if len(patches) > 0:
-        if len(patches) == 1:
-            str = 'patch ' + str
-        else:
-            str = 'patches ' + str
-        context.add_message(str)
-
-    return (errors, form)
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)
 from patchwork.paginator import Paginator
 from patchwork.forms import MultiplePatchForm
 
@@ -34,36 +35,45 @@  def generic_list(request, project, view,
     context.project = project
     order = Order(request.REQUEST.get('order'), editable = editable_order)
 
-    form = MultiplePatchForm(project)
-
-    if request.method == 'POST' and \
-                       request.POST.get('form') == 'patchlistform':
-        action = request.POST.get('action', None)
-        if action:
-            action = action.lower()
+    # Explicitly set data to None because request.POST will be an empty dict
+    # when the form is not submitted, but passing a non-None data argument to
+    # a forms.Form will make it bound and we don't want that to happen unless
+    # there's been a form submission.
+    data = None
+    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)
+
+    if request.method == 'POST' and data.get('form') == 'patchlistform':
+        action = data.get('action', '').lower()
 
         # special case: the user may have hit enter in the 'create bundle'
         # text field, so if non-empty, assume the create action:
-        if request.POST.get('bundle_name', False):
+        if data.get('bundle_name', False):
             action = 'create'
 
         ps = []
-        for patch_id in get_patch_ids(request.POST):
+        for patch_id in get_patch_ids(data):
             try:
                 patch = Patch.objects.get(id = patch_id)
             except Patch.DoesNotExist:
                 pass
             ps.append(patch)
 
-        (errors, form) = set_patches(request.user, project, action, \
-                request.POST, ps, context)
+        if action in bundle_actions:
+            errors = set_bundle(user, project, action, data, ps, context)
+        elif properties_form and action in properties_form.actions:
+            errors = properties_form.process(user, action, ps, context)
+        else:
+            errors = []
+
         if errors:
             context['errors'] = errors
 
-    if not (request.user.is_authenticated() and \
-            project in request.user.get_profile().maintainer_projects.all()):
-        form = None
-
     for (filterclass, setting) in filter_settings:
         if isinstance(setting, dict):
             context.filters.set_status(filterclass, **setting)
@@ -83,7 +93,7 @@  def generic_list(request, project, view,
 
     context.update({
             'page':             paginator.current_page,
-            'patchform':        form,
+            'patchform':        properties_form,
             'project':          project,
             'order':            order,
             })