launchpad-dev team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Hi Brad.

I have some vague recollection that there were privacy performance
issues that forced us to push validation in to vocabularies. Are these
the same validators that Bjorn and Edwin are discussing? If so, I think
we need to either document why they are here, or commit to doing
performance testing and recoding if we move them.

On Tue, 2009-12-08 at 10:32 -0600, Edwin Grubbs wrote:
> On Tue, Dec 8, 2009 at 10:07 AM, Bjorn Tillenius <bjorn@xxxxxxxxxxxxx> wrote:
> > On Tue, Dec 08, 2009 at 09:44:09AM -0600, Edwin Grubbs wrote:
...
> >> >> -class ParticipatingPersonChoice(Choice):
> >> >> +        if is_valid_public_person(value):
> >> >> +            return True
> >> >> +        else:
> >> >> +            # The vocabulary prevents the revealing of private team names.
> >> >> +            raise PrivateTeamNotAllowed(value)
> >> >
> >> >
> >> > I was going to ask for some explicit tests for this field, and ask you
> >> > what happens if you pass in an invalid public person to the validator.
> >> > However, I did took a look as is_valid_public_person(), and it looks
> >> > like the name isn't correct, it should be named is_public_person, no?
> >>
> >>
> >> Renaming it to is_public_person() would be fine.
> >
> > Cool, that makes things a bit cleaner.
> >
> >
> >>
> >>
> >> > Do we really need functions to check the visibility attribute?
> >>
> >>
> >>
> >> The is_valid_public_person() and is_private_membership() are also used
> >> by the storm field validators validate_public_person() and
> >> validate_person_not_private_membership() found in
> >> lp/registry/interfaces/person.py.
> >>
> >> Although it is unlikely that these tests will ever change, it does
> >> make it possible to change the form validation and the model
> >> validation simultaneously. If it is worthwhile to keep, we should
> >> definitely add some comments so that we don't add code to those
> >> functions that is inappropriate for either level of validation.
> >>
> >> Do you think it is worthwhile to remove these functions?
> >
> > Hmm. Probably not. Ideally you should, or maybe move them to the model
> > code instead, but it's probably not worth the trouble. It's also not
> > clear to me, whether we need to check that the passed in value is an
> > IPerson.
> 
> 
> Checking the interface is probably not necessary for the form
> validation, although it definitely needs to check whether the value is
> None. The storm validator may encounter any kind of object, so it
> definitely needs an interface check. Since the interface check also
> handles None correctly for us, it is the most terse solution for a
> sharing the function between the form and the model.




-- 
__Curtis C. Hovey_________
http://launchpad.net/

Attachment: signature.asc
Description: This is a digitally signed message part