Skip to content
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.

Sign up for GitHub

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

Merged
merged 25 commits into from
Jan 3, 2022
Merged

Allow redirect to MagicLink after Invalid Validation Code #5150

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b10f53f
Allow redirect to MagicLink after Invalid Validation Code
akshayasalvi Sep 6, 2021
0a6362e
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Sep 11, 2021
a1ce081
Added login handling along with navigation
akshayasalvi Sep 11, 2021
0ecd7f7
Updated default value to null
akshayasalvi Sep 12, 2021
3f1ad9e
Changed translation key to flag and move translation to parent component
akshayasalvi Sep 15, 2021
745efbb
Added truthy check
akshayasalvi Sep 15, 2021
103bbc5
Change the condition for translation message
akshayasalvi Sep 16, 2021
1e77d15
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Oct 27, 2021
f278059
Moved the validation message to Link form component
akshayasalvi Oct 27, 2021
8e4f703
Corrected import
akshayasalvi Oct 27, 2021
a1af035
Changed the validation key
akshayasalvi Oct 27, 2021
f25e31f
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Nov 7, 2021
418c8f6
Added `validateCodeExpired` handling for expired links
akshayasalvi Nov 7, 2021
f42b310
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Nov 9, 2021
46cd66d
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Dec 1, 2021
4b4f0ce
Changed unvalidated account flag to validation expired
akshayasalvi Dec 1, 2021
e486081
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Dec 6, 2021
cf0fe90
Rolledback unvalidated account changed and fixed validation link message
akshayasalvi Dec 6, 2021
587948f
Changed keys for welcomeText and resend formflag
akshayasalvi Dec 6, 2021
0bfaf85
Added comment for login var
akshayasalvi Dec 6, 2021
361f097
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Dec 23, 2021
b1ba7fa
Updated comment and setting error null
akshayasalvi Dec 23, 2021
c51f126
Merge branch 'main' of github-personal:akshayasalvi/App into validati…
akshayasalvi Dec 27, 2021
c11c337
Removed the `validate: true` in setpassword
akshayasalvi Dec 28, 2021
6180d9e
Reordered conditions for correct validateExpiry message
akshayasalvi Dec 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one is it? The account doesn't exist or the validation link has expired? Is there no way to tell the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's any way to identify from the API response I can do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, to clarify, I don't think the error makes sense. We are showing it here:

App/src/pages/signin/ResendValidationForm.js

Lines 102 to 103 in 4b4f0ce

} else if (isOldUnvalidatedAccount || this.props.account.validateCodeExpired) {
message = this.props.translate('resendValidationForm.validationCodeFailedMessage');

Why tell the user the account does not exist? It must exist based on those two conditions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thing we are looking at is if the account "exists but is not yet validated" a.k.a. isOldUnvalidatedAccount - but that doesn't mean the account does not exist.


Copy link
Contributor

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.

},
detailsPage: {
localTime: 'Local time',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 hubo un error con el enlace de validación. El enlace de validación ha caducado o tu cuenta no existe',
},
detailsPage: {
localTime: 'Hora local',
Expand Down
16 changes: 12 additions & 4 deletions src/libs/actions/Session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import ONYXKEYS from '../../ONYXKEYS';
import redirectToSignIn from './SignInRedirect';
import * as API from '../API';
Expand Down Expand Up @@ -126,6 +127,7 @@ function fetchAccountDetails(login) {
validated: response.validated,
closed: response.isClosed,
forgotPassword: false,
validateCodeExpired: false,
});

if (!response.accountExists) {
Expand Down Expand Up @@ -238,7 +240,7 @@ function resetPassword() {
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: true, forgotPassword: true});
API.ResetPassword({email: credentials.login})
.finally(() => {
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false});
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false, validateCodeExpired: false});
});
}

Expand All @@ -252,7 +254,7 @@ function resetPassword() {
* @param {String} accountID
*/
function setPassword(password, validateCode, accountID) {
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true});
Onyx.merge(ONYXKEYS.ACCOUNT, {...CONST.DEFAULT_ACCOUNT_DATA, loading: true, validateCodeExpired: false});

API.SetPassword({
password,
Expand All @@ -267,8 +269,14 @@ function setPassword(password, validateCode, accountID) {

// This request can fail if the password is not complex enough
Onyx.merge(ONYXKEYS.ACCOUNT, {error: response.message});
})
.finally(() => {
}).catch((errResponse) => {
const login = lodashGet(errResponse, 'data.email', null);
Onyx.merge(ONYXKEYS.ACCOUNT, {validated: false, validateCodeExpired: true});
if (login) {
Onyx.merge(ONYXKEYS.CREDENTIALS, {login});
}
Navigation.navigate(ROUTES.HOME);
}).finally(() => {
Onyx.merge(ONYXKEYS.ACCOUNT, {loading: false});
});
}
Expand Down
10 changes: 4 additions & 6 deletions src/pages/signin/ResendValidationForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import _ from 'underscore';
import Str from 'expensify-common/lib/str';
import styles from '../../styles/styles';
import Button from '../../components/Button';
import Text from '../../components/Text';
Expand Down Expand Up @@ -34,6 +33,9 @@ const propTypes = {
closed: PropTypes.bool,
}),

/** Title to be shown in the form */
titleMessage: PropTypes.string.isRequired,
Copy link
Contributor

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:

  1. We are not passing a message here we are passing a translation key
  2. It would be better to pass along something like an errors object and then have the form itself define the translation keys to use.

Copy link
Contributor

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.


...withLocalizePropTypes,
};

Expand Down Expand Up @@ -84,11 +86,7 @@ class ResendValidationForm extends React.Component {
<>
<View>
<Text>
{this.props.translate('resendValidationForm.weSentYouMagicSignInLink', {
loginType: (Str.isSMSLogin(this.props.credentials.login)
? this.props.translate('common.phoneNumber').toLowerCase()
: this.props.translate('common.email')).toLowerCase(),
})}
{this.props.titleMessage}
</Text>
</View>
{!_.isEmpty(this.state.formSuccess) && (
Expand Down
20 changes: 17 additions & 3 deletions src/pages/signin/SignInPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../../ONYXKEYS';
import styles from '../../styles/styles';
import updateUnread from '../../libs/UnreadIndicatorUpdater/updateUnread/index';
Expand Down Expand Up @@ -68,7 +70,8 @@ class SignInPage extends Component {

const validAccount = this.props.account.accountExists
&& this.props.account.validated
&& !this.props.account.forgotPassword;
&& !this.props.account.forgotPassword
&& !this.props.account.validateCodeExpired;

// Show the password form if
// - A login has been entered
Expand All @@ -85,7 +88,18 @@ class SignInPage extends Component {
// - AND an account did not exist or is not validated for that login
const showResendValidationLinkForm = this.props.credentials.login && !validAccount;

const welcomeText = this.props.translate(`welcomeText.${showPasswordForm ? 'phrase4' : 'phrase1'}`);
const welcomeText = showResendValidationLinkForm && this.props.account.validateCodeExpired
? ''
: this.props.translate(`welcomeText.${showPasswordForm ? 'phrase4' : 'phrase1'}`);

const validateCodeExpired = lodashGet(this.props.account, 'validateCodeExpired', false);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unnessary line break. No reason to have two line breaks here.

const resendLinkTitleMessage = this.props.translate(`resendValidationForm.${validateCodeExpired ? 'validationCodeFailedMessage' : 'weSentYouMagicSignInLink'}`,
Copy link
Contributor

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(),
    });
}

Copy link
Contributor Author

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.

Copy link
Contributor

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.

{
loginType: (!this.props.credentials.login || Str.isSMSLogin(this.props.credentials.login)
? this.props.translate('common.phoneNumber').toLowerCase()
: this.props.translate('common.email')).toLowerCase(),
});

return (
<>
Expand All @@ -99,7 +113,7 @@ class SignInPage extends Component {

{showPasswordForm && <PasswordForm />}

{showResendValidationLinkForm && <ResendValidationForm />}
{showResendValidationLinkForm && <ResendValidationForm titleMessage={resendLinkTitleMessage} />}
</SignInPageLayout>
</SafeAreaView>
</>
Expand Down