Skip to content
This repository has been archived by the owner on Jan 18, 2025. It is now read-only.

Django Refactors #546

Merged
merged 1 commit into from
Jul 26, 2016
Merged

Django Refactors #546

merged 1 commit into from
Jul 26, 2016

Conversation

waprin
Copy link
Contributor

@waprin waprin commented Jul 14, 2016

Primarily addressed #494

Moves all Django code into contrib.django_util.

Adds new functionality to decorators and views that integrate with ORM
storage.

Adds two Django samples for the primary two use cases - using Google
OAuth as the primary auth, and integrating it into exisiting user
system.

DjangORMStorage is backwards compatible with old django_orm storage.

Fixes issue where Django storage was always creating new Models.

Move tests into Django package and have them create an actual test app
to create test databases etc.

Kills FlowField as its not useful.

'credentials_property': 'credential'
}

Where path.to.model class is the fully qualified name of a django.db.model

This comment was marked as spam.

@waprin
Copy link
Contributor Author

waprin commented Jul 14, 2016

Also should address #528 .

cc @letsmakesense @theromis in case you have opinions since you commented on Django stuff before.

"""

from importlib import import_module

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

if not request.user.is_authenticated():
return shortcuts.redirect(
'{}?next={}'.format(
settings.LOGIN_URL, request.path))

This comment was marked as spam.

This comment was marked as spam.

@theacodes
Copy link
Contributor

Finished a pass, please let me know when you're ready for another round.

Also, to reduce the review burden here, can you submit the samples as a separate PR after this is done?

@waprin waprin mentioned this pull request Jul 15, 2016
@waprin waprin force-pushed the refactor branch 3 times, most recently from 74b40fd to be34903 Compare July 15, 2016 22:52
@waprin waprin force-pushed the refactor branch 4 times, most recently from e32efa0 to e281441 Compare July 20, 2016 20:43
@waprin
Copy link
Contributor Author

waprin commented Jul 20, 2016

@nathanielmanistaatgoogle ready for another pass at your convenience, thanks

credentials into an existing Django user system.

Returns:
A tuple containing three strings, or None. If

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

Third pass done. It was much shorter; I think this is nearing completion! :-)

@waprin
Copy link
Contributor Author

waprin commented Jul 21, 2016

@nathanielmanistaatgoogle ready for another pass. Thanks for thorough review, still need to improve my attention to small details! Coveralls is complaining but all the files here have 100% coverage, unrelated files aren't at 100%. I previously was amending my commits and force pushing the branch, but I've since realized pushing commits that respond to comments and then squashing before merge makes it easiest to review, all the commits with TO SQUASH will be squashed once ready to merge.

@theacodes
Copy link
Contributor

@waprin there are some conflicts on this branch, can you resolve?

@waprin
Copy link
Contributor Author

waprin commented Jul 25, 2016

@jonparrott rebased. still will squash pre-merge so @nathanielmanistaatgoogle can view diffs from comments.

@nathanielmanistaatgoogle
Copy link
Contributor

Looks good to me.

@jonparrott, anything else from you coming out of the last week of review?

What's with the failing test result?

@theacodes
Copy link
Contributor

@waprin can you get travis green again? Once you do we can merge.

@jonparrott, anything else from you coming out of the last week of review?

This is the last one I need for 3.0.0. :) Thanks.

Moves all Django code into contrib.django_util.

Adds new functionality to decorators and views that integrate with ORM
storage.

DjangORMStorage is backwards compatible with old django_orm storage.

Fixes issue where Django storage was always creating new Models.

Move tests into Django package and have them create an actual test app
to create test databases etc.

Kills FlowField as it’s not useful.
@waprin
Copy link
Contributor Author

waprin commented Jul 26, 2016

@jonparrott fixed, just accidentally let the files I deleted back in in the merge.

Thank you for all reviews.

I still have those samples that I took out of this PR. Though I assume that shouldn't block the release anyway since they are not part of the core lib.

@theacodes
Copy link
Contributor

They won't block release. Feel free to send that PR whenever.

@theacodes theacodes merged commit 25165ad into googleapis:master Jul 26, 2016
@theacodes theacodes mentioned this pull request Jul 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants