adsabs / adsws

ADS web services
Other
2 stars 15 forks source link

Scopes: added manage.py method to update existing tokens #79

Closed romanchyla closed 8 years ago

romanchyla commented 8 years ago
romanchyla commented 8 years ago

@jonnybazookatone when you have a moment, please take a look (it also fixes the failing master)

jonnybazookatone commented 8 years ago

Just two easy questions:

  1. purely administrative: is there a reason you put the tests in tests/test_account.py and not in tests/test_commands.py. The manage.py commands are usually kept in there, despite the deceptive name, but maybe you had a reason you wanted it in the file you chose. Only nitpicking :-P.
  2. As it stands, if you successfully update the OAuthClient, but there is a problem during the update of the OAuthToken, it'll roll back changes to the tokens but not the clients. Even if you repeat the process and you do force_update as an argument, you won't add the same client_ids to the to_update list and so this logic will always be false if (force_token_update or token.client_id in to_update):. I think I'm reading that correctly, but worth double checking if I made a mistake. So is it possible you would update the clients and not the tokens and be forced to fix them manually?
romanchyla commented 8 years ago

Thanks Jonny, I should change both - especially, the second one (good catch!) -- i'll just move it to the try block

but as for the OR condition: if force_token_update is True, then it will proceed - and won't even bother testing the second condition