-
Notifications
You must be signed in to change notification settings - Fork 405
Signup redirect url parameter #530
base: master
Are you sure you want to change the base?
Conversation
I just realized signin form sends the next field as a post field. I will close this and update signup to act more like that. |
I added There really isn't a reason to send the redirect url as a post parameter. The post parameter comes from the initial url parameter, which gets sent through a post request anyway. I am not sure why a few tests fail. |
@Rsgm yeah, sending this redirect value additionally through POST form seems pretty odd at first glance. But after some thought I guess it is designed that way to make whole solution more robust against subtle errors/mistakes made often by developers. Consider a case when userena user overrides "sing in" template with something like following:
This way you could accidentally disable whole redirection feature by using I won't argue if silently dealing with such cases is bad or good. It is great that you choose the approach that is consistent with the one that we have already have in "sign in" flow because it will be less surprising to users. Now back to the PR. I will try to reproduce errors we see on Travis in my local environment. I have tried already to rerun tests in Travis and I'm pretty sure it is not a random failure. Anyway, it is interesting that some tests fail only on py32-django17 and py32-django18. Have you tried to reproduce these errors in your local environment? |
Thank you for the explanation. I guess I didn't know forms without an action use the current url. I agree, that is probably a good check to have. As for the tests. I only tested it on 2.7 and 3+, since I knew Travis would check the rest. I will try on my machine with all the versions though. I apologize for the coverage going down. I did add a test, but unit tests can't really reflect this functionality well. Last thing. I wasn't sure if this would fit in the scope of this project, but there is no real way to do it from outside of the app. Hopefully others find it useful too. |
The #535 was the reason of failures in CI and was stopping this PR from merging. Fortunately, it is finally resolved. Could you rebase your PR on current master so we can have it tested in Travis again? When doing rebase you can also squash your changes in order to keep commit log more concise. |
This works the same as the signin redirect. If a user was trying to access a page and gets redirected to sign in or sign up, after succeeding they will be redirected from where they came. Since a user can only be redirected to either the signin or signup page, it is beyond the scope of this feature to pass the query parameter in the url to the other page when requested. Updated signup redirect to work like signin Added signup redirect for SIGNIN_AFTER_SIGNUP
I apologize, I only just got around to fixing my branch. I updated this and my other previous merge request. Assuming everything passes, the previous request(#515) may need to be merged before this(I am certain how branches merge with the same commit). |
The travis error is looks like it is failing trying to set up the environment. It also failed a different python/django version on my other pull request, #515. |
This works the same as the signin redirect. If a user was trying to access a page and gets redirected to sign in or sign up, after succeeding they will be redirected from where they came.
Since a user can only be redirected to either the signin or signup page, it is beyond the scope of this feature to pass the query parameter in the url to the other page when requested.