[issue6] Google's Users API Integration

http://code.google.com/p/google-app-engine-django/issues/detail?id=6

[mattbrown.nz, 2008-05-09 10:40:19]

http://codereview.appspot.com/908/diff/1/3
File appengine_django/auth/__init__.py (right):

http://codereview.appspot.com/908/diff/1/3#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
You should probably add an Author or Copyright line too.

http://codereview.appspot.com/908/diff/1/3#newcode47
Line 47: 
add extra blank line

http://codereview.appspot.com/908/diff/1/3#newcode50
Line 50: This class is required for the user's email attribute."""
closing """ on next line.

http://codereview.appspot.com/908/diff/1/3#newcode56
Line 56: """Appengine User subclass that mimics the behavior of an Django user.
App Engine (nitpick)

http://codereview.appspot.com/908/diff/1/3#newcode59
Line 59: to make it usable for db.UserProperty().
I don't believe this is possible. See further comments below.

http://codereview.appspot.com/908/diff/1/3#newcode153
Line 153: mail.send_mail(sender=from_email,
The App Engine email API requires from_email to be an app administrator.

There is no easy way for the code to discover this and default to it, but it
would be good to document it in the function docstring as it's the sort of thing
people will easily forget.

http://codereview.appspot.com/908/diff/1/3#newcode160
Line 160: datastore_types._PROPERTY_TYPES.append(DjangoUser)
model properties need to be descended from db.Property and need to provide at
least the make_value_for_datastore and get_value_for_datastore methods. 

This class doesn't provide any of those methods and I can't see how it would be
able to be used as a property within a model. 

You'll need to create a DjangoUserProperty class derived from db.Property to
achieve this.

http://codereview.appspot.com/908/diff/1/3#newcode190
Line 190: 
add blank line

http://codereview.appspot.com/908/diff/1/4
File appengine_django/auth/context_processors.py (right):

http://codereview.appspot.com/908/diff/1/4#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
again, add author or copyright notice.

http://codereview.appspot.com/908/diff/1/4#newcode16
Line 16: Requires AuthenticationMiddleware"""
closing """ should be on next line.

http://codereview.appspot.com/908/diff/1/2
File appengine_django/auth/middleware.py (right):

http://codereview.appspot.com/908/diff/1/2#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
again, add author or copyright line.

http://codereview.appspot.com/908/diff/1/2#newcode29
Line 29: 
add blank line

[Andi, 2008-05-09 20:23:22]

http://codereview.appspot.com/908/diff/1/3
File appengine_django/auth/__init__.py (right):

http://codereview.appspot.com/908/diff/1/3#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> You should probably add an Author or Copyright line too.

Done.

http://codereview.appspot.com/908/diff/1/3#newcode47
Line 47: 
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> add extra blank line

Done.

http://codereview.appspot.com/908/diff/1/3#newcode50
Line 50: This class is required for the user's email attribute."""
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> closing """ on next line.

Done.

http://codereview.appspot.com/908/diff/1/3#newcode56
Line 56: """Appengine User subclass that mimics the behavior of an Django user.
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> App Engine (nitpick)

Done.

http://codereview.appspot.com/908/diff/1/3#newcode59
Line 59: to make it usable for db.UserProperty().
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> I don't believe this is possible. See further comments below.

Maybe this docstring is not clear enough. I don't want to use this class as a
property, but as a value for UserProperty. (see below)

http://codereview.appspot.com/908/diff/1/3#newcode153
Line 153: mail.send_mail(sender=from_email,
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> The App Engine email API requires from_email to be an app administrator.
> 
> There is no easy way for the code to discover this and default to it, but it
> would be good to document it in the function docstring as it's the sort of
thing
> people will easily forget.

The docstring now mentions the email API requirement, from_email parameter isn't
optional anymore.
In addition Django's mail module is imported instead of the mail module found in
google.appengine.api (regarding to issue 9, r16)

http://codereview.appspot.com/908/diff/1/3#newcode160
Line 160: datastore_types._PROPERTY_TYPES.append(DjangoUser)
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> model properties need to be descended from db.Property and need to provide at
> least the make_value_for_datastore and get_value_for_datastore methods. 
> 
> This class doesn't provide any of those methods and I can't see how it would
be
> able to be used as a property within a model. 
> 
> You'll need to create a DjangoUserProperty class derived from db.Property to
> achieve this.

I have to admit, this part maybe needs some discussion. This class should be
both a Django-like request.user attribute and accepted as a value for
db.UserProperty. Here's a short example... Let's assume I have this model
defined:

class MyItem(BaseModel):
  itemdata = db.StringProperty()
  owner = db.UserProperty()

To create a new instance of this model I do: item = MyItem(itemdata="foo",
owner=request.user) from within a view; in templates/views I can call
request.user.is_authenticated() and so on... To make this work DjangoUser needs
to be added to _PROPERTY_TYPES. Then it passes the type check in
google.api.database_types.ToPropertyPb (line 938, it passes the test in line
975, because it is a users.User instance). But please correct me if I completely
misunderstood something here or if you have some other suggestions on how this
class should behave.

http://codereview.appspot.com/908/diff/1/3#newcode190
Line 190: 
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> add blank line

Done.

http://codereview.appspot.com/908/diff/1/4
File appengine_django/auth/context_processors.py (right):

http://codereview.appspot.com/908/diff/1/4#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> again, add author or copyright notice.

Done.

http://codereview.appspot.com/908/diff/1/4#newcode16
Line 16: Requires AuthenticationMiddleware"""
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> closing """ should be on next line.

Done.

http://codereview.appspot.com/908/diff/1/2
File appengine_django/auth/middleware.py (right):

http://codereview.appspot.com/908/diff/1/2#newcode1
Line 1: # Licensed under the Apache License, Version 2.0 (the "License");
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> again, add author or copyright line.

Done.

http://codereview.appspot.com/908/diff/1/2#newcode29
Line 29: 
On 2008/05/09 10:40:20, mattbrown.nz wrote:
> add blank line

Done.

[mattbrown.nz, 2008-05-11 03:01:19]

http://codereview.appspot.com/908/diff/1/3
File appengine_django/auth/__init__.py (right):

http://codereview.appspot.com/908/diff/1/3#newcode160
Line 160: datastore_types._PROPERTY_TYPES.append(DjangoUser)
On 2008/05/09 20:23:23, aalbrecht wrote:

> 975, because it is a users.User instance). But please correct me if I
completely
> misunderstood something here or if you have some other suggestions on how this
> class should behave.

OK, thanks for the example it definitely clarifies the use case. 

I think the way you should do this is to create a new property 

class DjangoUserProperty(UserProperty):
 ...

That property would then take care of up/downscaling a DjangoUser object to/from
a users.User object in the make_value_for_datastore and get_value_from_datastore
methods. 

This has two advantages:
1) You don't need to go poking around with the internals of datastore_types.
Your make_value_for_datastore method ensures that the datastore only ever sees
users.User objects from the property.
2) Retrieving the value of the property will return a DjangoUser rather than a
users.User solving one of the outstanding issues you list. 

Does this sound reasonable to you?

[Andi, 2008-05-13 14:20:23]

New patch set uploaded:
 * DjangoUserModel as a basis for other user-related things
 * better helpers integration by using the Install* routines to setup
authentication

http://codereview.appspot.com/908/diff/1/3
File appengine_django/auth/__init__.py (right):

http://codereview.appspot.com/908/diff/1/3#newcode160
Line 160: datastore_types._PROPERTY_TYPES.append(DjangoUser)
The DjangoUser class is now a model. I think this is necessary to add support
for permissions and several other user related things in the future. Furthermore
the problems introduced with the previous mixture of a users.User subclass and
the Django User behavior are resolved with this solution.
In addition the get_profile() method is implemented, too.

[mattbrown.nz, 2008-05-16 03:59:29]

Hi Andi, 

Sorry for the delay in responding, I don't think I got an email when you posted
your lastest update. 

I'll need another day or two to give your new implementation a proper review,
but I'll get back to you as soon as possible. 

Thanks.

[mattbrown.nz, 2008-05-18 11:23:42]

Looking great.

Just a few minor comments.

http://codereview.appspot.com/908/diff/63/106
File appengine_django/__init__.py (left):

http://codereview.appspot.com/908/diff/63/106#oldcode280
Line 280: setattr(settings, "TEMPLATE_CONTEXT_PROCESSORS", tuple(ctx_procs))
This stanza can be removed completely. the auth context processor was the only
one known to cause issues.

http://codereview.appspot.com/908/diff/63/106
File appengine_django/__init__.py (right):

http://codereview.appspot.com/908/diff/63/106#newcode264
Line 264: allowed_apps = ("django.contrib.auth",)
We're using a blacklist for middleware modules, for consistency I think we
should stick with a blacklist for apps.

http://codereview.appspot.com/908/diff/63/105
File appengine_django/auth/models.py (right):

http://codereview.appspot.com/908/diff/63/105#newcode67
Line 67: assert nickname
These statements are only used to create a user, so move them inside the if.  

email() and nickname() will always return a value, why are the asserts needed?

http://codereview.appspot.com/908/diff/63/105#newcode154
Line 154: self._profile_cache = model.create_profile(self)
else:
 raise DoesNotExist

To stay consistent with Django's behaviour, this function should either return a
profile or raise an Exception. Currently this implementation would return None.

[Andi, 2008-05-18 19:17:46]

Thanks for taking time to review this! The new patchset fixes the problems you
mentioned. To raise an exception in DjangoUser.get_profile() a DoesNotExist
attribute was added to BaseModel using it's metaclass.

Andi

http://codereview.appspot.com/908/diff/63/106
File appengine_django/__init__.py (left):

http://codereview.appspot.com/908/diff/63/106#oldcode280
Line 280: setattr(settings, "TEMPLATE_CONTEXT_PROCESSORS", tuple(ctx_procs))
On 2008/05/18 11:23:43, mattbrown.nz wrote:
> This stanza can be removed completely. the auth context processor was the only
> one known to cause issues.

Done.

http://codereview.appspot.com/908/diff/63/106
File appengine_django/__init__.py (right):

http://codereview.appspot.com/908/diff/63/106#newcode264
Line 264: allowed_apps = ("django.contrib.auth",)
On 2008/05/18 11:23:43, mattbrown.nz wrote:
> We're using a blacklist for middleware modules, for consistency I think we
> should stick with a blacklist for apps.

Ok, there's now a blacklist for this part. I've listed all apps that are
initially set by "django-admin startproject" except the auth app. Maybe there
should be more django.* apps listed here, e.g. django.contrib.admin. But
fetching all apps that depend on a disallowed app is probably a separate
issue...

http://codereview.appspot.com/908/diff/63/105
File appengine_django/auth/models.py (right):

http://codereview.appspot.com/908/diff/63/105#newcode67
Line 67: assert nickname
On 2008/05/18 11:23:43, mattbrown.nz wrote:
> These statements are only used to create a user, so move them inside the if.  
> 
> email() and nickname() will always return a value, why are the asserts needed?

You're right, I've got this part from Rietveld's Account model, but email and
nickname are used slightly different there. Code is cleaned up now.

http://codereview.appspot.com/908/diff/63/105#newcode154
Line 154: self._profile_cache = model.create_profile(self)
On 2008/05/18 11:23:43, mattbrown.nz wrote:
> else:
>  raise DoesNotExist
> 
> To stay consistent with Django's behaviour, this function should either return
a
> profile or raise an Exception. Currently this implementation would return
None.

DoesNotExist is raised, DoesNotExist attribute was added to BaseModel
(PropertiedClassWithDjango)

[mattbrown.nz, 2008-05-19 00:38:02]

Hi Andi, 

This is looking great now, one final issue I found in my presubmit checks:

Now that django.contrib.auth is in installed_apps. The django.contrib.auth.tests
suite is run when we execute 'manage.py test' that test suite obviously doesn't
succeed completely with this replacement module. 

Would you be able to monkeypatch a replacement set of tests over top of the
invalid ones to fix this up?

[Andi, 2008-05-19 04:24:38]

Doc tests added

[Andi, 2008-05-19 11:13:23]

Hi Matt,

I'm sorry, but I had to upload another patch set with some minor changes to
fully support both Django versions.

Andi

[mattbrown.nz, 2008-05-19 14:19:29]

Hi Andi, 

We're getting there!

I still see one error in the tests in the 'dumpdata' command, you should be able
to reproduce it also by running './manage.py dumpdata'

Actually, I think it's two errors because it appears different in 0.96 and 0.97.
At a quick glance it appears something is making a query using a default Django
manager and then hitting some code that doesn't work with the helper.

[Andi, 2008-05-19 20:08:33]

There were several problems with the underlying Django stuff. Permission, Group
and Message models had to be implemented and the DjangoUser had to be renamed to
User. Otherwise the AppCache in django.db.models.loading stores a reference to
these models. This cache is used by dumpdata. Since I don't like to deal with
internals the solution for me was to add the missing models and rename
DjangoUser. If the names are equal to the original Django models, the AppCache
is updated correctly.

Sorry, I've completely misinterpreted the test failures, I thought it was some
other unrelated issue.

In addition I removed some needless imports.