Code review comment for lp:~salgado/launchpad/apidoc

On Jul 23, 2010, at 9:12 AM, Guilherme Salgado wrote:

> On Fri, 2010-07-23 at 12:42 +0000, Gary Poster wrote:
>> Hey Salgado. Thank you. Below, I've deleted sections for which I thought no further discussion was necessary.

Doing that again.

>>
>> On Jul 22, 2010, at 5:15 AM, Guilherme Salgado wrote:
>>
>>> On Wed, 2010-07-21 at 17:08 +0000, Gary Poster wrote:
>
...

>>
>>> I'm now wondering if I should do the same for all the eggs that I've
>>> updated as part of this -- they could also register new things that
>>> we're not expecting.
>>
>> The reason why I singled in on this one is that it is registered in
>> zopeapp.zcml, so it will affect production. The rest will only affect
>> tests and development, so I'm less concerned in this regard.
>>
>> zope.publisher's update I guess is worth a look, actually. But that's
>> the only other one I see.
>>
>
> http://paste.ubuntu.com/467986/ is the diff between the previous
> configure.zcml and the current version, so nothing to worry about, I
> think?

Agreed, thanks.

>
>
>>> - FWIW, LaunchpadPrincipalAnnotations could subclass UserDict.UserDict
>>>> and be shortened to the __init__ plus a call to the shared constructor
>>>> and the interface declarations. Do it if you want.
>>>
>>> I'm all for it!
>>>
>>> Also, I thought I could make apidoc.lp.dev work for anonymous users by
>>> just registering an adapter like the above for
>>> IUnauthenticatedPrincipal, but then I got
>>> http://paste.ubuntu.com/467401/
>>
>> Meh, digging into it just a bit, I see that preference group thing is
>> doing somewhat unusual security dances. I wouldn't be all that
>> surprised if that LocationError were actually hiding a
>> ForbiddenAttribute on a value that should be there, but maybe I'm
>> wrong.
>>
>>> I'm thinking that it may be better to actually require a logged-in user
>>> to use apidoc.lp.dev and ask them to login in case they're not, just
>>> like we do on other launchpad.AnyPerson pages. What do you think?
>>
>> Since this is only for devel instances, in the abstract there's no
>> reason not to expose this for anonymous users that I see. That will
>> also make it easier to make a static version of the apidoc, which I
>> think is part of the plan.
>
> Agreed. All I wanted was to make sure we require people to login as
> early as possible so that they don't see these obscure errors that one
> has no idea can be worked around just by logging in.
>
>>
>> If you want me to look into the traceback further just ask.
>
> I'd really appreciate as I wouldn't know where to start. Just some
> pointers so I can get started would be great.

I was skeptical of the way you were using object and UserDict together--that doesn't make UserDict a new-style class, so it doesn't play the "super" game. This fixes the traceback for unauthenticated users, and is generally more correct:

=== modified file 'lib/canonical/launchpad/webapp/authentication.py'
--- lib/canonical/launchpad/webapp/authentication.py 2010-07-22 09:12:46 +0000
+++ lib/canonical/launchpad/webapp/authentication.py 2010-07-23 14:54:07 +0000
@@ -326,12 +326,12 @@

 # zope.app.apidoc expects our principals to be adaptable into IAnnotations, so
 # we use these dummy adapters here just to make that code not OOPS.
-class TemporaryPrincipalAnnotations(object, UserDict):
+class TemporaryPrincipalAnnotations(UserDict):
     implements(IAnnotations)
     adapts(ILaunchpadPrincipal, IPreferenceGroup)

     def __init__(self, principal, pref_group):
- super(TemporaryPrincipalAnnotations, self).__init__()
+ UserDict.__init__(self)

 class TemporaryUnauthenticatedPrincipalAnnotations(