launchpad-dev team mailing list archive

[Bjorn Tillenius <bjorn@xxxxxxxxxxxxx>]

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


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


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

Do we really need functions to check the visibility attribute?


> +
> +
> +class ParticipatingPersonChoice(PersonChoice):
>      """A person or team who is not a private membership team.
>  
>      A person can participate in all contexts.  A PRIVATE team can participate
> @@ -796,8 +818,10 @@
>      user.  A PRIVATE MEMBERSHIP team is severely limited in the roles in which
>      it can participate.
>      """
> -    implements(IReferenceChoice)
> -    schema = IObject    # Will be set to IPerson once IPerson is defined.
>  
>      def constraint(self, value):
> -        return not is_private_membership(value)
> +        if not is_private_membership(value):
> +            return True
> +        else:
> +            # The vocabulary prevents the revealing of private team names.
> +            raise PrivateMembershipTeamNotAllowed(value)


-- 
Björn Tillenius | https://launchpad.net/~bjornt