Merge lp:~flacoste/launchpad/bug-559128 into lp:launchpad

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

On April 15, 2010, Gary Poster wrote:
> 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:
>
Well, I guess, I could call setupInteraction directly in that case. I'll do
this.

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

This changes the signature of the function. I'll pass on it since I don't feel
like chasing all the call sites :-)

Cheers

--
Francis J. Lacoste
<email address hidden>

On April 15, 2010, Francis J. Lacoste wrote:
> On April 15, 2010, Gary Poster wrote:
> > 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:
> Well, I guess, I could call setupInteraction directly in that case. I'll do
> this.
>
> > def setupInteraction(principal, login=None, participation=None):
> > """Sets up a new interaction with the given principal.
>
> This changes the signature of the function. I'll pass on it since I don't
> feel like chasing all the call sites :-)
>
> Cheers

Disregard this, your suggestion makes perfect sense and I'll implement it.

/me shouldn't reply before the context restore has finished...

--
Francis J. Lacoste
<email address hidden>