Launchpad itself

Code review comment for lp:~henninge/launchpad/bug-503454-security-py

  1. bug-503454-security-py
  2. Merge into devel
Revision history for this message
Henning Eggers (henninge) wrote :

Am 13.01.2010 22:06, Edwin Grubbs schrieb:
> Review: Approve code
> Hi Henning,
>
> This branch is a nice improvement. It's always good to shrink files.
>
> I have a few comments below, but I also would like this change made,
> which isn't actually part of your diff. In PersonRoles.__getattr__
> please add info to the exception you raise so that it matches the error
> python gives you for its default types such as:
> AttributeError: 'int' object has no attribute 'foo'
>
> merge-conditional
>

Thank you very much for your suggestions which I liked very much. All
were implemented. ;-)

An incremental diff is attached.

Cheers,
Henning

inc.diff
1=== modified file 'lib/canonical/launchpad/doc/personroles.txt'
2--- lib/canonical/launchpad/doc/personroles.txt 2010-01-12 17:07:32 +0000
3+++ lib/canonical/launchpad/doc/personroles.txt 2010-01-14 11:19:29 +0000
4@@ -1,6 +1,6 @@
5 = PersonRoles =
6
7-To make the checking for certain roles that a person can have more convenient
8+To make it more convenient to check which roles a person has,
9 the IPersonRoles adapter is provided. It's main use is to easily check if a
10 person is the member of a celebrity team or is a celebrity person. In
11 addition, methods for common checks are provided, too.
12@@ -8,14 +8,13 @@
13 The IPersonRoles interface is closely tight to the ILaunchpadCelebrities
14 interface. Any addition or removal of a person celebrity must be reflected in
15 adding or removing the corresponding property in IPersonRoles. Luckily the
16-celbrities.txt doctest includes a check for this and will give a useful
17-information of which attribute needs to be added or removed. Both interfaces
18+celebrities.txt doctest includes a check for this and will give useful
19+information on which attribute needs to be added or removed. Both interfaces
20 are found in the same file. There is no need to adapt the implementation
21 class PersonRoles, though (thanks to __getattr__).
22
23 PersonRoles is most prominent in AuthenticationBase in security.py. The user
24-parameter to checkAuthenticated is a PersonRoles object (used to be a Person
25-object).
26+parameter to checkAuthenticated is a PersonRoles object (was a Person object).
27
28
29 == The person object and the adapter ==
30@@ -60,8 +59,8 @@
31 True
32
33 To stay consistent, all attributes are prefixed with "in_" although the
34-attributes names of ILaunchpadCelebrities are not all lexically correct
35-plurals nor are all the attributes teams. This makes for odd sounding
36+attribute names of ILaunchpadCelebrities are not all lexically correct
37+plurals, nor are all the attributes teams. This makes for odd sounding
38 attribute names in IPersonRoles.
39
40 >>> print roles.in_admin
41
42=== modified file 'lib/canonical/launchpad/security.py'
43--- lib/canonical/launchpad/security.py 2010-01-12 17:42:43 +0000
44+++ lib/canonical/launchpad/security.py 2010-01-14 11:21:36 +0000
45@@ -2215,11 +2215,10 @@
46 self.obj.person.hide_email_addresses):
47 return True
48
49- person = IPerson(account, None)
50- if person is None:
51+ user = IPersonRoles(IPerson(account, None), None)
52+ if user is None:
53 return False
54
55- user = IPersonRoles(person)
56 return (self.obj.person is not None and user.inTeam(self.obj.person)
57 or user.in_commercial_admin
58 or user.in_registry_experts
59
60=== modified file 'lib/canonical/launchpad/utilities/personroles.py'
61--- lib/canonical/launchpad/utilities/personroles.py 2010-01-12 14:52:41 +0000
62+++ lib/canonical/launchpad/utilities/personroles.py 2010-01-14 11:10:40 +0000
63@@ -26,10 +26,14 @@
64 def __getattr__(self, name):
65 """Handle all in_* attributes."""
66 prefix = 'in_'
67+ errortext = "'PersonRoles' object has no attribute '%s'" % name
68 if not name.startswith(prefix):
69- raise AttributeError
70+ raise AttributeError(errortext)
71 attribute = name[len(prefix):]
72- return self.person.inTeam(getattr(self._celebrities, attribute))
73+ try:
74+ return self.person.inTeam(getattr(self._celebrities, attribute))
75+ except AttributeError:
76+ raise AttributeError(errortext)
77
78 def isOwner(self, obj):
79 """See IPersonRoles."""

« Back to merge proposal