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