search

danfairs / django-lazysignup

django-lazysignup is a package designed to allow users to interact with a site as if they were authenticated users, but without signing up. At any time, they can convert their temporary user account to a real user account.
BSD 3-Clause "New" or "Revised" License
410 stars 89 forks source link

Update for 1.6, refactor convert view to behave more like auth login view, make post-conversion login optional #40

Open aschriner opened 10 years ago

aschriner commented 10 years ago

There are 3 changes in this PR:

  1. Swapped manage.py for a django 1.6 version
  2. Reconfigured the "redirect_to" logic in the convert view to be more like the auth login view. (A few lines of code have been copy/pasted from auth login). Also, most importantly, allow specifying a post-convert URL in the settings file, like settings.LOGIN_REDIRECT_URL. Added tests for this logic.
  3. Allow disabling of automatic login after lazy user conversion via settings.AUTO_LOGIN_ON_LAZY_CONVERSION (with default to True). Not sure how to test this - suggestions welcome.

Overall, I undertook this set of changes to allow me to use lazysignup with django-registration's 2-step registration process (with email verification). I think this solves issue #4 in a very minimal way. (I plan to redirect users to the "Registration complete; now go check your email" view, and use the "converted" signal to send the email and create the RegistrationProfile instance - the activation code - required by django-registration). I think a similar approach can be used for other verification workflows.

danfairs commented 10 years ago

Hi there - thanks for this! I haven't had time to check this out yet, but I notice it's causing the Travis build to fail. I think it worked before - if we can get this sorted out, I'll do a full review and merge.

Thanks again!

aschriner commented 10 years ago

I looked at the travis detail report and (having never used Travis before) my guess is that it is related to the django 1.6 switch. I naively dropped a "DJANGO_VERSION=1.6" to the travis.yaml env section (thinking that was the list of versions of django you were testing). That very well could be the problem.

Also, I think the manage.py version may be backwards incompatible...so we may need to drop in a version of manage.py that checks the version and responds appropriately. If you can tell from the travis logs if that's the case then we can replace manage.py.

Thanks for your work on lazysignup!

Andy Schriner schrinaw@gmail.com

On Wed, Jul 23, 2014 at 4:33 AM, Dan Fairs notifications@github.com wrote:

Hi there - thanks for this! I haven't had time to check this out yet, but I notice it's causing the Travis build to fail. I think it worked before - if we can get this sorted out, I'll do a full review and merge.

Thanks again!

— Reply to this email directly or view it on GitHub https://github.com/danfairs/django-lazysignup/pull/40#issuecomment-49846699 .

danfairs commented 10 years ago

Right, that all makes sense. I guess we may need to do something clever with manage.py to make it compatible with both, perhaps a (nasty) version check or something.

aschriner commented 10 years ago

Yeah, I don't like the version check idea so much either, but it's probably necessary to run the tests on multiple versions of django. On the bright side, it's really only affecting the running of tests, not the core app code, since that's the only thing the bundled manage.py is used for (right?).

I took another look at the travis output, and on second thought, it doesn't look like a django version issue is causing the current problem (though I expect the version issue will become the next problem). The builds are all failing with

ImportError: 'tests' module incorrectly imported from '/home/travis/virtualenv/python2.7.6/lib/python2.7/site-packages/lazysignup'. Expected '/home/travis/build/danfairs/django-lazysignup/lazysignup'. Is this module globally installed?

Any ideas?

danfairs commented 10 years ago

Weird. I've seen plenty of ImportErrors in my time, but none that looked like that! I guess tests all pass locally? It almost looks like a Travis problem - perhaps Travis builds in one directory and then installs the package into a venv later - and somehow the PYTHONPATH is messed up. But I'm guessing here.

aschriner commented 10 years ago

Yeah, the tests all pass locally for me (running django 1.6, python 2.7.3). I agree - looks like an issue with Travis.

Andy Schriner schrinaw@gmail.com

On Wed, Jul 23, 2014 at 1:29 PM, Dan Fairs notifications@github.com wrote:

Weird. I've seen plenty of ImportErrors in my time, but none that looked like that! I guess tests all pass locally? It almost looks like a Travis problem - perhaps Travis builds in one directory and then installs the package into a venv later - and somehow the PYTHONPATH is messed up. But I'm guessing here.

— Reply to this email directly or view it on GitHub https://github.com/danfairs/django-lazysignup/pull/40#issuecomment-49907029 .