| Message ID | 20110225193135.9524.64622.stgit@localhost6.localdomain6 |
|---|---|
| State | Accepted |
| Headers | show |
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
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.
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,
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
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, })
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(-)