Skip to content
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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aschriner
Copy link

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
Copy link
Owner

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
Copy link
Author

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
#40 (comment)
.

@danfairs
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
#40 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants