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>
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
