-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
handling late logins #33
Comments
Hm, interesting... I quite like this idea. I'm not mad keen on adding yet another setting, but I can definitely see where this might be useful. I wonder if we could store the fact that a user has been authenticated lazily somewhere (session? cookie? - I think session gets flushed at login, iirc) and bulk update? Just thinking on the hoof here. In effect, this is merging users I guess. Thinking further, this might be hard to do in a totally generic way. What if, in your model, the book -> user relationship was unique at the book end? (Unlikely, I know!). So I guess this would indeed need to be opt-in, somehow. More than happy to take a look at any proof-of-concept implementations, or part implementations. |
In fact, maybe breaking this down is the best way to go. We currently have a convert signal, for when a lazy user signs up and becomes a real one. If we added another signal that represented a lazy user signing into an existing account, and the handler got passed the lazy user and the authenticated user, then any app-specific remapping could be done in such a handler. That seems less magical to me. What do you think? |
Alright, here be dragons, but this is a really, really dirty POC. def login(request):
# Probably the easiest way to remember the previous user...
lazy_user = None
if request.user.is_authenticated() and is_lazy_user(request.user):
lazy_user = request.user
# original login view
response = auth_views.login(request)
# Should probably check for POST here, eh...
if lazy_user is not None and isinstance(response, HttpResponseRedirectBase):
request.user.merge_user(lazy_user)
return response And in my user model I have the real magic: def merge_user(self, user):
"""
Merge all user data to this one.
This method will associate every related model from the given
``user`` to this user instance.
"""
for related in user._meta._related_objects_cache:
# could also use a whitelist instead, but excluding lazyuser is crucial here.
if related.name == 'lazysignup:lazyuser':
continue
back_rel = related.field.name
rel_objs = related.model.objects.filter(**{back_rel: user})
rel_objs.update(**{back_rel: self}) To answer your questions: We could provide some sort of boilerplate for doing this, as you can see above. But users will still have to trigger that by themselves. I think this is also the best way to go. Not to force anything on developers but give them the ability to quickly integrate this feature if they want it. |
Thanks for taking the time to sketch this out! Always easier to discuss when you've got real code to look at. That's actually fairly close to what I had in mind when I was talking about merging users (although I'm pretty sure there are higher level APIs available than poking around in a cache). However, I still think it's too magical. There might well be other things that need to happen, and those are bound to be app-specific - the point I made about uniqueness still stands, or people might have a GenericForeignKey, or any number of other things. That's why I think a signal is the way to go here - lazysignup is qualified to know that a user who was lazy has signed in as a regular user, but mucking about in other apps' data doesn't strike me as a good idea. Django itself has signals for login and logout: https://github.com/django/django/blob/master/django/contrib/auth/signals.py ... I see this as an analogous operation. I think it'd certainly be beneficial, though, to note how you might go about doing this in the docs - something like your PoC above. (As an aside, I think 'usually considered bad' is a little strong. As with many things, you can certainly abuse signals - for example, defining a signal and handling it within the same app is probably a bit suspicious - but it's perfectly ok for framework code to define and fire them when interesting things happen.) |
I'm working on sketching something out myself, I'll push a branch when it's ready to take a look at - hopefully what I mean will be clearer when there's code to look at. |
I see the branch is here https://github.com/danfairs/django-lazysignup/tree/33-late-logins |
Right - I started sketching out some ideas, but didn't have time to finish them off. If anyone interested in this wants to lend a hand, they're more than welcome! |
There should be, if possible, a way to gracefully handle registered users that forget to login before using a site's functions that are allowed to lazy users.
Example:
Naturally I'd do this by hand but maybe we could provide a view/decorator that wraps the login view and goes through a list of relations and re-maps them.
Something like
which will cause
login
to remap alluser.books
to the one that just logged in.The text was updated successfully, but these errors were encountered: