#634#677
Conversation
| if (Input::has('after_signup_redirect')) { | ||
| $confirmationLink.='?after_signup_redirect=/add-location'; | ||
| } | ||
| echo($confirmationLink); |
There was a problem hiding this comment.
Controllers shouldn't echo anything when they're ready to merge. Feel free to troubleshoot temporarily with echo statements but that isn't suitable for the master branch.
| ]); | ||
|
|
||
| } | ||
| else{ |
There was a problem hiding this comment.
Did you run 'composer all'? I have a feeling that else statement would fail the coding style tests.
|
|
||
| public function confirmEmail($user_email, $email_verification_token) | ||
| { | ||
| $after_signup_redirect = Input::get('after_signup_redirect'); |
There was a problem hiding this comment.
I like that you're getting the redirect value from input here but it looks like you're ignoring it everywhere else which could lead to bugs.
When signing up without the add-location redirect, does it correctly stay on the profile or does it incorrectly redirect to add-location?
There was a problem hiding this comment.
Just tested it and it works properly. The way signup is setup right now is that it sends you to a page which then tells you to sign in and not to your profile after you click the confirmation link. The button that tells you to sign in will send you to the profile unless the redirect is there
There was a problem hiding this comment.
Instead of hardcoding the /add-location in the redirect below, you should use $after_signup_redirect. If anyone later passes a different URL segment to confirmEmail, it should redirect there instead of /add-location.
Something like this:
'redirect' => '/signin?after_signin_redirect=' . $after_signup_redirect
|
|
||
| public function confirmEmail($user_email, $email_verification_token) | ||
| { | ||
| $after_signup_redirect = Input::get('after_signup_redirect'); |
There was a problem hiding this comment.
Instead of hardcoding the /add-location in the redirect below, you should use $after_signup_redirect. If anyone later passes a different URL segment to confirmEmail, it should redirect there instead of /add-location.
Something like this:
'redirect' => '/signin?after_signin_redirect=' . $after_signup_redirect
|
@joshi1983 updated the PR |
| return view('pages.signup.success', [ | ||
| 'confirmmessage' => 'Your email has been confirmed.', | ||
| 'can_sign_in' => true, | ||
| 'redirect' => '/signin?after_signin_redirect='.$after_signin_redirect |
There was a problem hiding this comment.
It should be $after_signup_redirect instead of $after_signin_redirect. This causes a bug that I'll fix in a new pull request.
There was a problem hiding this comment.
Pull request #683 fixed it. It was merged and deployed to https://app.accesslocator.com.
2 participants
Addresses #634
Redirects from signup button now.