Closed mlavin closed 10 years ago
Thank you for all your work.
Your changes fix appengine inequality queries, but there must be a problem with __isnull
in appengine. After loading the fixture and filling facebook provider with random key/secret, I still get:
In [1]: from allaccess.models import Provider
In [2]: Provider.objects.all().count()
Out[2]: 6
In [3]: Provider.objects.get(name='facebook').enabled()
Out[3]: True
In [4]: Provider.objects.enabled()
Out[4]: []
In [5]: q = {'consumer_secret__isnull': False}
In [6]: Provider.objects.filter(**q)
Out[6]: []
In [7]: Provider.objects.filter(consumer_secret=None).count()
Out[7]: 5
In [8]: Provider.objects.filter(consumer_secret=None)
Out[8]: [<Provider: bitbucket>, <Provider: twitter>, <Provider: microsoft>, <Provider: google>, <Provider: github>]
In [9]: Provider.objects.exclude(consumer_secret=None)
Out[9]: []
So, this pull request can probably be merged, but enabled()
is sadly still not working.
That's frustrating but thanks for the update. I'd really like to find a way to work but I'm not very familiar with running Django on the App Engine.
For the views it would be easy to add additional checks to the instance enabled()
. The context_processor is a little messier. It's fairly reasonable to filter in Python (there aren't going to be 100s or 1000s of providers) but you want to make sure that it is lazily evaluated since the context_processor is run on every render though the context might not be used. Maybe djang.utils.functional.lazy
can address that.
Updated to simply remove the use of enabled
which is more or less your original change. However the views and context processors have been updated to check for the enabled providers.
This is an alternate approach to supporting App Engine based on #38. This attempts to discover if it is run on the App Engine and adjusts the query to work around the restriction. While it opens up a minor edge case for App Engine users is seems like this change is simple enough to include without hurting anyone.
Any thoughts @seguri?