launchpad-dev team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

On Tue, Dec 8, 2009 at 1:54 AM, Bjorn Tillenius <bjorn@xxxxxxxxxxxxx> wrote:
> On Tue, Dec 08, 2009 at 05:01:16AM -0000, noreply@xxxxxxxxxxxxx wrote:
>> Merge authors:
>>   Edwin Grubbs (edwin-grubbs)
>> ------------------------------------------------------------
>> revno: 9989 [merge]
>> committer: Launchpad Patch Queue Manager <launchpad@xxxxxxxxxxxxxxxxx>
>> branch nick: launchpad
>> timestamp: Tue 2009-12-08 04:58:31 +0000
>> message:
>>   [r=sinzui][ui=none][bug=297833] Improved error message when trying to
>>       add a private team as a member of another team.
>> modified:
>>   lib/canonical/launchpad/fields/__init__.py
>>   lib/lp/registry/browser/person.py
>>   lib/lp/registry/browser/tests/team-views.txt
>>   lib/lp/registry/interfaces/person.py
>>   lib/lp/registry/stories/teammembership/xx-private-membership.txt
>>   lib/lp/registry/templates/team-add-my-teams.pt
>>
>>
>> --
>> lp:launchpad/devel
>> https://code.launchpad.net/~launchpad-pqm/launchpad/devel
>>
>> You are subscribed to branch lp:launchpad/devel.
>> To unsubscribe from this branch go to https://code.launchpad.net/~launchpad-pqm/launchpad/devel/+edit-subscription.
>
>> === modified file 'lib/canonical/launchpad/fields/__init__.py'
>> --- lib/canonical/launchpad/fields/__init__.py        2009-11-23 03:43:05 +0000
>> +++ lib/canonical/launchpad/fields/__init__.py        2009-12-01 22:09:05 +0000
>> @@ -37,6 +37,8 @@
>>      'PasswordField',
>>      'PillarAliases',
>>      'PillarNameField',
>> +    'PrivateMembershipTeamNotAllowed',
>> +    'PrivateTeamNotAllowed',
>>      'ProductBugTracker',
>>      'ProductNameField',
>>      'PublicPersonChoice',
>> @@ -758,7 +760,7 @@
>>      """Return True if the person is public."""
>>      from canonical.launchpad.interfaces import IPerson, PersonVisibility
>>      if not IPerson.providedBy(person):
>> -        raise ConstraintNotSatisfied("Expected a person.")
>> +        return False
>>      if person.visibility == PersonVisibility.PUBLIC:
>>          return True
>>      else:
>
> Not your code, but you could have updated the last lines to read:
>
>    return person.visibility == PersonVisibility.PUBLIC


Agreed.


>> @@ -770,7 +772,7 @@
>>      """True if the person/team has private membership visibility."""
>>      from canonical.launchpad.interfaces import IPerson, PersonVisibility
>>      if not IPerson.providedBy(person):
>> -        raise ConstraintNotSatisfied("Expected a person.")
>> +        return False
>>      if person.visibility == PersonVisibility.PRIVATE_MEMBERSHIP:
>>          # PRIVATE_MEMBERSHIP.
>>          return True
>
> Same here.


Agreed.


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


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

-Edwin