-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow redirect to MagicLink after Invalid Validation Code #5150
Allow redirect to MagicLink after Invalid Validation Code #5150
Conversation
Lint error as the PR is WIP and I've added some console to test |
Moving your question posted here. Since its more technical i think it will be better discussed here. So this part,
I tested your code. So firstly i don't see where you're doing the redirect. For example here is the url i used - http://localhost:8080/setpassword/10/DWGZCFRKT. When i entered the new password i noticed the API failed which is expected. After this there are two parts to address that i believe your code hasn't captured,
To solve these, Let me know if those make sense. |
Hi, Thanks for the response.
Okay I'll update the credentials in Onyx. But one addition would be if you see currently it is going to the
I did put |
No i don't have to send success just to send data. If you check response in catch block even now it has data. It just doesn't have the email which we should pass so you can use it. |
You're right. Just saw. Makes sense. Thanks 👍 |
Hi @chiragsalian Let me know once the API is ready and I'll finish this today. |
Sorry, i didn't get to this today, but its on my plate to work on for tomorrow. Once i create the return param i can test it out with your PR but even if my code is merged tomorrow it would take 3 business days before its live and we can merge your PR. Just letting you know. |
Should I then assume that I receive the param and then update this PR so that it is easier for you to test? |
Yes definitely. |
I've made the change. Just so you know, in the catch statement if you're passing response. The email will be under |
Thanks I’ll update the code over the weekend. |
@chiragsalian I've updated the PR and manually tested the flow. I've got one query though. The |
src/languages/es.js
Outdated
@@ -344,6 +344,7 @@ export default { | |||
linkHasBeenResent: 'El enlace se ha reenviado', | |||
weSentYouMagicSignInLink: ({loginType}) => `Hemos enviado un enlace mágico de inicio de sesión a tu ${loginType}.`, | |||
resendLink: 'Reenviar enlace', | |||
validationCodeFailedMessage: 'Parece que ha habido un error con tu enlace de validación. O bien el enlace de validación ha caducado, o su cuenta no existe.', |
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.
Just checking but did we run this translation past the Español team here at Expensify?
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 copy wasn't confirmed. I reached out to the Español team and will let you guys know once i get copy confirmation.
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.
The copy i received that we should use here is,
Parece que hubo un error con el enlace de validación. El enlace de validación ha caducado o tu cuenta no existe.
src/libs/actions/Session.js
Outdated
@@ -126,6 +127,7 @@ function fetchAccountDetails(login) { | |||
validated: response.validated, | |||
closed: response.isClosed, | |||
forgotPassword: false, | |||
validationCodeFailedMessage: null, |
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'm not sure if this is the best way to handle these errors.
It looks like this will set the translation key for the error message?
I think it would be better to make this an object with a key of errors
rather than have an object key per error. We are using a similar strategy in other forms so it would be a good idea to align on this and keep things consistent.
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.
Spoke to marc a bit about this let's actually change this to a boolean validateCodeExpired: false
here and not save translations keys.
src/libs/actions/Session.js
Outdated
.finally(() => { | ||
}).catch((errResponse) => { | ||
const login = lodashGet(errResponse, 'data.email', null); | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {validated: false, validationCodeFailedMessage: 'resendValidationForm.validationCodeFailedMessage'}); |
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 we display the error in the UI then we can use the correct translation key. I think there's no reason to persist it in device storage.
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.
WRt to my previous comment, this now becomes validateCodeExpired: true
here.
@@ -63,7 +66,7 @@ class ResendValidationForm extends React.Component { | |||
*/ | |||
validateAndSubmitForm() { | |||
this.setState({ | |||
formSuccess: this.props.translate('resendValidationForm.linkHasBeenResent'), | |||
formSuccess: this.props.translate(''), |
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 are we translating a key of ''
... ?
@@ -34,6 +34,9 @@ const propTypes = { | |||
closed: PropTypes.bool, | |||
}), | |||
|
|||
/** Title to be shown in the form */ | |||
titleMessage: PropTypes.string.isRequired, |
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 couple of issues with this:
- We are not passing a message here we are passing a translation key
- It would be better to pass along something like an
errors
object and then have the form itself define the translation keys to use.
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.
Discussed with marc on this one. So yeah we agree this variable name does not represent what it is. In your code its translation keys and not a message so its confusing.
Let's translate in the parent component so that this remains titleMessage and hence keeps the child component clean.
src/pages/signin/SignInPage.js
Outdated
? '' | ||
: this.props.translate(`welcomeText.${showPasswordForm ? 'phrase4' : 'phrase1'}`); | ||
|
||
const resendLinkTitleMessage = this.props.account.validationCodeFailedMessage ?? 'resendValidationForm.weSentYouMagicSignInLink'; |
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.
AFAIK we are not widely using the null coalescence operator in our JS code and standardize on lodashGet()
for many things. This should be something like
lodashGet(this.props.account, 'validationCodeFailedMessage', 'resendValidationForm.weSentYouMagicSignInLink')
But as mentioned elsewhere ... I don't think it's correct to store the translation key.
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.
the data param in the API response looks like an array but you've mentioned the key as response.data.email
Its an object. So your code i.e., lodashGet(errResponse, 'data.email', null);
is good.
src/languages/es.js
Outdated
@@ -344,6 +344,7 @@ export default { | |||
linkHasBeenResent: 'El enlace se ha reenviado', | |||
weSentYouMagicSignInLink: ({loginType}) => `Hemos enviado un enlace mágico de inicio de sesión a tu ${loginType}.`, | |||
resendLink: 'Reenviar enlace', | |||
validationCodeFailedMessage: 'Parece que ha habido un error con tu enlace de validación. O bien el enlace de validación ha caducado, o su cuenta no existe.', |
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 copy wasn't confirmed. I reached out to the Español team and will let you guys know once i get copy confirmation.
src/libs/actions/Session.js
Outdated
@@ -126,6 +127,7 @@ function fetchAccountDetails(login) { | |||
validated: response.validated, | |||
closed: response.isClosed, | |||
forgotPassword: false, | |||
validationCodeFailedMessage: null, |
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.
Spoke to marc a bit about this let's actually change this to a boolean validateCodeExpired: false
here and not save translations keys.
src/libs/actions/Session.js
Outdated
.finally(() => { | ||
}).catch((errResponse) => { | ||
const login = lodashGet(errResponse, 'data.email', null); | ||
Onyx.merge(ONYXKEYS.ACCOUNT, {validated: false, validationCodeFailedMessage: 'resendValidationForm.validationCodeFailedMessage'}); |
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.
WRt to my previous comment, this now becomes validateCodeExpired: true
here.
@@ -34,6 +34,9 @@ const propTypes = { | |||
closed: PropTypes.bool, | |||
}), | |||
|
|||
/** Title to be shown in the form */ | |||
titleMessage: PropTypes.string.isRequired, |
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.
Discussed with marc on this one. So yeah we agree this variable name does not represent what it is. In your code its translation keys and not a message so its confusing.
Let's translate in the parent component so that this remains titleMessage and hence keeps the child component clean.
@@ -84,7 +87,7 @@ class ResendValidationForm extends React.Component { | |||
<> | |||
<View> | |||
<Text> | |||
{this.props.translate('resendValidationForm.weSentYouMagicSignInLink', { | |||
{this.props.translate(this.props.titleMessage, { |
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.
As per this comment. let's translate in the parent.
Thanks @marcaaron and @chiragsalian for the comments. I’ll update the PR by tomorrow. |
@chiragsalian PR Updated |
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.
Left some minor comments, Rest of the code looks good to me and works well.
src/pages/signin/SignInPage.js
Outdated
|
||
const validateCodeExpired = lodashGet(this.props.account, 'validateCodeExpired', false); | ||
|
||
const resendLinkTitleMessage = this.props.translate(`resendValidationForm.${validateCodeExpired ? 'validationCodeFailedMessage' : 'weSentYouMagicSignInLink'}`, |
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 think there is too much here. This line is not very readable and it took me a while to understand why loginType is there for validationCodeFailedMessage
(took me a while to realize its not needed for validationCodeFailedMessage but is used just for weSentYouMagicSignInLink)
I personally feel this would be easier read if written like so,
let resendLinkTitleMessage = '';
if (validateCodeExpired) {
resendLinkTitleMessage = this.props.translate('resendValidationForm.validationCodeFailedMessage');
} else if (showResendValidationLinkForm) {
resendLinkTitleMessage = this.props.translate('resendValidationForm.weSentYouMagicSignInLink', {
loginType: (Str.isSMSLogin(this.props.credentials.login)
? this.props.translate('common.phoneNumber').toLowerCase()
: this.props.translate('common.email')).toLowerCase(),
});
}
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.
Okay, thanks. I'll update and raise the PR. Somewhere earlier I had seen that we prefer ternary operator over if-else
block.
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.
Yeah, normally we prefer ternary. This situation is a little different though, with the ternary the operations and arguments involved become hard to follow so I feel like if-else is a lot cleaner here.
src/languages/en.js
Outdated
@@ -344,6 +344,8 @@ export default { | |||
linkHasBeenResent: 'Link has been re-sent', | |||
weSentYouMagicSignInLink: ({loginType}) => `We've sent a magic sign in link to your ${loginType}.`, | |||
resendLink: 'Resend link', | |||
validationCodeFailedMessage: 'It looks like there was an error with your validation link. Either the validation link has expired, or your account does not exist.', | |||
|
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.
remove unnecessary line break here.
What's the status here? |
Haven't got a response here so thinking maybe we must consider this PR abandoned. |
@marcaaron Sorry I somehow missed this one. I've responded to your comment above. |
…on-code-expiry # Conflicts: # src/components/WelcomeText.js # src/pages/signin/SignInPage.js
@marcaaron PR updated |
src/libs/actions/Session/index.js
Outdated
Onyx.merge(ONYXKEYS.ACCOUNT, {error: Localize.translateLocal('setPasswordPage.accountNotValidated')}); | ||
const login = lodashGet(response, 'data.email', null); | ||
Onyx.merge(ONYXKEYS.ACCOUNT, { | ||
validated: true, accountExists: true, validateCodeExpired: true, error: Localize.translateLocal('setPasswordPage.accountNotValidated'), |
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.
Does the above change make sense?
It makes sense as a quick solution. However, based on the full conversation in this PR so far - it isn't the one we want and will introduce inconsistency with how we handle errors.
Please try to ask clarifying questions before proposing alternatives, because I'm not sure if we are seeing eye to eye on the problem. I will try to be as specific as possible so we can move on from this PR...
The issue is with setting validated: true
.
It seems the only reason we need to set this here is so that the correct error will show here:
App/src/pages/signin/ResendValidationForm.js
Lines 90 to 105 in 361f097
const isNewAccount = !this.props.account.accountExists; | |
const isOldUnvalidatedAccount = this.props.account.accountExists && !this.props.account.validated; | |
const isSMSLogin = Str.isSMSLogin(this.props.credentials.login); | |
const login = isSMSLogin ? this.props.toLocalPhone(Str.removeSMSDomain(this.props.credentials.login)) : this.props.credentials.login; | |
const loginType = (isSMSLogin ? this.props.translate('common.phone') : this.props.translate('common.email')).toLowerCase(); | |
let message = ''; | |
if (isNewAccount) { | |
message = this.props.translate('resendValidationForm.newAccount', { | |
login, | |
loginType, | |
}); | |
} else if (isOldUnvalidatedAccount) { | |
message = this.props.translate('resendValidationForm.unvalidatedAccount'); | |
} else if (this.props.account.validateCodeExpired) { | |
message = this.props.translate('resendValidationForm.validationCodeFailedMessage'); |
The isOldUnvalidatedAccount
variable needs to be false
and I suspect that's only reason we are setting it to true
. But it is not the correct state for the account. We must change the logic that shows the errors not the state of the account.
@marcaaron I've updated the PR. Hope this does make sense. Thanks for being patient on this one. It has gone through multiple iterations and is a lot different from the initial changes. Appreciate your help. Will make any changes if we're still on a different page. |
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 @akshayasalvi, doesn't seem like my feedback was understood so I suggested the changes we need to make.
@marcaaron @chiragsalian Is there anything else that needs to be done in this PR from my side? |
Nope, not at the moment. I was off a few days for the new years but i'll test/review your PR today 👍 |
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.
Code LGTM and works well. I'm going to merge this PR since it would unblock others waiting on this functionality.
I've left one NAB to address so can you make a follow up for it.
Additionally the screenshot does not match what actually happens so can you update it. The copy between the two are different. This is the copy your PR will actually give,
phrase2: 'Money talks. And now that chat and payments are in one place, it\'s also easy.', | ||
phrase3: 'Your payments get to you as fast as you can get your point across.', | ||
phrase4: 'Welcome back to the New Expensify! Please enter your password.', | ||
welcomeBack: 'Welcome back to the New Expensify! Please enter your password.', |
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'm not a fan of these names, especially when i read it in code like so,
`welcomeText.${showPasswordForm ? 'welcomeBack' : 'welcome'}`
Since its confusing to me why if showPasswordForm is true do we want welcomeBack versus welcome. Plus variables should be named after their intention of like what they store and not their parent/class names.
So in this case welcome
, wants the phone or email so i would suggest - enterPhoneOrEmail
. This becomes welcomeText.enterPhoneOrEmail which is a lot cleaner as welcomeText.welcome is redundant.
As for welcomeBack
, it wants the password so i would suggest - enterPassword
.
So now the logic becomes,
`welcomeText.${showPasswordForm ? 'enterPassword' : 'enterPhoneOrEmail'}`
which is a lot more verbose and cleaner imo.
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 is a NAB - not a blocker.
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.
That's a good point. In general, feels like the names for these translation keys is a bit all over the place. Wondering if we can maybe come up with some standard guidance for how to name them and then get everyone to apply it in a consistent way?
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.
Wondering if we can maybe come up with some standard guidance for how to name them and then get everyone to apply it in a consistent way?
Yes, love it.
🚀 Deployed to staging by @chiragsalian in version: 1.1.24-19 🚀
|
🚀 Deployed to production by @francoisl in version: 1.1.24-19 🚀
|
@chiragsalian Can you please review?
Details
Fixed Issues
$ #4737
Tests
QA Steps
new.expensify.com/<accountId>/<code>
Example URL :
http://localhost:8080/setpassword/10504929/FGVGEKKUJ
Tested On
Screenshots
Web
v2
Mobile Web
Desktop
iOS
Android