Hi Guilherme,

I've just got a few small questions below, and one suggestion for the name of your context manager.

Cheers,
-Michael

> === modified file 'lib/canonical/launchpad/webapp/login.py'
> --- lib/canonical/launchpad/webapp/login.py	2010-02-18 10:52:51 +0000
> +++ lib/canonical/launchpad/webapp/login.py	2010-02-25 11:53:51 +0000
> @@ -12,6 +12,7 @@
>  from BeautifulSoup import UnicodeDammit
>
>  from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
> +from openid.extensions import sreg
>  from openid.fetchers import setDefaultFetcher, Urllib2Fetcher
>
>  import transaction
> @@ -27,7 +28,6 @@
>
>  from z3c.ptcompat import ViewPageTemplateFile
>
> -from canonical.cachedproperty import cachedproperty
>  from canonical.config import config
>  from canonical.launchpad import _
>  from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
> @@ -181,6 +181,8 @@
>          openid_vhost = config.launchpad.openid_provider_vhost
>          self.openid_request = consumer.begin(
>              allvhosts.configs[openid_vhost].rooturl)
> +        self.openid_request.addExtension(
> +            sreg.SRegRequest(optional=['email', 'fullname']))
>
>          trust_root = self.request.getApplicationURL()
>          assert not self.openid_request.shouldSendRedirect(), (
> @@ -258,8 +260,34 @@
>
>      def render(self):
>          if self.openid_response.status == SUCCESS:
> -            account = getUtility(IAccountSet).getByOpenIDIdentifier(
> -                self.openid_response.identity_url.split('/')[-1])
> +            identifier = self.openid_response.identity_url.split('/')[-1]
> +            account_set = getUtility(IAccountSet)
> +            try:
> +                account = account_set.getByOpenIDIdentifier(identifier)
> +            except LookupError:
> +                # Here we assume the OP sent us the user's email address and
> +                # full name in the response. Note we can only do that because
> +                # we used a fixed OP (login.launchpad.net) that includes the
> +                # user's email address in the response when asked to.  Once we

s/email address/email address and full name ? Or is full name returned from
all OP's when requested?

> +                # start using other OPs we won't be able to make this
> +                # assumption here as they might not include what we want in
> +                # the response.
> +                sreg_response = sreg.SRegResponse.fromSuccessResponse(
> +                    self.openid_response)
> +                assert sreg_response is not None, (
> +                    "OP didn't include an sreg extension in the response.")
> +                email_address = sreg_response.get('email')
> +                full_name = sreg_response.get('fullname')
> +                assert email_address is not None and full_name is not None, (
> +                    "No email address or full name found in sreg response; "
> +                    "can't create a new account for this identity URL.")
> +                account, email = account_set.createAccountAndEmail(
> +                    email_address,
> +                    PersonCreationRationale.OWNER_CREATED_LAUNCHPAD,
> +                    full_name,
> +                    password=None,
> +                    openid_identifier=identifier)
> +
>              if account.status == AccountStatus.SUSPENDED:
>                  return self.suspended_account_template()
>              if IPerson(account, None) is None:
>

...

> === renamed file 'lib/canonical/launchpad/webapp/tests/test_new_login.py' => 'lib/canonical/launchpad/webapp/tests/test_login.py'
> --- lib/canonical/launchpad/webapp/tests/test_new_login.py	2010-02-17 19:40:41 +0000
> +++ lib/canonical/launchpad/webapp/tests/test_login.py	2010-02-25 11:53:51 +0000
> @@ -1,24 +1,31 @@
>  # Copyright 2009 Canonical Ltd.  All rights reserved.
> +from __future__ import with_statement
>
> +# pylint: disable-msg=W0105
>  """Test harness for running the new-login.txt tests."""
>
>  __metaclass__ = type
>
>  __all__ = []
>
> +from contextlib import contextmanager
>  import httplib
>  import unittest
>
>  import mechanize
>
>  from openid.consumer.consumer import FAILURE, SUCCESS
> -
> -from canonical.launchpad.interfaces.account import AccountStatus
> +from openid.extensions import sreg
> +
> +from zope.component import getUtility
> +from zope.security.proxy import removeSecurityProxy
> +
> +from canonical.launchpad.interfaces.account import AccountStatus, IAccountSet
>  from canonical.launchpad.testing.pages import (
>      extract_text, find_main_content, find_tag_by_id, find_tags_by_class)
>  from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
>  from canonical.launchpad.testing.browser import Browser, setUp, tearDown
> -from canonical.launchpad.webapp.login import OpenIDCallbackView
> +from canonical.launchpad.webapp.login import OpenIDCallbackView, OpenIDLogin
>  from canonical.launchpad.webapp.servers import LaunchpadTestRequest
>  from canonical.testing.layers import AppServerLayer, DatabaseFunctionalLayer
>
> @@ -29,10 +36,13 @@
>
>  class FakeOpenIDResponse:
>
> -    def __init__(self, identity_url, status=SUCCESS, message=''):
> +    def __init__(self, identity_url, status=SUCCESS, message='', email=None,
> +                 full_name=None):
>          self.message = message
>          self.status = status
>          self.identity_url = identity_url
> +        self.sreg_email = email
> +        self.sreg_fullname = full_name
>
>
>  class StubbedOpenIDCallbackView(OpenIDCallbackView):
> @@ -42,25 +52,47 @@
>          self.login_called = True
>
>
> +@contextmanager
> +def stub_SRegResponse_fromSuccessResponse():
> +    def sregFromFakeSuccessResponse(cls, success_response, signed_only=True):
> +        return {'email': success_response.sreg_email,
> +                'fullname': success_response.sreg_fullname}
> +
> +    orig_method = sreg.SRegResponse.fromSuccessResponse
> +    # Use a stub SRegResponse.fromSuccessResponse that works with
> +    # FakeOpenIDResponses instead of real ones.
> +    sreg.SRegResponse.fromSuccessResponse = classmethod(
> +        sregFromFakeSuccessResponse)
> +
> +    yield
> +
> +    sreg.SRegResponse.fromSuccessResponse = orig_method
> +
> +

OK, so the contextmanager gives us neat way to monkey-patch? Nice, but isn't
this going against the "use it instead of try/finally" rule-of-thumb?

Also, do you think the function could be named so it flows with the with
statement, ie.

with SRegResponse_fromSuccessResponse_stubbed():

(or something similar that fits our style guidelines).

>  class TestOpenIDCallbackView(TestCaseWithFactory):
>      layer = DatabaseFunctionalLayer
>
> -    def _createView(self, account, response_status=SUCCESS, response_msg=''):
> +    def _createViewWithResponse(
> +            self, account, response_status=SUCCESS, response_msg=''):
> +        openid_response = FakeOpenIDResponse(
> +            ITestOpenIDPersistentIdentity(account).openid_identity_url,
> +            status=response_status, message=response_msg)
> +        return self._createView(openid_response)
> +
> +    def _createView(self, response):
>          request = LaunchpadTestRequest(
>              form={'starting_url': 'http://launchpad.dev/after-login'},
>              environ={'PATH_INFO': '/'})
>          view = StubbedOpenIDCallbackView(object(), request)
>          view.initialize()
> -        view.openid_response = FakeOpenIDResponse(
> -            ITestOpenIDPersistentIdentity(account).openid_identity_url,
> -            status=response_status, message=response_msg)
> +        view.openid_response = response
>          return view
>
>      def test_full_fledged_account(self):
>          # In the common case we just login and redirect to the URL specified
>          # in the 'starting_url' query arg.
>          person = self.factory.makePerson()
> -        view = self._createView(person.account)
> +        view = self._createViewWithResponse(person.account)
>          view.render()
>          self.assertTrue(view.login_called)
>          response = view.request.response
> @@ -73,7 +105,7 @@
>          # create one.
>          account = self.factory.makeAccount('Test account')
>          self.assertIs(None, IPerson(account, None))
> -        view = self._createView(account)
> +        view = self._createViewWithResponse(account)
>          view.render()
>          self.assertIsNot(None, IPerson(account, None))
>          self.assertTrue(view.login_called)
> @@ -82,13 +114,36 @@
>          self.assertEquals(view.request.form['starting_url'],
>                            response.getHeader('Location'))
>
> +    def test_unseen_identity(self):
> +        # When we get a positive assertion about an identity URL we've never
> +        # seen, we automatically register an account with that identity
> +        # because someone who registered on login.lp.net or login.u.c should
> +        # be able to login here without any further steps.
> +        identifier = '4w7kmzU'
> +        account_set = getUtility(IAccountSet)
> +        self.assertRaises(
> +            LookupError, account_set.getByOpenIDIdentifier, identifier)
> +        openid_response = FakeOpenIDResponse(
> +            'http://testopenid.dev/+id/%s' % identifier, status=SUCCESS,
> +            <email address hidden>', full_name='Foo User')
> +        view = self._createView(openid_response)
> +        with stub_SRegResponse_fromSuccessResponse():
> +            view.render()

See above suggestion for the name of the contextmanager. I think it would make
it more readable.

> +        self.assertTrue(view.login_called)
> +        account = account_set.getByOpenIDIdentifier(identifier)
> +        self.assertIsNot(None, account)
> +        self.assertEquals(AccountStatus.ACTIVE, account.status)
> +        self.assertEquals('<email address hidden>',
> +                          removeSecurityProxy(account.preferredemail).email)

Is there a reason for not testing the full name as well?

> +        self.assertIsNot(None, IPerson(account, None))
> +
>      def test_deactivated_account(self):
>          # The user has the account's password and is trying to login, so we'll
>          # just re-activate their account.
>          account = self.factory.makeAccount(
>              'Test account', status=AccountStatus.DEACTIVATED)
>          self.assertIs(None, IPerson(account, None))
> -        view = self._createView(account)
> +        view = self._createViewWithResponse(account)
>          view.render()
>          self.assertIsNot(None, IPerson(account, None))
>          self.assertTrue(view.login_called)

...

> @@ -172,12 +235,51 @@
>                            extract_text(error_msg))
>
>
> -def fill_login_form_and_submit(browser, email_address, password):
> -    assert browser.getControl(name='field.email') is not None, (
> -        "We don't seem to be looking at a login form.")
> -    browser.getControl(name='field.email').value = email_address
> -    browser.getControl(name='field.password').value = password
> -    browser.getControl('Continue').click()
> +class FakeOpenIDRequest:
> +    extensions = None
> +
> +    def addExtension(self, extension):
> +        if self.extensions is None:
> +            self.extensions = [extension]
> +        else:
> +            self.extensions.append(extension)
> +
> +    def shouldSendRedirect(self):
> +        return False
> +
> +    def htmlMarkup(self, trust_root, return_to):
> +        return None
> +
> +
> +class FakeOpenIDConsumer:
> +    def begin(self, url):
> +        return FakeOpenIDRequest()
> +
> +
> +class StubbedOpenIDLogin(OpenIDLogin):
> +    def _getConsumer(self):
> +        return FakeOpenIDConsumer()
> +
> +
> +class TestOpenIDLogin(TestCaseWithFactory):
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_sreg_fields(self):
> +        # We request the user's email address and Full Name (through the SREG
> +        # extension) to the OpenID provider so that we can automatically
> +        # register unseen OpenID identities.
> +        request = LaunchpadTestRequest()
> +        # This is a hack to make the request.getURL(1) call issued by the view
> +        # not raise an IndexError.
> +        request._app_names = ['foo']
> +        view = StubbedOpenIDLogin(object(), request)
> +        view()
> +        extensions = view.openid_request.extensions
> +        self.assertIsNot(None, extensions)
> +        sreg_extension = extensions[0]
> +        self.assertIsInstance(sreg_extension, sreg.SRegRequest)
> +        self.assertEquals(['email', 'fullname'],
> +                          sorted(sreg_extension.allRequestedFields()))
>
>
>  def test_suite():
>

Thanks!

IRC Log:

12:50 < noodles775> salgado: you mention this is a temporary thing, what will happen to all the account records that are created during the window before 
                    the refactoring of the Account table?
12:51 < salgado> noodles775, we're going to move the openid_identifiers to a separate table and then drop Account*
12:51 < noodles775> intellectronica: ta.
12:52 < noodles775> salgado: ok, so none of it will be needed. Great.
1