launchpad-dev team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

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:
>> >> @@ -779,16 +781,36 @@
>> >>          return False
>> >>
>> >>
>> >> -class PublicPersonChoice(Choice):
>> >> +class PrivateTeamNotAllowed(ConstraintNotSatisfied):
>> >> +    __doc__ = _("A private team is not allowed.")
>> >> +
>> >> +
>> >> +class PrivateMembershipTeamNotAllowed(ConstraintNotSatisfied):
>> >> +    __doc__ = _("A private-membership team is not allowed.")
>> >> +
>> >> +
>> >> +class PersonChoice(Choice):
>> >> +    """A person or team.
>> >> +
>> >> +    This is useful as a superclass and provides a clearer error message than
>> >> +    "Constraint not satisfied".
>> >> +    """
>> >> +    implements(IReferenceChoice)
>> >> +    schema = IObject    # Will be set to IPerson once IPerson is defined.
>> >> +
>> >> +
>> >> +class PublicPersonChoice(PersonChoice):
>> >>      """A person or team who is public."""
>> >> -    implements(IReferenceChoice)
>> >> -    schema = IObject    # Will be set to IPerson once IPerson is defined.
>> >>
>> >>      def constraint(self, value):
>> >> -        return is_valid_public_person(value)
>> >> -
>> >> -
>> >> -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.

-Edwin