-
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-17458039 - Handle error states for social/passwordless login and reset password #2185
base: feature-passwordless-social-login
Are you sure you want to change the base?
@W-17458039 - Handle error states for social/passwordless login and reset password #2185
Conversation
@yunakim714 It seems like users aren't able to resubmit for passwordless login after they cause an error. Steps to reproduce:
Expected:
Actual:
|
@hajinsuha1 Resolved the bug! Let me know if you still see the issue. Thanks for pointing that out! |
if (res.status !== 200) { | ||
const errorData = await res.json() | ||
throw new Error(`${res.status} ${errorData.message}`) | ||
} |
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 moving the logic to throw the error message into commerce-sdk-react?
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.
We could... Is there a specific reason to move this to the sdk?
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 guess it would be to simplify the code within the retail react app, if we give the responsibility of throwing the error to the sdk the retail react app doesn't have to worry about checking the status code and can just catch the error when it occurs.
it'll also reduce code duplication, for example for passwordless checkout we wouldn't need to duplicate this logic
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.
+1 on moving it to the sdk
/callback_uri doesn't match/i, | ||
/error getting user info/i, | ||
/passwordless permissions error/i, | ||
/client secret is not provided/i, |
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.
Where do we based these messages on? I assume this is from API. What happens if the API adds/removes these messages?
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.
Yes these messages are from the API. If the error message does not match any of these, the error will show the generic API_ERROR_MESSAGE
, which is "Something went wrong! Please try again."
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.
my main concern is to have to maintain this list of constant. In a case where they change a bit of wording and an error will become obsolete.
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.
Looking at the spreadsheet of error states I wonder if instead of matching by error message regex we should base it on status code? It seems like it would be possible
@@ -76,7 +77,9 @@ const SocialLogin = ({form, idps}) => { | |||
redirectURI: redirectURI | |||
}) | |||
} catch (error) { | |||
const message = formatMessage(API_ERROR_MESSAGE) | |||
const message = /redirect_uri doesn't match/.test(error.message) | |||
? formatMessage(FEATURE_UNAVAILABLE_ERROR_MESSAGE) |
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.
To think about this message a little bit, if I recall correctly, if we the social config is not set up correctly, the button to login using socialLogin would not show. This implies the feature should be available. Should we use a better error message? 🤔
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.
why don't we use a generic error instead for simplicity?
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.
When discussing with Nathan we decided that if the user has not configured this feature correctly, then we should show this "feature not available" error. Even if the config is not set up correctly, if the user has enabled social login in the config, the button using social login will show.
@yunakim714 Would it possible to provide some steps on how to reproduce these error from the sheet? |
@alexvuong @hajinsuha1 I added a Steps to Reproduce column in the spreadsheet so you guys can better test this feature! |
type: 'manual', | ||
message: formatMessage(API_ERROR_MESSAGE) | ||
}) | ||
const message = /error getting user info/i.test(error.message) |
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 should we also have this error display the CREATE_ACCOUNT_FIRST_ERROR_MESSAGE
?
{
"status_code" : "404 NOT_FOUND",
"message" : "User not found"
}
Steps to reproduce:
- enter an email of a user that doesn't exist like
a@s
- Click
Continue Securely
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.
Sorry now that I'm trying on your env https://wasatch-mrt-social-login-poc.mrt-storefront-staging.com/ (bvgn_stg) I see it does display the correct error msg while my env that uses zzrf-001
(https://wasatch-mrt-jinsu.mrt-storefront-staging.com/) doesn't.
It looks like bvgn_stg
throws a 502 while zzrf_001
throws a 404. I wonder if this is an expected API change?
@@ -46,7 +46,8 @@ const ResetPasswordLanding = () => { | |||
await resetPassword({email, token, newPassword: values.password}) | |||
navigate('/login') | |||
} catch (error) { | |||
const message = /Unauthorized/i.test(error.message) | |||
const errorData = await error.response?.json() | |||
const message = /invalid token/i.test(errorData.message) |
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/non-blocking: Should we move this to constant? 🤔
Description
Before releasing social and passwordless login, we need to ensure that all error states are covered and presented in the UI.
The following spreadsheet details different error scenarios for social and passwordless login, and what error message is prompted after the error: Social/Passwordless Login Error States
Types of Changes
Changes
How to Test-Drive This PR
Changes are deployed to: https://wasatch-mrt-social-login-poc.mrt-storefront-staging.com/
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization