Add Django Util With Decorator and Views

[waprin]

This Django extension adds the views, decorators, and signals to oauth2client.

This is heavily based on the work @jonparrott did for oauth2client/flask_util.

Currently the only storage offered uses Django sessions. Based on issue #319 more storage options are planned along with integration to the existing CredentialsField in django_orm.

This adds an environment variable to the tox script. This might not be strictly necessary, but it did simplify the tests a lot, since Django settings get looked up at import time and cached so it's easiest if they have an environment variable pointing to an existing settings module when the tests are started.

This could have been squeezed into a single file, but I spilt it into separate files based on existing Django extension conventions.

[dhermes]

All new files added should have 100% coverage (towards #212).

From:
https://coveralls.io/builds/3925551

The following have less than 100%:

• django_util/apps.py: 0%
• django_util/storage.py: 89.47%
• tests/test_django_util.py: 98.05% (every line that isn't run should be removed or pragma: NO COVERed)

[dhermes]

Running tox -e cover locally it also looks like

oauth2client/django_util/storage.py         19      2      4      0    83%   51-52
oauth2client/django_util/views.py           62      0     10      1    99%   133->135

[dhermes]

If running the tests create db.sqlite3, you should add it to the .gitignore.

[waprin]

@dhermes thanks for taking a look, I now I have 100% coverage on all the main files and fixe the gitignore. The test file is still missing 3 lines, I added #pragma: no cover, since they are test views and the point is that the decorator redirects before they are run. I could maybe mock them but I think it's clearer to have them as functions with the decorator...but for some reason coverage isn't respecting the pragma. Investigating why...

[dhermes]

Check out .coveragerc the NO COVER has to be uppercase. (I just did git grep -i pragma to get an idea.)

[waprin]

Wow, thanks for the headsup on the pragma case sensitivity, it was really confusing me, forgot to check all the hidden files! Ok, now I should have 100% test coverage. I also added NO COVER to apps.py as well, it's covered in 2.7+ but it skips the code in 2.6, and since the 2.6 case isn't run by coverage the 'else' branch isn't covered. It's basically just a metadata class for recent Django versions, which dropped support for 2.6 and so the version of Django tox installs for 2.6 doesn't have the imports.

So I think this is ready for further review or merge whenever you or nathaniel find the time. jonparrot already did a review.

[theacodes]

Summoning @anthmgoogle, he should give this a review as well.

[dhermes]

Sorry, I don't plan on reviewing (not enough cycles). I was just "protecting" my work towards #212 by making sure big changes were fully tested.

I actually took a stab at testing django_orm.py and stopped at the point that I'd need a test DB. Now that you've added one, maybe you could easily cover the rest of django_orm.py (just the Storage class) and get us one step closer to 100% coverage?

[nathanielmanistaatgoogle]

I am able to review this but it would be best if I were last. Poke me after both @jonparrott and @anthmgoogle are satisfied?

[waprin]

@dhermes Will do, thanks for the help. I planned to do that in a separate PR as part of Storage improvement efforts but I could add it onto this one. Also curious if it would make sense to move django_orm into this package or if that's not ok since it breaks existing code (I'm guessing it's ok since this library has releases).

@nathanielmanistaatgoogle sounds good to me, @jonparrott already took a look (and it's a lot of extremely similar code to his own) but I'll hold off for @anthmgoogle and ping you after.

[theacodes]

I'm satisfied in terms of overall approach, parity with flask_util, documentation, and style (though I'm not nearly as thorough as Nathaniel). @anthmngoogle to confirm it fits with his expectations to satisfy #223.

[dhermes]

@waprin You should do it in a separate PR. The smaller a PR is, the easier it is on everyone. As for moving django_orm, it seems like the right thing to do. An alias could be set up and deprecated slowly over time (@jonparrott has ideas to this effect).

[anthmgoogle]

Finished a review pass. I suggested one follow up feature that could be done at a later time. Some minor suggestions about error messages, but consider this a LGTM from me.

Looking great. Really exciting to see this functionality.

[waprin]

@anthmgoogle Thanks for review, sorry for delay in turnaround. Let me know if you can +1 now and I can pass it along to Nathaniel.

I agree that missing the optional oauth was a major oversight so I added it. Now there are two decorators oauth_required and oauth_enabled, with similar semantics to the webapp2 and flask decorators. The oauth_enabled lets you check if the authorization has started and if not generate the authorization URL. The biggest difference from flask/webapp2 is they attach their instance to the Django request object.

Also based on your comments, I also improved the error messages in the oauth2callback, and reference the Github issue for the pluggable storage feature in the TODO. That is next up on our radar along with some other refactoring.

[anthmgoogle]

LGTM.

[waprin]

@nathanielmanistaatgoogle ready for your pass when you have a chance.

[nathanielmanistaatgoogle]

I am oversubscribed for the next few weeks but happy to accommodate if necessary; how time-sensitive is this?

[theacodes]

I suppose that's up to @anthmgoogle.

I don't want to start on #326, #319, and #225 until this is in, but those are rather long-tail issues.

[waprin]

@nathanielmanistaatgoogle It's the season of giving, perhaps you are ready to give this another pass. I know you are tied up in storage PR atm, just your weekly friendly ping this is still here.

[nathanielmanistaatgoogle]

Thank you for your patience; I am finally getting to this in the next hour.

I apologize if this is slow to swap back into my working memory and I ask the same questions I asked last month.

[nathanielmanistaatgoogle]

I'm somewhat bothered by the structure of this - rather than a new package with a non-empty __init__.py and several other modules, I would sort of expect behavior like this to land in a django_util.py module in the oauth2client package with other behavior included in package-private helper modules. How much control do we have over how this code is arranged into modules and packages? How strongly is Django pushing the design of this code into its current shape?

[waprin]

Not sure exactly what you mean by package-private helper modules - I am guessing you mean everything that is part of the public API goes in oauth2client/django_util.py and everything else goes into a helper module or two? I originally did this PR as one file, and it wouldn't be too much work to go back. Django doesn't really force you in any technical sense. However, I laid it out the way I did so it mirrors other Django apps/extensions I looked at:

From the Django project::
https://github.com/django/django/tree/master/django/contrib/admin
https://github.com/django/django/tree/master/django/contrib/auth

In the wild:
https://github.com/ui/django-rq/tree/master/django_rq
https://github.com/omab/django-social-auth/tree/master/social_auth

So it's a choice between being closer to Django style or other extensions in this library (like Flask). @jonparrott suggested I make it closer to Django which made sense to me. Also, keep in mind we are probably going to move this to a contrib package anyway, #326, which also tilts my opinion towards keeping closer to Django style.

But, ultimately I'm not super committed to one way or the other, and if you have something else in mind, I'm fine to change it to whatever you'd like.

[nathanielmanistaatgoogle]

Yeah, I'm now persuaded that the way this is laid out is just fine.

What's with the failing test result?

Does this need to wait for anything else to go in before it can be merged?

[dhermes]

Fix the broken RST in the docstring

Warning, treated as error:
/home/travis/build/google/oauth2client/oauth2client/django_util/decorators.py:docstring
of oauth2client.django_util.decorators.oauth_enabled:27: WARNING: Field list ends
without a blank line; unexpected unindent.

[waprin]

@nathanielmanistaatgoogle had to rebase, ready to merge when you are.

[nathanielmanistaatgoogle]

Now that the contrib directory exists, should this code be placed there?

[theacodes]

Yes, if you've rebased, @waprin, might as well go ahead and move it. Otherwise I can just move it when I move everything else.

[waprin]

Ok I'll move it now

[waprin]

Ok, moved to contrib and I did some double manual checking so I think it's good to go. @nathanielmanistaatgoogle

[dhermes]

Yay!

[anthmgoogle]

Great to see this feature. Thanks all.