Merge lp:~leonardr/launchpadlib/improve-workflow into lp:launchpadlib

Nice cover letter.

> Since it's now possible to distinguish "I don't want you to have access to my Launchpad account" from "I haven't made a decision about giving you access", I took the opportunity to improve launchpadlib's behavior when access is denied. Previously launchpadlib just raised an unhelpful HTTPError. Now it ensures that login_with() and the like return None instead of a Launchpad object. When you call login_with(), you can check the return value and know that if it's None, the user made a decision not to give you access--you can sys.exit() or complain or whatever.

This of course needs to go into the docs and I would say ideally to get a brief post on the blog (even just a pointer to this mp.)

I think you should raise a specific exception if the user denies access, rather than returning None:

1- If the app ignores the return code, we don't want it to just blithely continue and then cause knock-on errors. The default should be for the app to stop.
2- Returning None for errors is just bad.
3- It gives you a space to eventually possibly pass back a sensible message. (I mean I doubt the user wants to explain why, but perhaps you can also in the future say "this app is banned" or "you're creating too many tokens" etc.) I would say you should probably put the error body in to the exception, even if at the moment it's not super helpful.

+ print " (%s)" % self.authorization_url
+ print "should be opening in your browser. Use your browser to"
+ print "to authorize this program to access Launchpad on your behalf."

It would be nice to not directly print but rather pass this to a callback function so guis can hook it. It could be a followon but perhaps you could just hoist it out while you're here.

- launchpad.credentials.save_to_path(consumer_credentials_path)
- os.chmod(consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)
+ if launchpad is not None:
+ launchpad.credentials.save_to_path(consumer_credentials_path)
+ os.chmod(
+ consumer_credentials_path, stat.S_IREAD | stat.S_IWRITE)

It's always much more secure to create the path secure (by passing a third parameter to os.open) than to chmod it later: the app may be interrupted, someone may grab it in the race window, etc.

Otherwise looks good.

Hi,

One trick I have seen that is a bit nicer than polling is to pass a localhost
url as the OAuth callback, and then set up a listening socket at that URL, and
respond to any requests made to it by checking the status of the token.

It's not that much different really, but gives you a more responsive application
for users without hitting LP frequently.

Thanks,

James

I responded to your comments and also changed the code that creates directories so that they're chmod 0700. Take another look.

+1 on all of poolies points.

James - the local socket thing is very fragile. better to use long
poll on LP to have lp signal when its ready.

-Rob

+ self.output(self.WAITING_FOR_USER % self.authorization_url)
+ while credentials.access_token is None:
+ try:
+ credentials.exchange_request_token_for_access_token(web_root)
+ except HTTPError, e:
+ if e.response.status == 401:
+ # The user has not made their decision yet.
+ time.sleep(1)
+ elif e.response.status == 403:
+ # The user decided not to authorize this
+ # application.
+ raise EndUserDeclinedAuthorization(e.content)
+ else:
+ # The user decided to grant access.
+ # credentials.access_token is no longer None.
+ break

This treats a 500 error as deciding to grant access. Really? I think you want the 'break' inside the try block. Aside from that it looks good.

 review: tweak

I do wonder a bit about the load this will introduce but perhaps we can measure that. Can we find out from the logs how many times this is called at the moment?

Ok, take another look.

1. I made the poll time configurable with a module constant, and moved the sleep() call to before the request so it will always be called once. Since it would take superhuman reflexes to click a link before the sleep() call expires (for any reasonable value of the poll time), this will save at least one HTTP request every time.

2. If Launchpad goes down during the poll, the poll will continue instead of the program assuming the user granted access. (I tested this by killing launchpad.dev after opening token-authorized in my browser.)

3. I saw only 50 requests for +access-token for July 16th (a Friday), out of about 4.5 million requests total. Even a large number of additional POSTs should go unnoticed. But I'd be OK with increasing the poll time to 2 or 3 seconds.

+ else:
+ # The user has not made their decision yet,
+ # or there was an error accessing the server.
+ pass

So this seems to me to mean that if the server is giving some non-403 error, the client will silently keep polling indefinitely. I think that is a bit strange. I can see how you might want to keep polling even if you get a transient error, but I think you should at least print the error. That's what I meant in my comment earlier today.

btw I suggest if you're making substantive changes is response to feedback you actually describe them in the commit message. If someone annotates the code later just "response to feedback" doesn't explain a lot.

As a (imo wishlist-priority) follow on, https://bugs.edge.launchpad.net/launchpad-foundations/+bug/607418

All right, I've changed the code so that if there's a problem communicating with Launchpad after the initial browser open, the exception will be printed to stdout.