Code review comment for lp:~leonardr/launchpadlib/trusted-workflow-tests-2

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>