Code review comment for lp:~flacoste/launchpad/bug-559128

Wow, very nice!

"Zopefull": nice word :-)

Very nice cleanups in the branch, such as correcting the imports from zope.security.

A niggle, but I found it a bit odd when ``setupInteractionForPerson`` called ``return setupInteractionByEmail(ANONYMOUS, participation)``. I wondered why I should call a function with the word "email" in it to get an interaction when there's no email involved. My preference would be to change setupInteraction to look like this:

def setupInteraction(principal, login=None, participation=None):
    """Sets up a new interaction with the given principal.

    The login gets added to the launch bag.

    You can optionally pass in a participation to be used. If no
    participation is given, a Participation is used.
    """
    # If principal is None, this method acts just like endInteraction.
    if principal is None:
        endInteraction()
        return

    # THIS IS THE NEW BIT
    if principal == ANONYMOUS:
        authutil = getUtility(IPlacelessAuthUtility)
        principal = authutil.unauthenticatedPrincipal()

    if participation is None:
        participation = Participation()
(... and so on, as is)

Then the line in `setupInteractionForPerson`` would read ``return setupInteraction(ANONYMOUS)``. That makes much more sense to me. I'd guess you'd also delete these lines from ``setupInteractionByEmail`` too, because they would be unnecessary:

119 + else:
120 + principal = authutil.unauthenticatedPrincipal()

As I said, that's a niggle, and I won't insist on it, but it did give me pause.

Looks like you caught all my XXXs. :-) Yay!

Thank you,

Gary