Code review comment for lp:~michael.nelson/launchpad/db-598464-get-or-create-from-identity

{{{
13:55 < noodles775> Hi salgado, if you've got a spare few mins, can you take a look at the issue I've identified at: https://code.edge.launchpad.net/~michael
.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885
13:55 < noodles775> Let me know if I'm not making sense on the MP.
13:56 < salgado> noodles775, sure, I have it in my inbox and will get to it shortly
13:56 < noodles775> salgado: thanks.
14:06 < salgado> noodles775, in which case is the sreg extension not included in the response?
14:07 < noodles775> salgado: normal login with existing account. ie. 1) start dev server, 2) click login 3) enter <email address hidden>:test 4) click login. In
 this case calling _getEmailAddressAndFullName raises the assert.
14:08 < noodles775> It's not seen in the current code as _getEmailAddressAndFullName is only called if the account doesn't exist.
14:09 -!- abentley [~<email address hidden>] has quit [Ping timeout: 260 seconds]
14:10 < salgado> noodles775, that's weird because the code that generates the positive openid response is including the sreg extension unconditionally
14:10 * salgado tries to dupe
14:11 < salgado> indeed, duped
14:11 -!- abentley [~<email address hidden>] has joined #launchpad-reviews
14:12 < noodles775> Yeah, I'd assumed the sreg response should always be there, and was surprised too.
14:14 < salgado> noodles775, but this is a bug in the testopenid implementation
14:14 < noodles775> Where should I look?
14:14 < salgado> testopenid == the OpenID provider we use when developing/testing
14:14 < salgado> I'm checking it now
14:14 < salgado> noodles775, lib/lp/testopenid/browser/server.py
14:15 < noodles775> OK, so we can be confident that the staging/production SSO always include the sreg.
14:15 < noodles775> Thanks.
14:15 < salgado> createPositiveResponse() adds the sreg extension, but it's empty
14:15 -!- jtv-afk [~jtv@canonical/launchpad/jtv] has quit [Quit: Leaving.]
14:16 < salgado> noodles775, I think I've found the problem
14:17 < salgado> noodles775, OpenIDLogin.render() asks for 'email' and 'fullname' in the sreg response
14:17 < salgado> bug testopenid ignores that and only includes the nickname
14:17 -!- rinze is now known as jelmer-lunch
14:17 < salgado> s/bug/but
14:18 < noodles775> Right... I see.
14:18 < noodles775> So should it instead include *only* the requested items (email/fullname) or in addition to the nickname?
14:19 -!- abentley [~<email address hidden>] has quit [Quit: Ex-Chat]
14:19 < salgado> I think it's fine to include the 3 of them, but the nickname shouldn't be used anywhere
14:20 < noodles775> OK, I'll update it... thanks salgado !
14:22 < noodles775> salgado: also, I assume you don't have time to review the actual branch, but if you could have a brief look and make sure you think it's a sane change, that would be great.
14:24 < salgado> noodles775, sure, I can do that
14:25 < noodles775> Ta.
14:27 < salgado> noodles775, out of curiosity, why somebody needs a launchpad account to buy stuff from the software center? wouldn't a Ubuntu SSO account be more appropriate in this case?
14:28 < noodles775> salgado: yes, the must already have an SSO account to make the payment, it's after they've paid for the software, we need to give them access to a private PPA (ie. a private ppa subscription), and that's only possible if you've got an LP account.
14:29 < salgado> I see
14:29 < salgado> and it'd be lots of work to make it possible for someone to have a private ppa subscription without having a Launchpad profile, I assume?
14:30 < noodles775> Yes, but that is the long-term plan.
14:31 < salgado> cool!
14:36 -!- jtv [~<email address hidden>] has joined #launchpad-reviews
14:38 < salgado> 415 + except LookupError:
14:38 < salgado> 416 + assert email_address is not None and full_name is not None, (
14:38 < salgado> 417 + "An email address and full name are required to "
14:38 < salgado> 418 + "create an account.")
14:38 < salgado> noodles775, I'd move that assert out of the except block
14:39 < noodles775> That won't be required... I'd only added it because of the above issue.
14:40 < noodles775> Hrm, although, it's still possible for other callsites in the future to pass in None (even if processPositive.. doesn't).
14:40 < noodles775> So yep, will do.
14:40 < salgado> indeed
14:48 < salgado> noodles775, I think you can get rid of checked_for_email and the two email_set.getByEmail(email_address) calls by doing "email = email_set.getByEmail(email_address)" at the beginning instead of "email = None"
14:49 -!- leonardr [~<email address hidden>] has joined #launchpad-reviews
14:50 < noodles775> salgado: yep - that was part of r9514 which I've reverted after our discussion (I couldn't do it at the beginning at that point, because email was None when called via processPositive..)
14:50 < salgado> right
14:50 < salgado> I guess I should reload the m-p page, then
14:50 * noodles775 hasn't pushed... hangon.
14:51 < noodles775> OK, pushed r9515...
15:02 < salgado> noodles775, IPersonSet.getOrCreateByOpenIDIdentifier() has a requestor argument but the actual implementation doesn't
15:02 < noodles775> salgado: thanks, I'll remove it from the IFace. I've just pushed 9516 which just adds the email/fullname to the sreg response for the testopenid server too.
15:04 < salgado> cool, let me reload again
15:14 -!- stub [~stub@canonical/launchpad/stub] has quit [Quit: Leaving.]
15:31 < noodles775> Thanks salgado... I'll paste the irc conversation on the MP too. (And I've pushed the changes you suggested so far). I think that should be enough to get a normal review.
}}}