launchpad-dev team mailing list archive

[Brad Crittenden <brad.crittenden@xxxxxxxxxxxxx>]

On Dec 8, 2009, at 13:46 , Curtis Hovey wrote:

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

The changes mentioned here don't affect the vocabularies AFAICT.

The vocabularies perform screening based on the privacy setting and the user's membership.  What Edwin proposed last week was having the vocabularies return all of a person's visible teams and then letting the various fields decide whether a private team was appropriate or not rather than having specific vocabularies that only returned one type of team.  Looking at the details of the vocabularies now, that appears to be what has been happening all along.

While we had a lot of performance issues with the vocabularies, and did a lot of tweaking to get the queries to work, I don't recall that being an issue that drove more validation into them. 

--Brad


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