On Fri, 2009-10-30 at 17:35 +0000, Leonard Richardson wrote:
> Leonard Richardson has proposed merging lp:~leonardr/launchpadlib/trusted-workflow-tests-2 into lp:launchpadlib.
> 
>     Requested reviews:
>     LAZR Developers (lazr-developers)
> 
> 
> This branch creates a TrustedTokenAuthorizationConsoleApp, a console
> application that guides the end-user through the process of
> authorizing a request token. It creates a lot of doctests for the App
> object and a small command-line script that invokes it. The doctests
> use the new UserInput class which simulates a user typing things into
> a console application.
> 
> There are a lot of small spacing tweaks in this branch to make the
> application look good when run from the command line. These changes
> don't necessarily show up in the doctests, which are much more lenient
> about whitespace.

Did you consider changing your tests to run without the
NORMALIZE_WHITESPACE option, which is enabled by default for all our
tests?  But I guess there's no real benefit in making the test more
fragile just to show that the app looks good when running on the command
line.

> 
> In addition to the doctests I manually ran the console application to
> test things that can't be automatically tested such as the use of
> getpass and the opening of a page in the web browser.
> -- 

> === modified file 'src/launchpadlib/apps.py'
> --- src/launchpadlib/apps.py    2009-10-28 17:27:53 +0000
> +++ src/launchpadlib/apps.py    2009-10-30 17:35:17 +0000
> @@ -20,9 +20,18 @@
>  themselves are kept in bin/.
>  """
>  
> +__all__ = [
> +    'RequestTokenApp',
> +    'TrustedTokenAuthorizationConsoleApp',
> +    ]
> +
> +import getpass
> +import sys
> +
>  import simplejson
>  
> -from launchpadlib.credentials import Credentials
> +from launchpadlib.credentials import (
> +    Credentials, RequestTokenAuthorizationEngine, TokenAuthorizationException)
>  from launchpadlib.uris import lookup_web_root
>  
>  
> @@ -43,3 +52,124 @@
>          return simplejson.dumps(token)
>  
>  
> +class TrustedTokenAuthorizationConsoleApp(RequestTokenAuthorizationEngine):
> +    """An application that authorizes request tokens."""
> +
> +    def __init__(self, web_root, consumer_name, request_token,
> +                 access_levels='', input_method=raw_input):
> +        """Constructor.
> +
> +        :param access_levels: A string of comma-separated access level
> +        values.  To get an up-to-date list of access levels, pass
> +        token_format=Credentials.DICT_TOKEN_FORMAT into
> +        Credentials.get_request_token, load the dict as JSON, and look
> +        in 'access_levels'.
> +        """
> +        access_levels = [level.strip() for level in access_levels.split(',')]
> +        super(TrustedTokenAuthorizationConsoleApp, self).__init__(
> +            web_root, consumer_name, request_token, access_levels)
> +
> +        self.input_method = input_method
> +
> +    def run(self):
> +        """Try to authorize a request token from user input."""
> +        self.error_code = -1 # Start off assuming failure.
> +        start = "Launchpad credential client (console)"
> +        self.output(start)
> +        self.output("-" * len(start))
> +
> +        try:
> +            self()
> +        except TokenAuthorizationException, e:
> +            print str(e)
> +            self.error_code = -1
> +            return self.pressEnterToExit()

The above call is redundant because of the one below, no?

> +        return self.pressEnterToExit()
> +
> +    def exit_with(self, code):

Why is this not exitWith()?

> +        """Exit the app with the specified error code."""
> +        sys.exit(code)
> +
> +    def getSingleCharInput(self, prompt, valid):
> +        """Retrieve a single-character line from the input stream."""
> +        valid = valid.upper()
> +        input = None
> +        while input is None:
> +            input = self.input_method(prompt).upper()
> +            if len(input) != 1 or input not in valid:
> +                input = None
> +        return input
> +
> +    def pressEnterToExit(self):
> +        """Make the user hit enter, and then exit with an error code."""
> +        prompt = '\nPress enter to go back to "%s". ' % self.consumer_name
> +        self.input_method(prompt)
> +        self.exit_with(self.error_code)
> +        return self.error_code

Since exit_with() will call sys.exit(), there's no reason to return
anything here, is there?

> +
> +    def input_username(self, cached_username, suggested_message):

inputUsername()?

> +        """Collect the Launchpad username from the end-user.
> +
> +        :param cached_username: A username from a previous entry attempt,
> +        to be presented as the default.
> +        """
> +        if cached_username is not None:
> +            extra = " [%s] " % cached_username
> +        else:
> +            extra = "\n(No Launchpad account? Just hit enter.) "
> +        username = self.input_method(suggested_message + extra)
> +        if username == '':
> +            return cached_username
> +        return username
> +
> +    def input_password(self, suggested_message):
> +        """Collect the Launchpad password from the end-user."""
> +        if self.input_method is raw_input:
> +            password = getpass.getpass(suggested_message + " ")
> +        else:
> +            password = self.input_method(suggested_message)
> +        return password
> +
> +    def input_access_level(self, available_levels, suggested_message,
> +                           only_one_option=None):
> +        """Collect the desired level of access from the end-user."""
> +        if only_one_option is not None:
> +            self.output(suggested_message)
> +            prompt = self.message(
> +                'Do you want to give "%(app)s" this level of access? [YN] ')
> +            allow = self.getSingleCharInput(prompt, "YN")
> +            if allow == "Y":
> +                return only_one_option['value']
> +            else:
> +                return self.UNAUTHORIZED_ACCESS_LEVEL
> +        else:
> +            levels_except_unauthorized = [
> +                level for level in available_levels
> +                if level['value'] != self.UNAUTHORIZED_ACCESS_LEVEL]
> +            options = []
> +            for i in range(0, len(levels_except_unauthorized)):
> +                options.append(
> +                    "%d: %s" % (i+1, levels_except_unauthorized[i]['title']))
> +            self.output(suggested_message)
> +            for option in options:
> +                self.output(option)
> +            allowed = ("".join(map(str, range(1, i+2)))) + "Q"
> +            prompt = self.message(
> +                'What should "%(app)s" be allowed to do using your '
> +                'Launchpad account? [1-%(max)d or Q] ',
> +                extra_variables = {'max' : i+1})
> +            allow = self.getSingleCharInput(prompt, allowed)
> +            if allow == "Q":
> +                return self.UNAUTHORIZED_ACCESS_LEVEL
> +            else:
> +                return levels_except_unauthorized[int(allow)-1]['value']
> +
> +    def user_refused_to_authorize(self, suggested_message):
> +        """The user refused to authorize a request token."""
> +        self.output(suggested_message)
> +        self.error_code = -2
> +
> +    def user_authorized(self, access_level, suggested_message):
> +        """The user authorized a request token with some access level."""
> +        self.output(suggested_message)
> +        self.error_code = 0

The four methods above should be renamed as well, I guess.

Now I'll review the other two files that I didn't because they had
conflicts.

-- 
Guilherme Salgado <email address hidden>