-
Notifications
You must be signed in to change notification settings - Fork 144
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
@W-17550783 - [Passwordless login] Redirect customer to page prior to login #2221
base: feature-passwordless-social-login
Are you sure you want to change the base?
@W-17550783 - [Passwordless login] Redirect customer to page prior to login #2221
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow it was simpler than i thought! nice 🎉
packages/template-retail-react-app/app/pages/checkout/partials/contact-info.test.js
Outdated
Show resolved
Hide resolved
@@ -105,6 +105,8 @@ export const AuthModal = ({ | |||
|
|||
const handlePasswordlessLogin = async (email) => { | |||
try { | |||
// Save the path where the user logged in | |||
window.localStorage.setItem('returnToPage', window.location.pathname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason behind using localStorage? Can we use a query param to avoid having to handle this variable? Something like https://pwa-kit.storefront.com/login?token&1234&redirectTo=/checkout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few benefits of using query param than localStorage. One of which is it is not tied to a device. Imagine a shopper clicks login with link on a laptop, and then they check email and click on the magic link to logic. Can they resume their shopping from their?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexvuong To send the redirect_url
I would have to pass it in as part of the callbackURL param when calling the 1st authorizePasswordless
API call so I thought this method would be more straightforward (though Blair did say that passing in the query params as part of the callback URL should be fine).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yunakim714 Why would we need to pass it into the callbackURL ? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we try the query param solution if possible since the localStorage can potential cause more bugs. There are some edge cases that will potentially disable the redirection. Say, what if users clear their storage before they click on the magic link? or when they use incognito browser?
packages/template-retail-react-app/app/pages/checkout/partials/contact-info.jsx
Outdated
Show resolved
Hide resolved
mockAuthHelperFunctions[AuthHelpers.AuthorizePasswordless].mutateAsync | ||
).toHaveBeenCalledWith({ | ||
userid: validEmail, | ||
callbackURI: 'https://webhook.site/27761b71-50c1-4097-a600-21a3b89a546c?redirectUrl=/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is there a way to add a unit test that verfies when passwordlessConfigCallback
is a relative path it creates the URL correclty?
packages/template-retail-react-app/app/pages/checkout/partials/contact-info.jsx
Outdated
Show resolved
Hide resolved
if (redirectPath) { | ||
navigate(redirectPath) | ||
setRedirectPath('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we consider adding a unit test to verify that redirectPath is correctly navigated to?
packages/template-retail-react-app/app/pages/reset-password/reset-password-landing.jsx
Show resolved
Hide resolved
@yunakim714 i'm finding when I try to login via the /login page, the clicking the link redirects me to a page not found page |
@@ -77,7 +79,7 @@ export async function jwksCaching(req, res, options) { | |||
|
|||
// JWKS rotate every 30 days. For now, cache response for 14 days so that | |||
// fetches only need to happen twice a month | |||
res.set('Cache-Control', 'public, max-age=1209600') | |||
res.set('Cache-Control', 'public, max-age=1209600, stale-while-revalidate=86400') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason we add this in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to use the cached response for 1 day while the server/CDN fetches a fresh response. I added this in to prevent errors from occurring when the cache expires and the server needs to call the /jwks
endpoint again.
} | ||
const redirectTo = redirectPath ? redirectPath : '/account' | ||
navigate(redirectTo) | ||
setRedirectPath('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this line here can potentially cause react warning about unsettable state. We are navigating away before the state can be set.
I tried to check out code locally to test out, but I keep running into error with typescript from commerce-sdk-react despite some effects with workarounds.
Description
As a shopper, I want to be redirected back to the page I was just on after clicking the passwordless login link so that I can continue shopping from where I left off and save time. For example, when the shopper initiates passwordless login from the checkout page, they should be redirected back to the checkout page after clicking the magic link in their email.
Types of Changes
Changes
redirect_url
to the magic link that is sent to the userHow to Test-Drive This PR
Run unit tests:
npm run test app/hooks/use-auth-modal.test.js
npm run test app/pages/login/passwordless-landing.test.js
npm run test app/pages/checkout/partials/contact-info.test.js
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization